On Thu, 02/20 00:08, Jeff Cody wrote: > 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 <f...@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" }} >
Yes. I forgot to use bdrv_set_backing_hd in bdrv_append. Could you try if the below patch fixes it? Thanks. Fam --- diff --git a/block.c b/block.c index 1af43b9..66a8e35 100644 --- a/block.c +++ b/block.c @@ -1978,7 +1978,7 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) /* The contents of 'tmp' will become bs_top, as we are * swapping bs_new and bs_top contents. */ - bs_top->backing_hd = bs_new; + bdrv_set_backing_hd(bs_top, bs_new); bs_top->open_flags &= ~BDRV_O_NO_BACKING; pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file), bs_new->filename);