Date: Mon, 15 Jul 2019 15:28:40 -0700 From: John Baldwin <jhb@FreeBSD.org> To: cem@freebsd.org, Alan Somers <asomers@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349391 - head/sys/kern Message-ID: <3b2ade4e-d19d-31e4-7ee2-c0085a00e53e@FreeBSD.org> In-Reply-To: <CAG6CVpUscojbGvA0=BmZPhSR%2B5Yt5ah8SQKrBaouUNDpu9tw9A@mail.gmail.com> References: <201906251944.x5PJiNaA093352@repo.freebsd.org> <CAG6CVpUscojbGvA0=BmZPhSR%2B5Yt5ah8SQKrBaouUNDpu9tw9A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3b2ade4e-d19d-31e4-7ee2-c0085a00e53e>