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 <f...@redhat.com>
> > > Reviewed-by: Benoit Canet <ben...@irqsave.net>
> > > Reviewed-by: Jeff Cody <jc...@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.
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.
> 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
> > >
