Re: [Qemu-devel] [PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller

[Stefan Hajnoczi <stefanha@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 <famz@redhat.com>
> > > > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > > > > Reviewed-by: Jeff Cody <jcody@redhat.com>
> > > > > ---
> > > > >  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