| Message ID | 1392817351-22148-7-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
The Wednesday 19 Feb 2014 à 21:42:23 (+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 | 19 +++++++++++++++---- > include/block/block_int.h | 3 +++ > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index dec44d4..95d8c1f 100644 > --- a/block.c > +++ b/block.c > @@ -1044,19 +1044,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); > bdrv_unref(bs->backing_hd); > + } 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; > } > 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_ref(bs->backing_hd); > + > + 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); > } > @@ -1696,8 +1710,7 @@ void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > if (bs->backing_hd) { > - bdrv_unref(bs->backing_hd); > - bs->backing_hd = NULL; > + bdrv_set_backing_hd(bs, NULL); > } > bs->drv->bdrv_close(bs); > g_free(bs->opaque); > @@ -1899,7 +1912,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)); > > @@ -1918,7 +1930,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/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.8.5.4 > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On Wed, Feb 19, 2014 at 09:42:23PM +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 | 19 +++++++++++++++---- > include/block/block_int.h | 3 +++ > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index dec44d4..95d8c1f 100644 > --- a/block.c > +++ b/block.c > @@ -1044,19 +1044,33 @@ fail: > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > { > if (bs->backing_hd) { > + assert(error_is_set(&bs->backing_blocker)); When I run block-commit, on either the active or non-active layer, I get an assertion here. The qemu-iotests do not catch it, and I presume it is because happens a couple of seconds or so after the success message is returned over QMP. > + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > bdrv_unref(bs->backing_hd); > + } 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; > } > 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_ref(bs->backing_hd); > + > + 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); > } > @@ -1696,8 +1710,7 @@ void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > if (bs->backing_hd) { > - bdrv_unref(bs->backing_hd); > - bs->backing_hd = NULL; > + bdrv_set_backing_hd(bs, NULL); > } > bs->drv->bdrv_close(bs); > g_free(bs->opaque); > @@ -1899,7 +1912,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)); > > @@ -1918,7 +1930,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/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.8.5.4 > >
On Wed, 02/19 16:17, Jeff Cody wrote: > On Wed, Feb 19, 2014 at 09:42:23PM +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 | 19 +++++++++++++++---- > > include/block/block_int.h | 3 +++ > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/block.c b/block.c > > index dec44d4..95d8c1f 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1044,19 +1044,33 @@ fail: > > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > > { > > if (bs->backing_hd) { > > + assert(error_is_set(&bs->backing_blocker)); > > When I run block-commit, on either the active or non-active layer, I > get an assertion here. The qemu-iotests do not catch it, and I > presume it is because happens a couple of seconds or so after the > success message is returned over QMP. > I can't reproduce this, could you give some specific steps? Thanks. Fam
On Thu, Feb 20, 2014 at 01:01:38PM +0800, Fam Zheng wrote: > On Wed, 02/19 16:17, Jeff Cody wrote: > > On Wed, Feb 19, 2014 at 09:42:23PM +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 | 19 +++++++++++++++---- > > > include/block/block_int.h | 3 +++ > > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index dec44d4..95d8c1f 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -1044,19 +1044,33 @@ fail: > > > void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > > > { > > > if (bs->backing_hd) { > > > + assert(error_is_set(&bs->backing_blocker)); > > > > When I run block-commit, on either the active or non-active layer, I > > get an assertion here. The qemu-iotests do not catch it, and I > > presume it is because happens a couple of seconds or so after the > > success message is returned over QMP. > > > > I can't reproduce this, could you give some specific steps? Thanks. > Sure - I am guessing the key is performing some live block snapshots first. Here is what I did (this is from memory, but I think the steps are right): Nothing special really about the cmdline: qemu-system-x86_64 -drive file=/home/jtc/test.qcow2,if=virtio -qmp stdio ... The QMP commands: For the non-active layer case: { "execute": "qmp_capabilities" } { "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0","snapshot-file":"/tmp/snap1.qcow2","format": "qcow2" } } { "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0","snapshot-file":"/tmp/snap2.qcow2","format": "qcow2" } } { "execute": "block-commit", "arguments": { "device": "virtio0", "top": "/tmp/snap1.qcow2" } } For the active layer case (I think I still had 2 snapshots here, not entirely positive): { "execute": "qmp_capabilities" } { "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0","snapshot-file":"/tmp/snap1.qcow2","format": "qcow2" } } { "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0","snapshot-file":"/tmp/snap2.qcow2","format": "qcow2" } } { "execute": "block-commit", "arguments": { "device": "virtio0", "top": "/tmp/snap2.qcow2" } } { "execute": "block-job-complete", "arguments": { "device": "virtio0" }}
diff --git a/block.c b/block.c index dec44d4..95d8c1f 100644 --- a/block.c +++ b/block.c @@ -1044,19 +1044,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); bdrv_unref(bs->backing_hd); + } 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; } 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_ref(bs->backing_hd); + + 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); } @@ -1696,8 +1710,7 @@ void bdrv_close(BlockDriverState *bs) if (bs->drv) { if (bs->backing_hd) { - bdrv_unref(bs->backing_hd); - bs->backing_hd = NULL; + bdrv_set_backing_hd(bs, NULL); } bs->drv->bdrv_close(bs); g_free(bs->opaque); @@ -1899,7 +1912,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)); @@ -1918,7 +1930,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/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 | 19 +++++++++++++++---- include/block/block_int.h | 3 +++ 2 files changed, 18 insertions(+), 4 deletions(-)