Active commit regression fix
v2 cover Active commit regression fix
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
Signature · 1 line
1.9.3
PATCH v2 1/2 block: set device_list.tqe_prev to NULL on BDS removal
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().
--- block.c | 24 ++++++++++++++---------- blockdev.c | 3 +-- include/block/block.h | 1 + 3 files changed, 16 insertions(+), 12 deletions(-)
block.c +14 -10
@@ -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); } }
blockdev.c +1 -2
@@ -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);
include/block/block.h +1 -0
@@ -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,
Signature · 1 line
1.9.3
PATCH v2 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
@@ -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
@@ -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
@@ -142,3 +142,4 @@ 138 rw auto quick 139 rw auto quick 142 auto +143 rw auto quick
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH v2 1/2] block: set device_list.tqe_prev to NULL on BDS removal
In reply to Jeff CodyQuoted 25 lines · Jeff Cody
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_prevfield in thedevice_list, although the base node'stqe_prevfield points to a NULL entry. This non-NULLtqe_prevfield occurs after thebdrv_append()in the external snapshot callschange_parent_backing_link().In
change_parent_backing_link(), when the previous active layer is removed fromdevice_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_statesdevice_listiff 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 thedevice_list, and
2.) uses that new API, which also fixes the regression once used in
change_parent_backing_link().
Quoted 6 lines
Signed-off-by Jeff Cody <[email protected]>--- block.c | 24 ++++++++++++++---------- blockdev.c | 3 +-- include/block/block.h | 1 + 3 files changed, 16 insertions(+), 12 deletions(-)
Re: [Qemu-devel] [PATCH v2 2/2] block: qemu-iotests - add test for snapshot, commit, snapshot bug
In reply to Jeff CodyQuoted 8 lines · Jeff Cody
Signed-off-by Jeff Cody <[email protected]>--- 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).
Max Reitz
AppliedQuoted 17 lines · Jeff Cody
Changes from v1:
* Rather than allow insertion when
bs->device_listtqe_prevpoints to a NULL entry, make sure than we follow the block scheme of enforcingbs->device_list->tqe_previs 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-NULLtqe_prevpointers 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
Quoted 9 lines
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