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

[Jeff Cody]

On Wed, May 21, 2014 at 03:13:15PM +0200, Stefan Hajnoczi wrote:
> 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.
>

I agree (this patch and the other op blocker ones before this already
have my reviewed-by).