[Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Jeff Cody]

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

[Qemu-devel] [PATCH for 2.1 1/2] block: check for RESIZE blocker in the QMP command, not bdrv_truncate()

[Jeff Cody]

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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c    | 4 +---
 blockdev.c | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 6d7901e..6f1380e 100644
--- a/block.c
+++ b/block.c
@@ -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);
diff --git a/blockdev.c b/blockdev.c
index 80bae9d..dc535bf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -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

[Qemu-devel] [PATCH for 2.1 2/2] block: add qemu-iotest for resize base during live commit

[Jeff Cody]

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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 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

diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
new file mode 100755
index 0000000..acc7dbf
--- /dev/null
+++ b/tests/qemu-iotests/095
@@ -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
+owner=jcody@redhat.com
+
+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
diff --git a/tests/qemu-iotests/095.out b/tests/qemu-iotests/095.out
new file mode 100644
index 0000000..5864dda
--- /dev/null
+++ b/tests/qemu-iotests/095.out
@@ -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
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0f07440..e3dc4e8 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -99,3 +99,4 @@
 090 rw auto quick
 091 rw auto
 092 rw auto quick
+095 rw auto
-- 
1.9.3

Re: [Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 06/25/2014 02:55 PM, Jeff Cody wrote:
> 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

Series: Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  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
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Kevin Wolf]

Am 25.06.2014 um 22:55 hat Jeff Cody geschrieben:
> 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.

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

Re: [Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Fam Zheng]

On Fri, 06/27 11:44, Kevin Wolf wrote:
> 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...

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

Re: [Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Kevin Wolf]

Am 10.07.2014 um 10:42 hat Fam Zheng geschrieben:
> On Fri, 06/27 11:44, Kevin Wolf wrote:
> > 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...
> 
> 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.

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.

> 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.
> [...]
> Is this a good direction?

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

Kevin

Re: [Qemu-devel] [PATCH for 2.1 0/2] Fix commit of oversized layer

[Fam Zheng]

On Thu, 07/10 11:25, Kevin Wolf wrote:
> Am 10.07.2014 um 10:42 hat Fam Zheng geschrieben:
> > On Fri, 06/27 11:44, Kevin Wolf wrote:
> > > 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...
> > 
> > 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.
> 
> 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.

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