Date: Mon, 9 May 2016 11:21:42 -0600 From: Warner Losh <imp@bsdimp.com> To: Steven Hartland <steven@multiplay.co.uk> Cc: Alan Somers <asomers@freebsd.org>, Warner Losh <imp@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: <14D0B1CA-8F30-4DE4-A4F7-424F75BFE07E@bsdimp.com> In-Reply-To: <405a3410-a0ee-a638-eefe-c1f8980e5624@freebsd.org> References: <201602171716.u1HHG2c2098316@repo.freebsd.org> <CAOtMX2j28M9oTV3BFGkKQ8s5sX1vHBeU-_xeM4GGUBrVdzypYQ@mail.gmail.com> <405a3410-a0ee-a638-eefe-c1f8980e5624@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_041D1166-B93C-41B0-B5C1-8A46666EFDAD Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On May 9, 2016, at 11:14 AM, Steven Hartland <steven@multiplay.co.uk> = wrote: >=20 >=20 >=20 > On 09/05/2016 18:04, Alan Somers wrote: >>=20 >>=20 >> On Wed, Feb 17, 2016 at 10:16 AM, Warner Losh <imp@freebsd.org> = wrote: >> Author: imp >> Date: Wed Feb 17 17:16:02 2016 >> New Revision: 295707 >> URL: https://svnweb.freebsd.org/changeset/base/295707 >>=20 >> 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. >>=20 >> 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 >>=20 >> smh noticed that while your commit message says that g_reset_bio is = mandatory after g_{new,alloc}_bio, your diff only replaced existing = calls to bzero. You didn't insert g_reset_bio calls after all = g_alloc_bio calls, for example 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? >>=20 > 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 call g_reset_bio. I don=E2=80=99t. Please see my other reply. It=E2=80=99s only when you = *RE*use the bio that you need to call g_reset_bio(), not when you use it = in the 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 = fresh one hence its not guaranteed to be bzeroed? If so why have the = caller responsible, 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 = contents 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 previously. > If the concept of this was to ensure correctly initialised objects = from uma 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=99= t worked my way back to geom=E2=80=A6 Warner --Apple-Mail=_041D1166-B93C-41B0-B5C1-8A46666EFDAD Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJXMMcmAAoJEGwc0Sh9sBEAYSYQAM4I+/PnoOVFILRrQ/DLP3Co krJkuy9eG4DPDW3qI1u8Qq3KglKLnANVh/aMqfC7MyccpQ4xf5DkWXObeljZ8W5/ 59XX4uMJbZKNBDoBTM3eAtP3G1GZoHJkypEvmaGR83Yv+3nqcUlA1MQpv0LkTURg kopoGTf7jMgiNGQCyPTzF6WkVrhCBKi0M+UmwfRaaM6Wvcnh4QTolfhErIWbVeqB /K0B2yj9DJiWjDPmgSrSfzSM4iv83bA/+Q9qQE3EqZc3uBuz5m7fw237JE/96+eJ qDRgnw+NuFuaoLjXa+tq6gPhjhT1kJuVtOR0OnrCxnhSijpnqS+8FlICZ3pMjYsw V4V0DdP4+9k4RdG2HOd2x5omVKilULMNUuDrJZEuAn9NeJfMDL9v+4X2zBkwytq2 xuY1UqMfGYUz+J9X7hCG6UhwGoPLoNMATHZCVOJN7gOx6tXe8SD1yUX2Pu8SQamz XoEdGg1lX8hwYj7EXo1wkSr7eihnfNjQuCdcwzWNaJlmzqbwsyWTIYRY7EgIEvl+ kfCjtQXcS62J/hfKOdxxQprJfh4J0yxdgI4ff/gKBncYph28NqdRD6V2UnX/4zyJ TPC7y+4ccIR28DafnSyMoujuoq1u4jMPeAwgBY+guZE5rZN62K6dWzZUxLEi8Xj/ StMWuv4Xwe1ijHOYnwHB =Dqc+ -----END PGP SIGNATURE----- --Apple-Mail=_041D1166-B93C-41B0-B5C1-8A46666EFDAD--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?14D0B1CA-8F30-4DE4-A4F7-424F75BFE07E>