| Message ID | 1394436370-8908-7-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@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)); Do we want to unswap the blocker field inside bdrv_move_feature_fields() now? > 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); > } > } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 1d3f76f..1f4f78b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -369,6 +369,9 @@ struct BlockDriverState { > BlockJob *job; > > QDict *options; > + > + /* The error object in use for blocking operations on backing_hd */ > + Error *backing_blocker; > }; > > int get_tmp_filename(char *filename, int size); > -- > 1.9.0 > >
On Sun, 04/06 20:31, 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 <famz@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)); > > Do we want to unswap the blocker field inside bdrv_move_feature_fields() > now? Conceptually a BDS's blocker is a "feature field" (whatever it is) that shouldn't be swapped by bdrv_swap(), just as .device_name and .refcnt. So it is added to bdrv_move_feature_fields() since introduced, in patch 02. Are you seeing any issue with this? Fam > > > 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); > > } > > } > > diff --git a/include/block/block_int.h b/include/block/block_int.h > > index 1d3f76f..1f4f78b 100644 > > --- a/include/block/block_int.h > > +++ b/include/block/block_int.h > > @@ -369,6 +369,9 @@ struct BlockDriverState { > > BlockJob *job; > > > > QDict *options; > > + > > + /* The error object in use for blocking operations on backing_hd */ > > + Error *backing_blocker; > > }; > > > > int get_tmp_filename(char *filename, int size); > > -- > > 1.9.0 > > > > >
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); } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 1d3f76f..1f4f78b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -369,6 +369,9 @@ struct BlockDriverState { BlockJob *job; QDict *options; + + /* The error object in use for blocking operations on backing_hd */ + Error *backing_blocker; }; int get_tmp_filename(char *filename, int size);
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 <famz@redhat.com> --- block.c | 24 ++++++++++++++++++++---- block/mirror.c | 1 + include/block/block_int.h | 3 +++ 3 files changed, 24 insertions(+), 4 deletions(-)