svn commit: r349391 - head/sys/kern

[Bruce Evans]

On Tue, 16 Jul 2019, Alan Somers wrote:

> On Tue, Jul 16, 2019 at 3:48 AM Bruce Evans <brde at optusnet.com.au> wrote:
>>
>> On Mon, 15 Jul 2019, John Baldwin wrote:
>>> ...
>>> I'm not sure which variants are most readable:
>> ...
>>> C)
>>>    if (fp->f_seqcount + howmany(resid, 16384) < fp->f_seqcount)
>>>            fp->f_seqcount = IO_SEQMAX;
>>>    else
>>>            fp->f_seqcount = lmin(IO_SEQMAX,
>>>                fp->f_seqcount + howmany(resid, 16384));
>>>
>>> I'm probably not a fan of C).
>>
>> On supported arches, it recovers from overflow in howmany(), but only if
>> the compiler doesn't optimize away the recovery.
>>
>> The first part can be done better:
>>
>>         if (uio->uio_resid >= IO_SEQMAX * 16384)
>>                 fp->f_seqcount = IO_SEQMAX;
>>
>> Then for the second part, any version works since all values are small and
>> non-negative, but I prefer to restore the the version that I rewrote 15-20
>> years ago and committed 11 years ago (r175106):
>>
>>                 fp->f_seqcount += howmany(uio->uio_resid, 16384);
>>                 if (fp->f_seqcount > IO_SEQMAX)
>>                         fp->f_seqcount = IO_SEQMAX;
>> ...
> Bruce,
>    Is this how you want it?
> Index: sys/kern/vfs_vnops.c
> ===================================================================
> --- sys/kern/vfs_vnops.c    (revision 349977)
> +++ sys/kern/vfs_vnops.c    (working copy)
> @@ -499,8 +499,13 @@
>          * closely related to the best I/O size for real disks than
>          * to any block size used by software.
>          */
> -        fp->f_seqcount += lmin(IO_SEQMAX,
> -            howmany(uio->uio_resid, 16384));
> +        if (uio->uio_resid >= IO_SEQMAX * 16384)
> +            fp->f_seqcount = IO_SEQMAX;
> +        else {
> +            fp->f_seqcount += howmany(uio->uio_resid, 16384);
> +            if (fp->f_seqcount > IO_SEQMAX)
> +                fp->f_seqcount = IO_SEQMAX;
> +        }
>         return (fp->f_seqcount << IO_SEQSHIFT);
>     }

That looks OK, except it is misformatted with tabs corrupted to other than
8 spaces.

I checked that uio_resid is checked by some callers to be >= 0, so we
don't have to check that here.  (Callers between userland and here
start with size_t nbytes or an array of sizes for readv(), and have
to check that nbytes or the sum of the sizes fits in ssize_t, else
overflow would alread have occurred on assigment to uio_resid.  Callers
that construct a uio that is not so directly controlled by userland
presumably don't construct ones with unusable sizes.)

Bruce