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