From owner-svn-src-head@freebsd.org Thu Jun 27 21:48:33 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E54F615CF336; Thu, 27 Jun 2019 21:48:32 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 871F772D5B; Thu, 27 Jun 2019 21:48:32 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 6825A20241; Thu, 27 Jun 2019 17:48:26 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Thu, 27 Jun 2019 17:48:26 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; s=fm3; bh=5 OdQlXMjzArjBfKmAqhdIKW6iT3PA7DhTAilKANGuaY=; b=yjwdjUPzlJuZihuE/ 9iexOvO0+SmOh2VeBwROgImdy73YDWS8xtB0WO3HPv5kFufTgC+rVxmdQblQA0LV gAXSIkZxxKtV4YZI+CzgypSnLtGvE6T60/AENR4qXNSDVZdlAJFvnkJ92LbTjJlX gTugLD3NZyYJtjZR9EFc3X+fS6v5DKHcsLwxgEj0p1JykUuCXW8DhCj7jbXyjXU4 WXhWMeAbnFTXJd+qiOZUD3myjmUJelmb9/xXan0LaUtpFwxT5BNvDCocsjGB/37w TxV50524A/F/grfZim8JLG5k1ycAw90qcb5h8edSbvA6YPD336d7z4vqB0sVSMCq hQgLA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=5OdQlXMjzArjBfKmAqhdIKW6iT3PA7DhTAilKANGu aY=; b=oUdpY+VT1xxGGPRxJs8tSOZ17JjddORvZzCAnDAw8IfNJE1XCHOjFZ7Z4 zg6EMx9Ne22a8QKHTp4YoZo5nc9/mEQZUEbLTG69nVgEjC+RUoM2aE/JItBCm/6T VWmF0V5KsC+TecwR9TDQx1qXz3OlMhkpFkWpVndawlCkx0BxCczs3W8IBvDWF5ui A3ecMiL8AKzDB20Jeij2+PaNuxyjzStQmeECT4HlTMDoumcuTeA06x/lY2Qu0q47 VOHc+QuAm4hOVWwHTrAzWn8BMAtvjblQiEY+niW6xv9b3n2O1NfbH2PAs/0KyD+N GIVEQHesmPeejaYoudt4vVzZ1WU8A== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduvddrudelgddtvdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpegtggfuhfgjfffgkfhfvffosehtqhhmtdhhtdejnecuhfhrohhmpefutghothht ucfnohhnghcuoehstghothhtlhesshgrmhhstghordhorhhgqeenucffohhmrghinhepfh hrvggvsghsugdrohhrghenucfkphepudelvddrheehrdehgedriedtnecurfgrrhgrmhep mhgrihhlfhhrohhmpehstghothhtlhesshgrmhhstghordhorhhgnecuvehluhhsthgvrh fuihiivgeptd X-ME-Proxy: Received: from [10.178.24.8] (unknown [192.55.54.60]) by mail.messagingengine.com (Postfix) with ESMTPA id 19CEE380086; Thu, 27 Jun 2019 17:48:24 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs From: Scott Long In-Reply-To: <20190627210940.GR8697@kib.kiev.ua> Date: Thu, 27 Jun 2019 15:48:23 -0600 Cc: Alan Somers , src-committers , svn-src-all , svn-src-head Content-Transfer-Encoding: quoted-printable Message-Id: <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org> References: <201906201413.x5KEDB5u010923@repo.freebsd.org> <20190627191843.GQ8697@kib.kiev.ua> <20190627210940.GR8697@kib.kiev.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.3445.104.11) X-Rspamd-Queue-Id: 871F772D5B X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.96 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.96)[-0.962,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Jun 2019 21:48:33 -0000 > On Jun 27, 2019, at 3:09 PM, Konstantin Belousov = wrote: >=20 > On Thu, Jun 27, 2019 at 02:01:11PM -0600, Alan Somers wrote: >> On Thu, Jun 27, 2019 at 1:18 PM Konstantin Belousov = wrote: >>>=20 >>> On Thu, Jun 20, 2019 at 02:13:11PM +0000, Alan Somers wrote: >>>> Author: asomers >>>> Date: Thu Jun 20 14:13:10 2019 >>>> New Revision: 349231 >>>> URL: https://svnweb.freebsd.org/changeset/base/349231 >>>>=20 >>>> Log: >>>> Add FIOBMAP2 ioctl >>>=20 >>>>=20 >>>> This ioctl exposes VOP_BMAP information to userland. It can be = used by >>>> programs like fragmentation analyzers and optimized cp = implementations. But >>>> I'm using it to test fusefs's VOP_BMAP implementation. The "2" in = the name >>>> distinguishes it from the similar but incompatible FIBMAP ioctls = in NetBSD >>>> and Linux. FIOBMAP2 differs from FIBMAP in that it uses a 64-bit = block >>>> number instead of 32-bit, and it also returns runp and runb. >>>>=20 >>>> Reviewed by: mckusick >>>> MFC after: 2 weeks >>>> Sponsored by: The FreeBSD Foundation >>>> Differential Revision: https://reviews.freebsd.org/D20705 >>>>=20 >>>> Modified: >>>> head/sys/kern/vfs_vnops.c >>>> head/sys/sys/filio.h >>>> head/sys/ufs/ufs/ufs_bmap.c >>>>=20 >>>> Modified: head/sys/kern/vfs_vnops.c >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- head/sys/kern/vfs_vnops.c Thu Jun 20 13:59:46 2019 = (r349230) >>>> +++ head/sys/kern/vfs_vnops.c Thu Jun 20 14:13:10 2019 = (r349231) >>>> @@ -1458,6 +1458,25 @@ vn_stat(struct vnode *vp, struct stat *sb, = struct ucre >>>> return (0); >>>> } >>>>=20 >>>> +/* generic FIOBMAP2 implementation */ >>>> +static int >>>> +vn_ioc_bmap2(struct file *fp, struct fiobmap2_arg *arg, struct = ucred *cred) >>> I do not like the fact that internal kernel function takes the >>> user-visible structure which results in the mix of ABI and KBI. >>> Traditionally we pass explicit arguments to kern_XXX, vn_XXX and = similar >>> internal implementations. >>=20 >> Ok. It will increase the number of function arguments, which is = ugly, >> but I can do it if you prefer. >>=20 >>>=20 >>>> +{ >>>> + struct vnode *vp =3D fp->f_vnode; >>> Why do you pass fp to the function that >>> 1. Has vn_ namespace, i.e. operating on vnode. >>> 2. Only needs vnode to operate on. >>> Please change the argument from fp to vp. You would need to pass = f_cred >>> as additional argument, or move mac check to vn_ioctl (I think this = is >>> a better approach). >>=20 >> Ok. >>=20 >>>=20 >>>> + daddr_t lbn =3D arg->bn; >>> Style: initialization in declaration. >>>=20 >>>> + int error; >>>> + >>>> + vn_lock(vp, LK_SHARED | LK_RETRY); >>>> +#ifdef MAC >>>> + error =3D mac_vnode_check_read(cred, fp->f_cred, vp); >>>> + if (error =3D=3D 0) >>>> +#endif >>>> + error =3D VOP_BMAP(vp, lbn, NULL, &arg->bn, = &arg->runp, >>>> + &arg->runb); >>> Wrong indent for continuation line. >>>=20 >>>> + VOP_UNLOCK(vp, 0); >>>> + return (error); >>>> +} >>>> + >>>> /* >>>> * File table vnode ioctl routine. >>>> */ >>>> @@ -1481,6 +1500,9 @@ vn_ioctl(struct file *fp, u_long com, void = *data, stru >>>> if (error =3D=3D 0) >>>> *(int *)data =3D vattr.va_size - = fp->f_offset; >>>> return (error); >>>> + case FIOBMAP2: >>>> + return (vn_ioc_bmap2(fp, (struct = fiobmap2_arg*)data, >>> Need space between fiobmap2_arg and '*'. >>>> + active_cred)); >>> Wrong indent. >>>=20 >>>> case FIONBIO: >>>> case FIOASYNC: >>>> return (0); >>>>=20 >>>> Modified: head/sys/sys/filio.h >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- head/sys/sys/filio.h Thu Jun 20 13:59:46 2019 = (r349230) >>>> +++ head/sys/sys/filio.h Thu Jun 20 14:13:10 2019 = (r349231) >>>> @@ -62,6 +62,13 @@ struct fiodgname_arg { >>>> /* Handle lseek SEEK_DATA and SEEK_HOLE for holey file knowledge. = */ >>>> #define FIOSEEKDATA _IOWR('f', 97, off_t) /* SEEK_DATA = */ >>>> #define FIOSEEKHOLE _IOWR('f', 98, off_t) /* SEEK_HOLE = */ >>>> +struct fiobmap2_arg { >>>> + int64_t bn; >>>> + int runp; >>>> + int runb; >>>> +}; >>> This structure has different layout for LP64 and ILP32, and you did = not >>> provided the compat shims. >>=20 >> Really? How so? All of the fields have the same width on 64 and 32 >> bit archs, and there shouldn't be any need for padding, either. > Sorry, you are right. >=20 >>=20 >>>=20 >>>> +/* Get the file's bmap info for the logical block bn */ >>>> +#define FIOBMAP2 _IOWR('f', 99, struct fiobmap2_arg) >>>>=20 >>>> #ifdef _KERNEL >>>> #ifdef COMPAT_FREEBSD32 >>>>=20 >>>> Modified: head/sys/ufs/ufs/ufs_bmap.c >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- head/sys/ufs/ufs/ufs_bmap.c Thu Jun 20 13:59:46 2019 = (r349230) >>>> +++ head/sys/ufs/ufs/ufs_bmap.c Thu Jun 20 14:13:10 2019 = (r349231) >>>> @@ -200,12 +200,15 @@ ufs_bmaparray(vp, bn, bnp, nbp, runp, runb) >>>> *bnp =3D blkptrtodb(ump, = ip->i_din2->di_extb[-1 - bn]); >>>> if (*bnp =3D=3D 0) >>>> *bnp =3D -1; >>>> - if (nbp =3D=3D NULL) >>>> - panic("ufs_bmaparray: mapping ext = data"); >>>> + if (nbp =3D=3D NULL) { >>>> + /* indirect block not found */ >>>> + return (EINVAL); >>>> + } >>> This (and next chunk) loose useful checks for in-kernel interfaces. >>=20 >> mckusick disagrees with you on this. He thought that changing the >> panics into errors was a good idea. Note that ufs_bmap was already >> capable of returning errors that not all callers check. > For user-callable code, this is of course the requirement. But for = in-kernel > use, IMO it makes the interfaces loose. I=E2=80=99m firmly in agreement on changing panics into errors. When = callers are encountered that don=E2=80=99t properly check and propagate the errors, = they should be fixed too. I=E2=80=99m not saying that it=E2=80=99s an easy = process and that we should just delete all panics, but it=E2=80=99s a goal that should be = vigorously pursued. Scott