[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from blo
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller |
Date: |
Wed, 21 May 2014 15:13:15 +0200 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <address@hidden>
> > > > > Reviewed-by: Benoit Canet <address@hidden>
> > > > > Reviewed-by: Jeff Cody <address@hidden>
> > > > > ---
> > > > > 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
- Re: [Qemu-devel] [PATCH v20 01/15] block: Add BlockOpType enum, (continued)
Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller, Stefan Hajnoczi, 2014/05/21
[Qemu-devel] [PATCH v20 05/15] block: Add bdrv_set_backing_hd(), Fam Zheng, 2014/05/20
[Qemu-devel] [PATCH v20 07/15] block: Parse "backing" option to reference existing BDS, Fam Zheng, 2014/05/20
[Qemu-devel] [PATCH v20 06/15] block: Add backing_blocker in BlockDriverState, Fam Zheng, 2014/05/20