On Wed, 04/09 14:29, Jeff Cody wrote:
> On Mon, Mar 10, 2014 at 03:26:02PM +0800, Fam Zheng wrote:
> > This makes use of op_blocker and blocks all the operations except for
> > commit target, on each BlockDriverState->backing_hd.
> >
> > The asserts for op_blocker in bdrv_swap are removed because with this
> > change, the target of block commit has at least the backing blocker of
> > its child, so the assertion is not true. Callers should do their check.
> >
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > block.c | 24 ++++++++++++++++++++----
> > block/mirror.c | 1 +
> > include/block/block_int.h | 3 +++
> > 3 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 64738dc..95247c8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1050,16 +1050,33 @@ fail:
> > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState
> > *backing_hd)
> > {
> >
> > + if (bs->backing_hd) {
> > + assert(error_is_set(&bs->backing_blocker));
> > + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> > + } else if (backing_hd) {
> > + error_setg(&bs->backing_blocker,
> > + "device is used as backing hd of '%s'",
> > + bs->device_name);
> > + }
> > +
> > bs->backing_hd = backing_hd;
> > if (!backing_hd) {
> > bs->backing_file[0] = '\0';
> > bs->backing_format[0] = '\0';
> > + if (error_is_set(&bs->backing_blocker)) {
> > + error_free(bs->backing_blocker);
> > + }
> > goto out;
> > }
> > bs->open_flags &= ~BDRV_O_NO_BACKING;
> > pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> > backing_hd->filename);
> > pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> > backing_hd->drv ? backing_hd->drv->format_name : "");
> > +
> > + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> > + /* Otherwise we won't be able to commit due to check in bdrv_commit */
> > + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> > + bs->backing_blocker);
> > out:
> > bdrv_refresh_limits(bs);
> > }
> > @@ -1699,8 +1716,9 @@ void bdrv_close(BlockDriverState *bs)
> >
> > if (bs->drv) {
> > if (bs->backing_hd) {
> > - bdrv_unref(bs->backing_hd);
> > - bs->backing_hd = NULL;
> > + BlockDriverState *backing_hd = bs->backing_hd;
> > + bdrv_set_backing_hd(bs, NULL);
> > + bdrv_unref(backing_hd);
> > }
> > bs->drv->bdrv_close(bs);
> > g_free(bs->opaque);
> > @@ -1908,7 +1926,6 @@ void bdrv_swap(BlockDriverState *bs_new,
> > BlockDriverState *bs_old)
> > assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
> > assert(bs_new->job == NULL);
> > assert(bs_new->dev == NULL);
> > - assert(bdrv_op_blocker_is_empty(bs_new));
> > assert(bs_new->io_limits_enabled == false);
> > assert(!throttle_have_timer(&bs_new->throttle_state));
> >
> > @@ -1927,7 +1944,6 @@ void bdrv_swap(BlockDriverState *bs_new,
> > BlockDriverState *bs_old)
> > /* Check a few fields that should remain attached to the device */
> > assert(bs_new->dev == NULL);
> > assert(bs_new->job == NULL);
> > - assert(bdrv_op_blocker_is_empty(bs_new));
> > assert(bs_new->io_limits_enabled == false);
> > assert(!throttle_have_timer(&bs_new->throttle_state));
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index dd5ee05..6dc84e8 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -487,6 +487,7 @@ immediate_exit:
> > * trigger the unref from the top one */
> > BlockDriverState *p = s->base->backing_hd;
> > s->base->backing_hd = NULL;
> > + bdrv_op_unblock_all(p, s->base->backing_blocker);
> > bdrv_unref(p);
>
> FYI, this is what I changed in my testing, to try out the active layer
> case in bdrv_drop_intermediate(). Since you'll need to respin anyway,
> might as well clean this up to use the updated
> bdrv_drop_intermediate():
Good idea!
However bdrv_drop_intermediate() doesn't work for mirror job, because they are
not in the same backing chain. So we need an "if" here.
Will adjust this patch and add it in respin.
Thanks,
Fam
>
>
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -490,15 +490,8 @@ immediate_exit:
> if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
> bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
> }
> - bdrv_swap(s->target, s->common.bs);
> - if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
> - /* drop the bs loop chain formed by the swap: break the loop then
> - * trigger the unref from the top one */
> - BlockDriverState *p = s->base->backing_hd;
> - s->base->backing_hd = NULL;
> - bdrv_op_unblock_all(p, s->base->backing_blocker);
> - bdrv_unref(p);
> - }
> + bdrv_op_unblock_all(s->common.bs->backing_hd,
> s->common.bs->backing_blocker);
> + bdrv_drop_intermediate(s->common.bs, s->common.bs, s->target);
> }
> bdrv_unref(s->target);
> block_job_completed(&s->common, ret);
