svn commit: r349391 - head/sys/kern

[John Baldwin]

On 7/14/19 12:08 PM, Conrad Meyer wrote:
> Hi Alan,
> 
> This change restores the possible overflow beyond IO_SEQMAX that the
> removed conditional prevented.
> 
> On Tue, Jun 25, 2019 at 12:44 PM Alan Somers <asomers at freebsd.org> wrote:
>>
>> Author: asomers
>> Date: Tue Jun 25 19:44:22 2019
>> New Revision: 349391
>> URL: https://svnweb.freebsd.org/changeset/base/349391
>>
>> --- head/sys/kern/vfs_vnops.c   Tue Jun 25 19:36:01 2019        (r349390)
>> +++ head/sys/kern/vfs_vnops.c   Tue Jun 25 19:44:22 2019        (r349391)
>> @@ -499,10 +499,8 @@ sequential_heuristic(struct uio *uio, struct file *fp)
>>                  * closely related to the best I/O size for real disks than
>>                  * to any block size used by software.
>>                  */
>> -               fp->f_seqcount += MIN(IO_SEQMAX,
>> +               fp->f_seqcount += lmin(IO_SEQMAX,
>>                     howmany(uio->uio_resid, 16384));
>> -               if (fp->f_seqcount > IO_SEQMAX)
>> -                       fp->f_seqcount = IO_SEQMAX;
>>                 return (fp->f_seqcount << IO_SEQSHIFT);
>>         }
> 
> Perhaps instead this should be:
> 
> fp->f_seqcount = lmin(IO_SEQMAX,
>   fp->f_seqcount + howmany(...));

While I agree with your assessment of the removed conditional, I think the
suggestion above can have a funky overflow where f_seqcount + howmany(...) ends
up less than IO_SEQMAX and this expression would then be used instead of IO_SEQMAX?
The first lmin seems designed to minimize what we add in the hope that since the
existing value is bounded to '0...IO_SEQMAX' the result of the += can only be in
the range '0...2*IO_SEQMAX'.

I'm not sure which variants are most readable:

A)
    fp->f_seqcount += lmin(IO_SEQMAX,
        howmany(resid, 16384));
    if (fp->f_seqcount > IO_SEQMAX)
            fp->f_seqcount = IO_SEQMAX;

B)
    fp->f_seqcount += lmin(IO_SEQMAX,
        howmany(resid, 16384));
    fp->f_seqcount = lmin(fp->f_seqcount, IO_SEQMAX);


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).    

-- 
John Baldwin