* [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal
2016-02-02 1:33 [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Jeff Cody
@ 2016-02-02 1:33 ` Jeff Cody
2016-02-02 17:02 ` Max Reitz
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug Jeff Cody
2016-02-02 17:08 ` [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Max Reitz
2 siblings, 1 reply; 6+ messages in thread
From: Jeff Cody @ 2016-02-02 1:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-stable, qemu-devel, stefanha, mreitz
This fixes a regression introduced with commit 3f09bfbc7. Multiple
bugs arise in conjunction with live snapshots and mirroring operations
(which include active layer commit).
After a live snapshot occurs, the active layer and the base layer both
have a non-NULL tqe_prev field in the device_list, although the base
node's tqe_prev field points to a NULL entry. This non-NULL tqe_prev
field occurs after the bdrv_append() in the external snapshot calls
change_parent_backing_link().
In change_parent_backing_link(), when the previous active layer is
removed from device_list, the device_list.tqe_prev pointer is not
set to NULL.
The operating scheme in the block layer is to indicate that a BDS belongs
in the bdrv_states device_list iff the device_list.tqe_prev pointer
is non-NULL.
This patch does two things:
1.) Introduces a new block layer helper bdrv_device_remove() to remove a
BDS from the device_list, and
2.) uses that new API, which also fixes the regression once used in
change_parent_backing_link().
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block.c | 24 ++++++++++++++----------
blockdev.c | 3 +--
include/block/block.h | 1 +
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 5709d3d..08cc130 100644
--- a/block.c
+++ b/block.c
@@ -2220,21 +2220,25 @@ void bdrv_close_all(void)
}
}
+/* Note that bs->device_list.tqe_prev is initially null,
+ * and gets set to non-null by QTAILQ_INSERT_TAIL(). Establish
+ * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
+ * resetting it to null on remove. */
+void bdrv_device_remove(BlockDriverState *bs)
+{
+ QTAILQ_REMOVE(&bdrv_states, bs, device_list);
+ bs->device_list.tqe_prev = NULL;
+}
+
/* make a BlockDriverState anonymous by removing from bdrv_state and
* graph_bdrv_state list.
Also, NULL terminate the device_name to prevent double remove */
void bdrv_make_anon(BlockDriverState *bs)
{
- /*
- * Take care to remove bs from bdrv_states only when it's actually
- * in it. Note that bs->device_list.tqe_prev is initially null,
- * and gets set to non-null by QTAILQ_INSERT_TAIL(). Establish
- * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
- * resetting it to null on remove.
- */
+ /* Take care to remove bs from bdrv_states only when it's actually
+ * in it. */
if (bs->device_list.tqe_prev) {
- QTAILQ_REMOVE(&bdrv_states, bs, device_list);
- bs->device_list.tqe_prev = NULL;
+ bdrv_device_remove(bs);
}
if (bs->node_name[0] != '\0') {
QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
@@ -2275,7 +2279,7 @@ static void change_parent_backing_link(BlockDriverState *from,
if (!to->device_list.tqe_prev) {
QTAILQ_INSERT_BEFORE(from, to, device_list);
}
- QTAILQ_REMOVE(&bdrv_states, from, device_list);
+ bdrv_device_remove(from);
}
}
diff --git a/blockdev.c b/blockdev.c
index 07cfe25..5f8e1b6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2382,8 +2382,7 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
/* This follows the convention established by bdrv_make_anon() */
if (bs->device_list.tqe_prev) {
- QTAILQ_REMOVE(&bdrv_states, bs, device_list);
- bs->device_list.tqe_prev = NULL;
+ bdrv_device_remove(bs);
}
blk_remove_bs(blk);
diff --git a/include/block/block.h b/include/block/block.h
index 25f36dc..8c53b93 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -198,6 +198,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
BlockDriverState *bdrv_new_root(void);
BlockDriverState *bdrv_new(void);
+void bdrv_device_remove(BlockDriverState *bs);
void bdrv_make_anon(BlockDriverState *bs);
void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
void bdrv_replace_in_backing_chain(BlockDriverState *old,
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal Jeff Cody
@ 2016-02-02 17:02 ` Max Reitz
0 siblings, 0 replies; 6+ messages in thread
From: Max Reitz @ 2016-02-02 17:02 UTC (permalink / raw)
To: Jeff Cody, qemu-block; +Cc: kwolf, qemu-devel, stefanha, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]
On 02.02.2016 02:33, Jeff Cody wrote:
> This fixes a regression introduced with commit 3f09bfbc7. Multiple
> bugs arise in conjunction with live snapshots and mirroring operations
> (which include active layer commit).
>
> After a live snapshot occurs, the active layer and the base layer both
> have a non-NULL tqe_prev field in the device_list, although the base
> node's tqe_prev field points to a NULL entry. This non-NULL tqe_prev
> field occurs after the bdrv_append() in the external snapshot calls
> change_parent_backing_link().
>
> In change_parent_backing_link(), when the previous active layer is
> removed from device_list, the device_list.tqe_prev pointer is not
> set to NULL.
>
> The operating scheme in the block layer is to indicate that a BDS belongs
> in the bdrv_states device_list iff the device_list.tqe_prev pointer
> is non-NULL.
>
> This patch does two things:
>
> 1.) Introduces a new block layer helper bdrv_device_remove() to remove a
> BDS from the device_list, and
> 2.) uses that new API, which also fixes the regression once used in
> change_parent_backing_link().
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> block.c | 24 ++++++++++++++----------
> blockdev.c | 3 +--
> include/block/block.h | 1 +
> 3 files changed, 16 insertions(+), 12 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug
2016-02-02 1:33 [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Jeff Cody
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal Jeff Cody
@ 2016-02-02 1:33 ` Jeff Cody
2016-02-02 17:03 ` Max Reitz
2016-02-02 17:08 ` [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Max Reitz
2 siblings, 1 reply; 6+ messages in thread
From: Jeff Cody @ 2016-02-02 1:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-stable, qemu-devel, stefanha, mreitz
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
tests/qemu-iotests/143 | 114 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/143.out | 24 ++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 139 insertions(+)
create mode 100755 tests/qemu-iotests/143
create mode 100644 tests/qemu-iotests/143.out
diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143
new file mode 100755
index 0000000..6f3e0bb
--- /dev/null
+++ b/tests/qemu-iotests/143
@@ -0,0 +1,114 @@
+#!/bin/bash
+# Check live snapshot, followed by active commit, and another snapshot.
+#
+# This test is to catch the error case of BZ #1300209:
+# https://bugzilla.redhat.com/show_bug.cgi?id=1300209
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jcody@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+TMP_SNAP1=${TEST_DIR}/tmp.qcow2
+TMP_SNAP2=${TEST_DIR}/tmp2.qcow2
+
+_cleanup()
+{
+ _cleanup_qemu
+ rm -f "${TEST_IMG}" "${TMP_SNAP1}" "${TMP_SNAP2}"
+}
+
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=512M
+
+_make_test_img $size
+
+echo
+echo === Launching QEMU ===
+echo
+
+qemu_comm_method="qmp"
+_launch_qemu -drive file="${TEST_IMG}",if=virtio
+h=$QEMU_HANDLE
+
+
+echo
+echo === Performing Live Snapshot 1 ===
+echo
+
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
+
+
+# First live snapshot, new overlay as active layer
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
+ 'arguments': {
+ 'device': 'virtio0',
+ 'snapshot-file':'${TMP_SNAP1}',
+ 'format': 'qcow2'
+ }
+ }" "return"
+
+echo
+echo === Performing block-commit on active layer ===
+echo
+
+# Block commit on active layer, push the new overlay into base
+_send_qemu_cmd $h "{ 'execute': 'block-commit',
+ 'arguments': {
+ 'device': 'virtio0'
+ }
+ }" "READY"
+
+_send_qemu_cmd $h "{ 'execute': 'block-job-complete',
+ 'arguments': {
+ 'device': 'virtio0'
+ }
+ }" "COMPLETED"
+
+echo
+echo === Performing Live Snapshot 2 ===
+echo
+
+# New live snapshot, new overlays as active layer
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
+ 'arguments': {
+ 'device': 'virtio0',
+ 'snapshot-file':'${TMP_SNAP2}',
+ 'format': 'qcow2'
+ }
+ }" "return"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out
new file mode 100644
index 0000000..05cc9f4
--- /dev/null
+++ b/tests/qemu-iotests/143.out
@@ -0,0 +1,24 @@
+QA output created by 143
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=536870912
+
+=== Launching QEMU ===
+
+
+=== Performing Live Snapshot 1 ===
+
+{"return": {}}
+Formatting 'TEST_DIR/tmp.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+
+=== Performing block-commit on active layer ===
+
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+
+=== Performing Live Snapshot 2 ===
+
+Formatting 'TEST_DIR/tmp2.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d6e9219..9203a4d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -142,3 +142,4 @@
138 rw auto quick
139 rw auto quick
142 auto
+143 rw auto quick
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug Jeff Cody
@ 2016-02-02 17:03 ` Max Reitz
0 siblings, 0 replies; 6+ messages in thread
From: Max Reitz @ 2016-02-02 17:03 UTC (permalink / raw)
To: Jeff Cody, qemu-block; +Cc: kwolf, qemu-devel, stefanha, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 530 bytes --]
On 02.02.2016 02:33, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> tests/qemu-iotests/143 | 114 +++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/143.out | 24 ++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 139 insertions(+)
> create mode 100755 tests/qemu-iotests/143
> create mode 100644 tests/qemu-iotests/143.out
143 is already in use, I'll move it to 144 (hoping that you're fine with
that).
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] Active commit regression fix
2016-02-02 1:33 [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Jeff Cody
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal Jeff Cody
2016-02-02 1:33 ` [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug Jeff Cody
@ 2016-02-02 17:08 ` Max Reitz
2 siblings, 0 replies; 6+ messages in thread
From: Max Reitz @ 2016-02-02 17:08 UTC (permalink / raw)
To: Jeff Cody, qemu-block; +Cc: kwolf, qemu-devel, stefanha, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
On 02.02.2016 02:33, Jeff Cody wrote:
> Changes from v1:
>
> * Rather than allow insertion when bs->device_listtqe_prev points to
> a NULL entry, make sure than we follow the block scheme of enforcing
> bs->device_list->tqe_prev is NULL upon deletion. (Thanks Max!)
>
> Bug #1300209 is a regression in 2.5, introduced during the
> change away from bdrv_swap().
>
> When we change the parent backing link (change_parent_backing_link),
> we must also accomodate non-NULL tqe_prev pointers that point to a
> NULL entry. Please see patch #1 for more details.
>
> Jeff Cody (2):
> block: set device_list.tqe_prev to NULL on BDS removal
> block: qemu-iotests - add test for snapshot, commit, snapshot bug
>
> block.c | 24 ++++++----
> blockdev.c | 3 +-
> include/block/block.h | 1 +
> tests/qemu-iotests/143 | 114 +++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/143.out | 24 ++++++++++
> tests/qemu-iotests/group | 1 +
> 6 files changed, 155 insertions(+), 12 deletions(-)
> create mode 100755 tests/qemu-iotests/143
> create mode 100644 tests/qemu-iotests/143.out
Thanks, applied to my block tree:
https://github.com/XanClic/qemu/commits/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread