From owner-svn-src-all@freebsd.org Wed Jul 17 14:07:23 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id F3F40AD804; Wed, 17 Jul 2019 14:07:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id AC39C82E50; Wed, 17 Jul 2019 14:07:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 7DBC93DCC08; Thu, 18 Jul 2019 00:07:02 +1000 (AEST) Date: Thu, 18 Jul 2019 00:07:00 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alan Somers cc: Bruce Evans , John Baldwin , "Conrad E. Meyer" , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r349391 - head/sys/kern In-Reply-To: Message-ID: <20190717235525.L1726@besplex.bde.org> References: <201906251944.x5PJiNaA093352@repo.freebsd.org> <3b2ade4e-d19d-31e4-7ee2-c0085a00e53e@FreeBSD.org> <20190716180618.C962@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=D+Q3ErZj c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=sS6U7fMKXT-JdnrxqNEA:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: AC39C82E50 X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; spf=pass (mx1.freebsd.org: domain of brde@optusnet.com.au designates 211.29.132.42 as permitted sender) smtp.mailfrom=brde@optusnet.com.au X-Spamd-Result: default: False [-5.44 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.996,0]; RCVD_COUNT_TWO(0.00)[2]; FROM_HAS_DN(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:211.29.132.0/23]; FREEMAIL_FROM(0.00)[optusnet.com.au]; MIME_GOOD(-0.10)[text/plain]; MIME_TRACE(0.00)[0:+]; DMARC_NA(0.00)[optusnet.com.au]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; MX_GOOD(-0.01)[extmail.optusnet.com.au]; NEURAL_HAM_SHORT(-0.54)[-0.536,0]; RCPT_COUNT_SEVEN(0.00)[7]; IP_SCORE(-2.60)[ip: (-7.44), ipnet: 211.28.0.0/14(-3.22), asn: 4804(-2.34), country: AU(0.01)]; RCVD_NO_TLS_LAST(0.10)[]; RCVD_IN_DNSWL_LOW(-0.10)[42.132.29.211.list.dnswl.org : 127.0.5.1]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[optusnet.com.au]; ASN(0.00)[asn:4804, ipnet:211.28.0.0/14, country:AU]; FREEMAIL_CC(0.00)[optusnet.com.au]; FROM_EQ_ENVFROM(0.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Jul 2019 14:07:23 -0000 On Tue, 16 Jul 2019, Alan Somers wrote: > On Tue, Jul 16, 2019 at 3:48 AM Bruce Evans 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