From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58130) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wn6Ki-0004mz-7g for qemu-devel@nongnu.org; Wed, 21 May 2014 09:13:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wn6Kd-00027o-31 for qemu-devel@nongnu.org; Wed, 21 May 2014 09:13:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33893) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wn6Kc-000277-Qd for qemu-devel@nongnu.org; Wed, 21 May 2014 09:13:19 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4LDDIpE006392 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 21 May 2014 09:13:18 -0400 Date: Wed, 21 May 2014 15:13:15 +0200 From: Stefan Hajnoczi Message-ID: <20140521131315.GD6619@stefanha-thinkpad.redhat.com> References: <1400565880-13409-1-git-send-email-famz@redhat.com> <1400565880-13409-5-git-send-email-famz@redhat.com> <20140520114334.GA5292@localhost.localdomain> <20140521013608.GB18886@T430.nay.redhat.com> <20140521043436.GA13700@localhost.localdomain> <20140521060846.GF18886@T430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140521060846.GF18886@T430.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, qemu-devel@nongnu.org, Jeff Cody , hbrock@redhat.com, armbru@redhat.com, rjones@redhat.com, imain@redhat.com, pbonzini@redhat.com On Wed, May 21, 2014 at 02:08:46PM +0800, Fam Zheng wrote: > On Wed, 05/21 00:34, Jeff Cody wrote: > > On Wed, May 21, 2014 at 09:36:08AM +0800, Fam Zheng wrote: > > > On Tue, 05/20 07:43, Jeff Cody wrote: > > > > On Tue, May 20, 2014 at 02:04:29PM +0800, Fam Zheng wrote: > > > > > It makes no sense to check for "any" blocker on bs, we are here only > > > > > because of the mechanical conversion from in_use to op_blockers. Remove > > > > > it now, and let the callers check specific operation types. Backup and > > > > > mirror already have it, add checker to stream and commit. > > > > > > > > > > Signed-off-by: Fam Zheng > > > > > Reviewed-by: Benoit Canet > > > > > Reviewed-by: Jeff Cody > > > > > --- > > > > > blockdev.c | 8 ++++++++ > > > > > blockjob.c | 2 +- > > > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/blockdev.c b/blockdev.c > > > > > index 5d950fa..21fc55b 100644 > > > > > --- a/blockdev.c > > > > > +++ b/blockdev.c > > > > > @@ -1850,6 +1850,10 @@ void qmp_block_stream(const char *device, bool has_base, > > > > > return; > > > > > } > > > > > > > > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { > > > > > + return; > > > > > + } > > > > > + > > > > > if (base) { > > > > > base_bs = bdrv_find_backing_image(bs, base); > > > > > if (base_bs == NULL) { > > > > > @@ -1894,6 +1898,10 @@ void qmp_block_commit(const char *device, > > > > > return; > > > > > } > > > > > > > > > > + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { > > > > > + return; > > > > > + } > > > > > + > > > > > > > > Is the blocker intended to operate at the device level, i.e. to mark a > > > > whole chain as 'blocked' for one or more operations? Or, is it > > > > intended to block at the singular BDS level (the commit message in > > > > patch 2 implies this meaning)? > > > > > > Good question! It should be per BDS, that's why we need backing_blocker. > > > > > > Fam > > > > > > > > > > > More to the point: if a BDS is marked as blocked, does that also imply > > > > all of the images in its backing chain are also considered blocked? > > > > > > No. > > > > Then why don't we check the blocker values for overlay_bs, top_bs, > > base_bs, and intermediate images in qmp_block_commit()? This patch > > only checks the active bs blocker.. yet in some commits, the active > > layer BDS may not actually be affected at all. > > > > > > > > > Conversely, if a BDS is *not* marked as blocked, does that mean all of > > > > its backing chain is also unblocked? > > > > > > No. But all the backing_hd is blocked by backing_blocker, so we are safe. > > > > > > With node-name introduced, some qmp operations are accessible on a BDS in the > > > middle of a chain, with node-name argument. E.g. @BlockdevSnapshot (Hmm, why > > > would blockdev-snapshot operate on node-name, when it's called blockdev-*?). > > > > > > > Thanks - and that is exactly what prompted my question; with > > node-name, we don't necessarily start at the top of the chain, and we > > can (and will) reference BDSs individually. > > > > > > > So the question is, what happens if user tries to take some operation on mid, > > > with the node-name: > > > > > > base <-- mid <-- active > > > > > > With this series, we are safe because mid is protected by the backing_blocker > > > of active, which blocks all the operations on mid, except as commit target and > > > backup source. > > > > > > > Right, but that commit exception disproves the rule of 'blockers work > > on the BDS level'. > > > > It means intermediate images (anything below active) will not be > > blocked for commit. And this ends up working OK (I think even with my > > block-commit node-name patches), because we still use the 'device' to > > lookup the active BDS, and that one BDS will be blocked and we catch > > that in this patch. But that may not always be the case, particular > > since not all block-commits even involve the active layer. > > > > And this means that we are back to the first part - the active BDS > > blocker is effectively for the chain, not the BDS. We rely on the > > fact that the active layer BDS will be blocked, to make up for the > > fact that the rest of the chain is not blocked for commit. > > Right. It is not practical to apply the semantics of op blocker on a whole > chain, because we need different permissions on active and its backing, and ... > > > > > So to be safe, and to use the blocker as individual BDS blockers, > > shouldn't we do a bdrv_op_block_all() in block_job_create() > > for every BDS in a chain affected by the block job, and then clear our > > exception whitelist for those same BDSs (if they still exist!) when > > the block job is complete (or on failure to start the job)? That way, > > we are not relying on the active layer blocker acting as a proxy > > blocker for the entire chain. > > ... backing_blocker is introduced to save the effort to add blockers > recursively. I think it means we should distinguish COMMIT_SOURCE and > COMMIT_TARGET, as in block-backup, and only allow COMMIT_TARGET on backing_hd. > > Could this make us safe? What does it mean to your node-name commit changes? We have these node-name issues independent of this patch series. bdrv_in_use() has the same problem that node-name allows you to reference nodes that we never called bdrv_set_in_use() on. I think the discussion is important but should not block this patch series. Stefan