Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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);
Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
On Thu, Feb 20, 2014 at 04:28:56PM +0800, Fam Zheng wrote:
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.
Yep, this seems to fix it.
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);
[Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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));
+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
Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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 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));
+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 ben...@irqsave.net
Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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.
+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
Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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.
Fam
Re: [Qemu-devel] [PATCH v14 06/14] block: Add backing_blocker in BlockDriverState
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 }}
