From owner-svn-src-all@freebsd.org Mon May 9 18:21:05 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 730B2B34A3B for ; Mon, 9 May 2016 18:21:05 +0000 (UTC) (envelope-from steven@multiplay.co.uk) Received: from mail-wm0-x235.google.com (mail-wm0-x235.google.com [IPv6:2a00:1450:400c:c09::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0985118E5 for ; Mon, 9 May 2016 18:21:04 +0000 (UTC) (envelope-from steven@multiplay.co.uk) Received: by mail-wm0-x235.google.com with SMTP id e201so148442327wme.0 for ; Mon, 09 May 2016 11:21:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=multiplay-co-uk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to; bh=uJs+hRm0AqiMXHL2w6ugvZYfd80GXtZs9z8m8kLPFE4=; b=OT5jz+Sexccipq3i/6fNFCugbDL+AZAygtFGGmHfYzYXRlPcL+qmcHjnVPWbtn2Sxv ooNDTrte6zUQ0wHPj75WlYYi+ditBJ3N4qu4zxwOuEHoCgUssyHZ67dPXNK/YM7+iK2f dgBufYrV44l8I1b99PEFuJFLCUr+z/k7zIwkzbcBsLbKw1ZAroD72OElESbKWCzIt3WK JldCM//OYIHBE6zJJimN9yyII5gRWdKisdKCSe7ZrKe19o1Ia+MQiI38e51CtgcuXJGv //SvZF0IWgUwuu9cHPOsTEyUV+U+Zf1XfZ6a0FwmPKAeKbaZm89x7EAXuTrlmHmsET/2 j1hA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=uJs+hRm0AqiMXHL2w6ugvZYfd80GXtZs9z8m8kLPFE4=; b=L090e3OLu5/M1SyRxDFduYGFa8FXGgAWFWLBTYAsKaPxH8kf2TrBXIH/cNrtWg8DvM RbSL9+hG8MW54XI7vTdZdoxxSpHSKhyJiwRshWB6PrRG8bY1DGV1idKtkHVI/ObjCRaQ 6XpjaAuK/7V1WYiqUonLRJCPY8rMZAyqORTi5YlnoyoENlpsbrfXoSK7LjQNGhdM2L6L Todj2m444DBVX3DS05eOQ6jQbOMJ+f67j7cBdzN0kEREvl/T5S/ggLRc6ZIg7EjaC1Aa qaSpQZfNndVUp3bnunxNVSGmovXe2SrFsr8PfjFVIp0Xz++O7ranOzKFYjM08s1qeTBg /h6A== X-Gm-Message-State: AOPr4FUE0t0XR+fE+3t+0VmA5NeqKjtPeg7qend4HcGG2LbWB3ZICEyDldOx2SHE0nxwRjZ6 X-Received: by 10.194.69.106 with SMTP id d10mr35169211wju.165.1462818063365; Mon, 09 May 2016 11:21:03 -0700 (PDT) Received: from [10.10.1.58] (liv3d.labs.multiplay.co.uk. [82.69.141.171]) by smtp.gmail.com with ESMTPSA id u4sm32522064wjz.4.2016.05.09.11.21.01 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 09 May 2016 11:21:01 -0700 (PDT) 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 To: Warner Losh References: <201602171716.u1HHG2c2098316@repo.freebsd.org> <405a3410-a0ee-a638-eefe-c1f8980e5624@freebsd.org> <14D0B1CA-8F30-4DE4-A4F7-424F75BFE07E@bsdimp.com> Cc: Alan Somers , Warner Losh , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" From: Steven Hartland Message-ID: Date: Mon, 9 May 2016 19:21:05 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <14D0B1CA-8F30-4DE4-A4F7-424F75BFE07E@bsdimp.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 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: Mon, 09 May 2016 18:21:05 -0000 On 09/05/2016 18:21, Warner Losh wrote: >> On May 9, 2016, at 11:14 AM, Steven Hartland wrote: >> >> >> >> On 09/05/2016 18:04, Alan Somers wrote: >>> >>> On Wed, Feb 17, 2016 at 10:16 AM, Warner Losh 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 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? >>> >> 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’t. Please see my other reply. It’s 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’s not the case at all. There’s 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’t 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’s to ensure internal state to the bio isn’t 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’ve started documenting CAM, but handn’t worked my way back to geom… > Thanks for the clarifications Warner, appreciated :) Regards Steve