Fix commit of oversized layer

This fixes a regression in block-commit; if the top image is larger than the base image, we attempt to resize the base image. The regression is that we fail the image truncate operation, returning -EBUSY.

Jeff Cody (2):
block: check for RESIZE blocker in the QMP command, not
bdrv_truncate()
block: add qemu-iotest for resize base during live commit

 block.c                    |  4 +--
 blockdev.c                 |  5 +++
 tests/qemu-iotests/095     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/095.out | 31 +++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 124 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/095
 create mode 100644 tests/qemu-iotests/095.out

1.9.3

If we check for the RESIZE blocker in bdrv_truncate(), that means a commit will fail if the overlay layer is larger than the base, due to the backing blocker.

This is a regression in behavior from 2.0; currently, commit will try to grow the size of the base image to match the overlay size, if the overlay size is larger.

By moving this into the QMP command qmp_block_resize(), it allows usage of bdrv_truncate() within block jobs.

---
 block.c    | 4 +---
 blockdev.c | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

index 6d7901e..6f1380e 100644
@@ -3498,9 +3498,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
         return -ENOTSUP;
     if (bs->read_only)
         return -EACCES;
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
-        return -EBUSY;
-    }
+
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);

index 80bae9d..dc535bf 100644
@@ -1819,6 +1819,11 @@ void qmp_block_resize(bool has_device, const char *device,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        return;
+    }
+
     /* complete all in-flight operations before resizing the device */
     bdrv_drain_all();
 

1.9.3

If 'base' is smaller than the overlay image being committed into it, then the base image will be grown in commit_run via bdrv_truncate().

This tests to make sure that this works, and the bdrv_truncate() is not blocked when it shouldn't be.

---
 tests/qemu-iotests/095     | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/095.out | 31 +++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 118 insertions(+)
 create mode 100755 tests/qemu-iotests/095
 create mode 100644 tests/qemu-iotests/095.out

new file mode 100755
index 0000000..acc7dbf
@@ -0,0 +1,86 @@
+#!/bin/bash
+#
+# Test for commit of larger active layer
+#
+# This tests live snapshots of images on a running QEMU instance, using
+# QMP commands.  Both single disk snapshots, and transactional group
+# snapshots are performed.
+#
+# Copyright (C) 2014 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!
+
+_cleanup()
+{
+    _cleanup_qemu
+    rm  -f "${TEST_IMG}.base" "${TEST_IMG}.snp1"
+	_cleanup_test_img
+}
+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_smaller=5M
+size_larger=100M
+
+_make_test_img $size_smaller
+mv "${TEST_IMG}" "${TEST_IMG}.base"
+
+_make_test_img -b "${TEST_IMG}.base" $size_larger
+mv "${TEST_IMG}" "${TEST_IMG}.snp1"
+
+_make_test_img -b "${TEST_IMG}.snp1" $size_larger
+
+echo
+echo "=== Base image info before commit and resize ==="
+$QEMU_IMG info "${TEST_IMG}.base" | _filter_testdir
+
+echo
+echo === Running QEMU Live Commit Test ===
+echo
+
+qemu_comm_method="qmp"
+_launch_qemu -drive file="${TEST_IMG}",if=virtio,id=test
+h=$QEMU_HANDLE
+
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
+
+_send_qemu_cmd $h "{ 'execute': 'block-commit',
+                                 'arguments': { 'device': 'test',
+                                 'top': '"${TEST_IMG}.snp1"' } }" "BLOCK_JOB_COMPLETED"
+
+echo
+echo "=== Base image info after commit and resize ==="
+$QEMU_IMG info "${TEST_IMG}.base" | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0

new file mode 100644
index 0000000..5864dda
@@ -0,0 +1,31 @@
+QA output created by 095
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=5242880 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 backing_file='TEST_DIR/t.IMGFMT.base' 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 backing_file='TEST_DIR/t.IMGFMT.snp1' 
+
+=== Base image info before commit and resize ===
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 5.0M (5242880 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+
+=== Running QEMU Live Commit Test ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 104857600, "offset": 104857600, "speed": 0, "type": "commit"}}
+
+=== Base image info after commit and resize ===
+image: TEST_DIR/t.qcow2.base
+file format: qcow2
+virtual size: 100M (104857600 bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+*** done

index 0f07440..e3dc4e8 100644
@@ -99,3 +99,4 @@
 090 rw auto quick
 091 rw auto
 092 rw auto quick
+095 rw auto

1.9.3

Series: Reviewed-by: Eric Blake <[email protected]>

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Am 25.06.2014 um 22:55 hat Jeff Cody geschrieben:

Thanks, applied to the block branch.

One thing I'm not sure about is whether commit (all of synchronous, live and live on active layer) should check the RESIZE blocker before resizing the backing file.

In general, it feels like it would be the right thing to do, especially considering the goal of operation categories in the final state, but on the other hand it means that RESIZE would have to be excluded from bs->backing_blocker, too, allowing standalone resize commands on backing files. Not sure that this would be a good idea...

Kevin

Is it really dangerous if we relax the backing_blocker on resize? In general, I expect the only critical category of operation is chain manipulation, particularly bdrv_swap.

And speaking of bdrv_swap, in longer term, if we have the BlockBackend that can serve as a level of abstraction between BlockDriverState (the backend implementation) and its users (the backend consumers, like device), we can probably drop bdrv_swap() by updating the BB->BDS link.

To illustrate:

    [top] <----------- device emulation
      |        |------ NBD server
      |        `------ commit block job
    [mid]
      |
    [bot]

When we commit top to mid, bdrv_swap will move the data into original top's position, so the old users of top now automatically start to use mid (of course, it is assigned some properties of top, like device name).

    [mid] <----------- device emulation
      |        |------ NBD server
      |        `------ commit block job (done)
      |
    [bot]

Where the original [mid]'s memory is freed (bdrv_unref, precisely).

With the imaginary BlockBackend, we start over again:

    [top] <----------- <backend0> <------- device emulation
      |                    |          `--- NBD server
      |                    |
      |                commit block job
    [mid]
      |
    [bot]

Now the block job, device and NBD server all uses BlockBackend <backend0>, who represents an endpoint for the blockdev operations. Normally they don't care who is behind backend0, except that block job will look at the backing BDSes, because in order to commit through the chain, it has to know the topology behind it.

When the job is done, block job can safely update the backend's link to point to [bot], while all the other backend user's pointers remain valid because we don't move backend0 around. Everyone is happy.

    [top]      /------ <backend0> <------- device emulation
      |        |           |          `--- NBD server
      |        |           |
      |        |       commit block job (done)
    [mid]      |
      |        |
    [bot] <----/

Is this a good direction?

Fam

Am 10.07.2014 um 10:42 hat Fam Zheng geschrieben:

I'm not completely sure about backing_blockers on resize, but I don't think there's currently a way to make it safe, except if the image could be safely removed from the backing chain altogether.

In any case, as long as block jobs don't set blockers on all images they touch but rather just on the top-level one, allowing to resize arbitrary nodes is dangerous because the block jobs can't cope with BDSes changing their size in the middle of the operation.

Yes, eventually we'll want to get rid of bdrv_swap() and just update pointers instead. We're not quite there yet, though.

Kevin

What's the worst case here? Something like block job never complete? User might get to some error state in this case but that's expected.

Because guest can't cope with devices changing their size in the middle of some of its operations, as well.

Fam