Skip to contentSkip to conversation

Active commit regression fix

PATCH Superseded Jeff Cody 3 participants 6 messages Jan 30, 2016 – Feb 2, 2016

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jeff Cody

author
#0 lore raw

cover Active commit regression fix

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: change parent backing link when *tqe_prev == NULL
block: qemu-iotests - add test for snapshot, commit, snapshot bug

 block.c                    |   2 +-
 tests/qemu-iotests/143     | 114 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/143.out |  24 ++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/143
 create mode 100644 tests/qemu-iotests/143.out
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH 1/2 block: change parent backing link when *tqe_prev == NULL

In change_parent_backing_link(), we only inserted the new BlockDriverState entry into the device_list if the tqe_prev pointer was
NULL. However, we must also allow insertion when the BDS pointed
to by the tqe_prev pointer is NULL as well.

This fixes a bug with external snapshots, and live active layer commits.

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.

Once the active commit is finished, bdrv_replace_in_backing_chain() is called to set the base node as the new active node, and remove the node that was the prior active layer from the device_list.

If we only check against the tqe_prev pointer field and not the entity it is pointing to, then we fail to insert base image into the device list. The previous active layer is still removed from the device_list, leaving an empty device_list queue.

With an empty device_list queue, odd behavior occurs - such as not allowing any more live snapshots.

This commit fixes this issue, by checking for a NULL tqe_prev entity in the devices_list.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +1 -1
index 5709d3d..0b8526b 100644
@@ -2272,7 +2272,7 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
+        if (!to->device_list.tqe_prev || !*to->device_list.tqe_prev) {
             QTAILQ_INSERT_BEFORE(from, to, device_list);
         }
         QTAILQ_REMOVE(&bdrv_states, from, device_list);
Signature · 1 line
1.9.3
#2 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL

Quoted 36 lines · Jeff Cody

In change_parent_backing_link(), we only inserted the new BlockDriverState entry into the device_list if the tqe_prev pointer was
NULL. However, we must also allow insertion when the BDS pointed
to by the tqe_prev pointer is NULL as well.

This fixes a bug with external snapshots, and live active layer commits.

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.

Once the active commit is finished, bdrv_replace_in_backing_chain() is called to set the base node as the new active node, and remove the node that was the prior active layer from the device_list.

If we only check against the tqe_prev pointer field and not the entity it is pointing to, then we fail to insert base image into the device list. The previous active layer is still removed from the device_list, leaving an empty device_list queue.

With an empty device_list queue, odd behavior occurs - such as not allowing any more live snapshots.

This commit fixes this issue, by checking for a NULL tqe_prev entity in the devices_list.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block.c +0 -0
index 5709d3d..0b8526b 100644

block.c static void change_parent_backing_link(BlockDriverState *from, · line 2275

Quoted 5 lines
block.c +1 -1
@@ -2272,7 +2272,7 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
+        if (!to->device_list.tqe_prev || !*to->device_list.tqe_prev) {

I'm not sure this is the right fix; bdrv_make_anon() clearly states that we do want device_list.tqe_prev to be NULL if and only if the BDS is not part of the device list. So this should not be happening.

block.c static void change_parent_backing_link(BlockDriverState *from, · line 2278

Quoted 3 lines
block.c +0 -0
             QTAILQ_INSERT_BEFORE(from, to, device_list);
         }
         QTAILQ_REMOVE(&bdrv_states, from, device_list);

Inserting a from->device_list.tqe_prev = NULL; here makes the iotest happy and looks like the better fix to me.

Max

Quoted 1 line
author
#3 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL

Quoted 32 lines · Max Reitz
Quoted 31 lines · Jeff Cody

In change_parent_backing_link(), we only inserted the new BlockDriverState entry into the device_list if the tqe_prev pointer was
NULL. However, we must also allow insertion when the BDS pointed
to by the tqe_prev pointer is NULL as well.

This fixes a bug with external snapshots, and live active layer commits.

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.

Once the active commit is finished, bdrv_replace_in_backing_chain() is called to set the base node as the new active node, and remove the node that was the prior active layer from the device_list.

If we only check against the tqe_prev pointer field and not the entity it is pointing to, then we fail to insert base image into the device list. The previous active layer is still removed from the device_list, leaving an empty device_list queue.

With an empty device_list queue, odd behavior occurs - such as not allowing any more live snapshots.

This commit fixes this issue, by checking for a NULL tqe_prev entity in the devices_list.

---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

block.c static void change_parent_backing_link(BlockDriverState *from, · line 2275

Quoted 10 lines
block.c +1 -1
index 5709d3d..0b8526b 100644
@@ -2272,7 +2272,7 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
+        if (!to->device_list.tqe_prev || !*to->device_list.tqe_prev) {

I'm not sure this is the right fix; bdrv_make_anon() clearly states that we do want device_list.tqe_prev to be NULL if and only if the BDS is not part of the device list. So this should not be happening.

Good point. This also screams for a helper function to remove a BDS from the device_list, to enforce this behavior (we remove a BDS from the device_list is 3 spots, and this time it was missed in one of them). Hopefully that will help prevent future errors.

Quoted 9 lines

block.c static void change_parent_backing_link(BlockDriverState *from, · line 2278

Quoted 3 lines
block.c +0 -0
             QTAILQ_INSERT_BEFORE(from, to, device_list);
         }
         QTAILQ_REMOVE(&bdrv_states, from, device_list);

Inserting a from->device_list.tqe_prev = NULL; here makes the iotest happy and looks like the better fix to me.

Max

Thanks!

-Jeff

author
#4 lore raw

PATCH 2/2 block: qemu-iotests - add test for snapshot, commit, snapshot bug

In reply to Jeff Cody
---
 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
tests/qemu-iotests/143 new file +114 -0
new file mode 100755
index 0000000..6f3e0bb
@@ -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
[email protected]
+
+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
tests/qemu-iotests/143.out new file +24 -0
new file mode 100644
index 0000000..05cc9f4
@@ -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
tests/qemu-iotests/group +1 -0
index d6e9219..9203a4d 100644
@@ -142,3 +142,4 @@
 138 rw auto quick
 139 rw auto quick
 142 auto
+143 rw auto quick
Signature · 1 line
1.9.3
Reviewed-by on the v1 series
#5 lore raw
In reply to Jeff Cody
Quoted 10 lines · Jeff Cody

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: change parent backing link when *tqe_prev == NULL
block: qemu-iotests - add test for snapshot, commit, snapshot bug

Series:

Quoted 9 lines
 block.c                    |   2 +-
 tests/qemu-iotests/143     | 114 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/143.out |  24 ++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/143
 create mode 100644 tests/qemu-iotests/143.out
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org