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

[Fam Zheng]

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.

> 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-*?).

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.

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.

Fam

> 
> 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
> >