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

[Jeff Cody <jcody@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?
>

It is still safe with my node-name commit (and stream in v2) series,
because they still reference the chain via 'device', so we can check
the blocker there.

I'm not sure the ultimate solution is to break commit into
COMMIT_TARGET and COMMIT_SOURCE, at least not yet.  Commit will stay
safe for a while yet without it, and I think the ultimate solution is
to somehow either have the blocker truly operate on the individual BDS
level, or provide a chain-wide blocker that exists beyond just the
active layer (which doesn't seem unreasonable for certain operations).

Until that happens, what we need to do is make sure that any block job
operation continues to reference the 'device' and top-level BDS, since
we are in this hybrid mode of operation.

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