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

[Fam Zheng]

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?

Thanks,
Fam

> 
> 
> > The idea is, we firstly block any operation in the middle of the chain with
> > backing_blocker, and we relax those that we think is safe and demanded. This
> > mechanism also protects the node-name based operations.
> >
> 
> I worry that as-is, what this really means is that the blockers on the
> intermediate images don't really do much.  Because any operation that
> will actually affect an intermediate image will need similar treatment
> as commit to relax the rules... so the actual operations you want to
> block all end up with exceptions to not be blocked.  But I think if we
> do what I outlined above, that could negate my fear.
> 
> > 
> > > 
> > > If the answer to the two questions above is 'yes', then the
> > > bdrv_op_block/unblock functions should probably operate recursively
> > > down the chain to the bottom-most backing file.
> > > 
> > > If the answer is 'no', then for some operations like stream and commit
> > > (and probably others), don't we also need to worry about the blocker
> > > state of a lot more images in the chain?
> > > 
> > > 
> > > >      /* default top_bs is the active layer */
> > > >      top_bs = bs;
> > > >  
> > > > diff --git a/blockjob.c b/blockjob.c
> > > > index 60e72f5..7d84ca1 100644
> > > > --- a/blockjob.c
> > > > +++ b/blockjob.c
> > > > @@ -41,7 +41,7 @@ void *block_job_create(const BlockJobDriver *driver, 
> > > > BlockDriverState *bs,
> > > >  {
> > > >      BlockJob *job;
> > > >  
> > > > -    if (bs->job || !bdrv_op_blocker_is_empty(bs)) {
> > > > +    if (bs->job) {
> > > >          error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> > > >          return NULL;
> > > >      }
> > > > -- 
> > > > 1.9.2
> > > > 
>