Date: Tue, 17 May 2016 10:33:44 -0600 From: Warner Losh <imp@bsdimp.com> To: Steven Hartland <steven@multiplay.co.uk> Cc: Alan Somers <asomers@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r295707 - in head/sys: cddl/contrib/opensolaris/uts/common/fs/zfs dev/mmc dev/virtio/block geom geom/journal geom/mirror geom/raid geom/raid3 kern Message-ID: <CANCZdfquhA9kjLExbBAnQemXKt2J8A6aTFLybXAJQeTiWos=Sg@mail.gmail.com> In-Reply-To: <c9e497a2-a0e3-168a-1f35-172aea1f66c8@multiplay.co.uk> References: <201602171716.u1HHG2c2098316@repo.freebsd.org> <CAOtMX2j28M9oTV3BFGkKQ8s5sX1vHBeU-_xeM4GGUBrVdzypYQ@mail.gmail.com> <405a3410-a0ee-a638-eefe-c1f8980e5624@freebsd.org> <14D0B1CA-8F30-4DE4-A4F7-424F75BFE07E@bsdimp.com> <c9e497a2-a0e3-168a-1f35-172aea1f66c8@multiplay.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
I've updated the g_bio man page with all the info presented here. Please let me know if I missed anything, or if you have suggestions for improvement. Warner On Mon, May 9, 2016 at 12:21 PM, Steven Hartland <steven@multiplay.co.uk> wrote: > On 09/05/2016 18:21, Warner Losh wrote: > > On May 9, 2016, at 11:14 AM, Steven Hartland <steven@multiplay.co.uk> <st= even@multiplay.co.uk> wrote: > > > > On 09/05/2016 18:04, Alan Somers wrote: > > > On Wed, Feb 17, 2016 at 10:16 AM, Warner Losh <imp@freebsd.org> <imp@free= bsd.org> wrote: > Author: imp > Date: Wed Feb 17 17:16:02 2016 > New Revision: 295707 > URL: https://svnweb.freebsd.org/changeset/base/295707 > > Log: > Create an API to reset a struct bio (g_reset_bio). This is mandatory > for all struct bio you get back from g_{new,alloc}_bio. Temporary > bios that you create on the stack or elsewhere should use this before > first use of the bio, and between uses of the bio. At the moment, it > is nothing more than a wrapper around bzero, but that may change in > the future. The wrapper also removes one place where we encode the > size of struct bio in the KBI. > > Modified: > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_geom.c > head/sys/dev/mmc/mmcsd.c > head/sys/dev/virtio/block/virtio_blk.c > head/sys/geom/geom.h > head/sys/geom/geom_io.c > head/sys/geom/journal/g_journal.c > head/sys/geom/mirror/g_mirror.c > head/sys/geom/raid/g_raid.c > head/sys/geom/raid3/g_raid3.c > head/sys/kern/kern_physio.c > > smh noticed that while your commit message says that g_reset_bio is manda= tory after g_{new,alloc}_bio, your diff only replaced existing calls to bze= ro. You didn't insert g_reset_bio calls after all g_alloc_bio calls, for e= xample in vdev_geom_io_start. Do you intend to follow up this change with = a g_reset_bio everywhere that g_alloc_bio is called, or did you mean that g= _reset_bio is optional after all bios returned by g_{new,alloc}_bio? > > > Yer I was just penning this too: > This commit was just referenced in https://reviews.freebsd.org/D6153 > It seems rather odd to require all callers to g_{new,alloc}_bio to also c= all g_reset_bio. > > I don=E2=80=99t. Please see my other reply. It=E2=80=99s only when you *R= E*use the bio that you need to call g_reset_bio(), not when you use it in t= he first place. You can no longer call bzero() on the bio to reset it. > > > I assume this is because uma can return an existing object instead of fre= sh one hence its not guaranteed to be bzeroed? If so why have the caller re= sponsible, seems petty error prone. A quick look at users of g_alloc_bio it= seems like this is something that's not done currently done in all places,= even some usages of memset still hanging around, are these cases a bug? > > No. That=E2=80=99s not the case at all. There=E2=80=99s going to be conte= nts of the BIO that cannot be blithely cleared by the users of the memory. = Many other structures in the kernel are like this, but bio wasn=E2=80=99t p= reviously. > > > If the concept of this was to ensure correctly initialised objects from u= ma would the callback handers to uma_zcreate not be a better option as that= would guarantee things are correct instead of leaving it to the caller? > > No. It=E2=80=99s to ensure internal state to the bio isn=E2=80=99t blown = away by a subsequent bzero() call before calling g_destroy_bio(). > > > As a side matter, this area really needs some man pages so the its clear = to all what is needed and when. > > Agreed. The whole storage stack, however, is wonderfully under-documented= . I=E2=80=99ve started documenting CAM, but handn=E2=80=99t worked my way b= ack to geom=E2=80=A6 > > > Thanks for the clarifications Warner, appreciated :) > > Regards > Steve >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfquhA9kjLExbBAnQemXKt2J8A6aTFLybXAJQeTiWos=Sg>