* [Qemu-devel] [PATCH v2 0/2] Active commit regression fix
@ 2016-02-02 1:33 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
` (2 more replies)
0 siblings, 3 replies; 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
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
--
1.9.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [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
* [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 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
* 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
end of thread, other threads:[~2016-02-02 17:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 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:03 ` Max Reitz
2016-02-02 17:08 ` [Qemu-devel] [PATCH v2 0/2] Active commit regression fix Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.