[Qemu-devel] [RFC PATCH 0/2] iotests: fleecing test

[Qemu-devel] [RFC PATCH 0/2] iotests: fleecing test

[John Snow]

A simple, hastily-written example of image fleecing over NBD.

John Snow (2):
  block: allow blockdev-backup from any source
  iotests: add 222 to test basic fleecing

 blockdev.c               |   2 +-
 tests/qemu-iotests/222   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group |   1 +
 3 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemu-iotests/222

-- 
2.14.4

[Qemu-devel] [RFC PATCH 1/2] block: allow blockdev-backup from any source

[John Snow]

In the case of image fleecing, the node we choose as the source
for a blockdev-backup is going to be both a root node AND the
backing node for the exported image. It does not qualify as a root
image in this case.

Loosen the restriction.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 58d7570932..526f8b60be 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3517,7 +3517,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
         backup->compress = false;
     }
 
-    bs = qmp_get_root_bs(backup->device, errp);
+    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
     if (!bs) {
         return NULL;
     }
-- 
2.14.4

Re: [Qemu-devel] [RFC PATCH 1/2] block: allow blockdev-backup from any source

[Eric Blake]

On 06/26/2018 05:22 PM, John Snow wrote:
> In the case of image fleecing, the node we choose as the source
> for a blockdev-backup is going to be both a root node AND the
> backing node for the exported image. It does not qualify as a root
> image in this case.
> 
> Loosen the restriction.

Did we regress (and if so, when), or has this never worked?  But you are 
right: visually, we are starting with:

                   Device
Base (backing) <- Top (active)

then want to add nodes for an NBD export:
                   Device    NBD
Base (backing) <- Top    <- Tmp

with a blockdev-backup "sync":"none" from Top to Tmp (any writes from 
the Device first copy the old data to Tmp; the NBD export sees a 
read-only view of Tmp that is unchanging from the time the backup job 
started, regardless of what the Device does in the meantime).

Then when the fleece job ends, the NBD export is stopped, the 
blockdev-backup job is canceled, and Tmp is thrown away as unneeded.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 58d7570932..526f8b60be 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3517,7 +3517,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>           backup->compress = false;
>       }
>   
> -    bs = qmp_get_root_bs(backup->device, errp);
> +    bs = bdrv_lookup_bs(backup->device, backup->device, errp);

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

>       if (!bs) {
>           return NULL;
>       }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [RFC PATCH 1/2] block: allow blockdev-backup from any source

[John Snow]

On 06/26/2018 09:38 PM, Eric Blake wrote:
> On 06/26/2018 05:22 PM, John Snow wrote:
>> In the case of image fleecing, the node we choose as the source
>> for a blockdev-backup is going to be both a root node AND the
>> backing node for the exported image. It does not qualify as a root
>> image in this case.
>>
>> Loosen the restriction.
> 
> Did we regress (and if so, when), or has this never worked?  But you are
> right: visually, we are starting with:

It's actually a regression, because I had a variant of this test in my
local files that used to work. We never really advertised or tested this
feature, though, so ...

> 
>                   Device
> Base (backing) <- Top (active)
> 
> then want to add nodes for an NBD export:
>                   Device    NBD
> Base (backing) <- Top    <- Tmp

          [device] --> [active]
                          ^
[nbd] --> [fleecing]------'


So active here has two parents: the device root, and the temporary
fleecing node. This disqualifies it from being the source in
blockdev_backup, but there's no real reason to.

If the caller of blockdev_backup chooses a dumb source node they'll get
dumb data. GIGO.

> 
> with a blockdev-backup "sync":"none" from Top to Tmp (any writes from
> the Device first copy the old data to Tmp; the NBD export sees a
> read-only view of Tmp that is unchanging from the time the backup job
> started, regardless of what the Device does in the meantime).
> 
> Then when the fleece job ends, the NBD export is stopped, the
> blockdev-backup job is canceled, and Tmp is thrown away as unneeded.
> 

Yup, highlighted a little better in the next patch.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 58d7570932..526f8b60be 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3517,7 +3517,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup
>> *backup, JobTxn *txn,
>>           backup->compress = false;
>>       }
>>   -    bs = qmp_get_root_bs(backup->device, errp);
>> +    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>       if (!bs) {
>>           return NULL;
>>       }
>>
> 

[Qemu-devel] [RFC PATCH 2/2] iotests: add 222 to test basic fleecing

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/222   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group |   1 +
 2 files changed, 122 insertions(+)
 create mode 100644 tests/qemu-iotests/222

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
new file mode 100644
index 0000000000..133d10c351
--- /dev/null
+++ b/tests/qemu-iotests/222
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+# This test covers the basic fleecing workflow.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+# John helped, too.
+#
+# 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: John Snow <jsnow@redhat.com>
+
+import iotests
+from iotests import log, qemu_img, qemu_io, qemu_io_silent
+
+iotests.verify_platform(['linux'])
+
+patterns = [("0x5d", "0", "64k"),
+            ("0xd5", "1M", "64k"),
+            ("0xdc", "32M", "64k"),
+            ("0xcd", "67043328", "64k")]  # 64M - 64K
+
+overwrite = [("0xab", "0",        "64k"), # Full overwrite
+             ("0xad", "1015808",  "64k"), # Partial-left (1M-32K)
+             ("0x1d", "33587200", "64k"), # Partial-right (32M+32K)
+             ("0xea", "64M", "64k")]      # Adjacent-right (64M)
+
+with iotests.FilePath('base.img') as base_img_path, \
+     iotests.FilePath('fleece.img') as fleece_img_path, \
+     iotests.FilePath('nbd.sock') as nbd_sock_path, \
+     iotests.VM() as vm:
+
+    log('--- Setting up images ---')
+    log('')
+
+    assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+    assert qemu_img('create', '-f', iotests.imgfmt, fleece_img_path, '64M') == 0
+
+    for p in patterns:
+        qemu_io('-c', 'write -P%s %s %s' % p, base_img_path)
+
+    log('Done')
+
+    log('')
+    log('--- Launching VM ---')
+    log('')
+
+    vm.add_drive(base_img_path)
+    vm.launch()
+
+    log('')
+    log('--- Setting up Fleecing Graph ---')
+    log('')
+
+    srcNode = "drive0"
+    tgtNode = "fleeceNode"
+
+    # create tgtNode backed by srcNode
+    log(vm.qmp("blockdev-add", **{
+        "driver": "qcow2",
+        "node-name": tgtNode,
+        "file": {
+            "driver": "file",
+            "filename": fleece_img_path,
+        },
+        "backing": srcNode,
+    }))
+
+    # Establish COW from source to fleecing node
+    log(vm.qmp("blockdev-backup",
+               device=srcNode,
+               target=tgtNode,
+               sync="none"))
+
+    log('')
+    log('--- Setting up NBD Export ---')
+    log('')
+
+    nbd_uri = 'nbd+unix:///%s?socket=%s' % (tgtNode, nbd_sock_path)
+    log(vm.qmp("nbd-server-start",
+               **{"addr": { "type": "unix",
+                            "data": { "path": nbd_sock_path } } }))
+
+    log(vm.qmp("nbd-server-add", device=tgtNode))
+
+    log('')
+    log('--- Sanity Check ---')
+    log('')
+
+    for p in patterns:
+        cmd = "read -P%s %s %s" % p
+        log(cmd)
+        assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
+
+    log('')
+    log('--- Testing COW ---')
+    log('')
+
+    for p in overwrite:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io(srcNode, cmd))
+
+    log('')
+    log('--- Verifying Data ---')
+    log('')
+
+    for p in patterns:
+        cmd = "read -P%s %s %s" % p
+        log(cmd)
+        assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index eea75819d2..8019a9f721 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -220,3 +220,4 @@
 218 rw auto quick
 219 rw auto
 221 rw auto quick
+222 rw auto quick
-- 
2.14.4

Re: [Qemu-devel] [RFC PATCH 2/2] iotests: add 222 to test basic fleecing

[Eric Blake]

On 06/26/2018 05:22 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/222   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group |   1 +
>   2 files changed, 122 insertions(+)
>   create mode 100644 tests/qemu-iotests/222
> 
> diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
> new file mode 100644
> index 0000000000..133d10c351
> --- /dev/null
> +++ b/tests/qemu-iotests/222
> @@ -0,0 +1,121 @@
> +#!/usr/bin/env python
> +#
> +# This test covers the basic fleecing workflow.
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +# John helped, too.

LOL.

> +
> +patterns = [("0x5d", "0", "64k"),
> +            ("0xd5", "1M", "64k"),
> +            ("0xdc", "32M", "64k"),
> +            ("0xcd", "67043328", "64k")]  # 64M - 64K
> +
> +overwrite = [("0xab", "0",        "64k"), # Full overwrite
> +             ("0xad", "1015808",  "64k"), # Partial-left (1M-32K)
> +             ("0x1d", "33587200", "64k"), # Partial-right (32M+32K)
> +             ("0xea", "64M", "64k")]      # Adjacent-right (64M)
> +
> +with iotests.FilePath('base.img') as base_img_path, \
> +     iotests.FilePath('fleece.img') as fleece_img_path, \
> +     iotests.FilePath('nbd.sock') as nbd_sock_path, \
> +     iotests.VM() as vm:

Does python require \ even after ','?

The test looks valid - you are definitely reading data over NBD from the 
point in time that you started the blockdev-backup job, even while the 
source image continues to be modified.

> +    for p in overwrite:
> +        cmd = "write -P%s %s %s" % p
> +        log(cmd)
> +        log(vm.hmp_qemu_io(srcNode, cmd))
> +
> +    log('')
> +    log('--- Verifying Data ---')
> +    log('')
> +
> +    for p in patterns:
> +        cmd = "read -P%s %s %s" % p
> +        log(cmd)
> +        assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri) == 0

Perhaps additional steps would be to then stop the NBD export, stop the 
block job, delete the tgtNode fleecing file, then stop qemu, and finally 
check that the overwritten patterns correctly show up in the source 
image (that is, also prove that we can tear down a job, and that the 
overwrites worked).  And we may want to enhance this test (or use it as 
a starting point to copy into a new test) to play with persistent dirty 
bitmaps thrown into the mix as well.  But what you have is already a 
great start to prevent regressions, so:

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [RFC PATCH 2/2] iotests: add 222 to test basic fleecing

[John Snow]

On 06/26/2018 09:49 PM, Eric Blake wrote:
> On 06/26/2018 05:22 PM, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/222   | 121

Probably would have helped to include 222.out, though.

>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group |   1 +
>>   2 files changed, 122 insertions(+)
>>   create mode 100644 tests/qemu-iotests/222
>>
>> diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
>> new file mode 100644
>> index 0000000000..133d10c351
>> --- /dev/null
>> +++ b/tests/qemu-iotests/222
>> @@ -0,0 +1,121 @@
>> +#!/usr/bin/env python
>> +#
>> +# This test covers the basic fleecing workflow.
>> +#
>> +# Copyright (C) 2018 Red Hat, Inc.
>> +# John helped, too.
> 
> LOL.
> 
>> +
>> +patterns = [("0x5d", "0", "64k"),
>> +            ("0xd5", "1M", "64k"),
>> +            ("0xdc", "32M", "64k"),
>> +            ("0xcd", "67043328", "64k")]  # 64M - 64K
>> +
>> +overwrite = [("0xab", "0",        "64k"), # Full overwrite
>> +             ("0xad", "1015808",  "64k"), # Partial-left (1M-32K)
>> +             ("0x1d", "33587200", "64k"), # Partial-right (32M+32K)
>> +             ("0xea", "64M", "64k")]      # Adjacent-right (64M)
>> +
>> +with iotests.FilePath('base.img') as base_img_path, \
>> +     iotests.FilePath('fleece.img') as fleece_img_path, \
>> +     iotests.FilePath('nbd.sock') as nbd_sock_path, \
>> +     iotests.VM() as vm:
> 
> Does python require \ even after ','?
> 

Dunno. Cargo cult from 216.

> The test looks valid - you are definitely reading data over NBD from the
> point in time that you started the blockdev-backup job, even while the
> source image continues to be modified.
> 
>> +    for p in overwrite:
>> +        cmd = "write -P%s %s %s" % p
>> +        log(cmd)
>> +        log(vm.hmp_qemu_io(srcNode, cmd))
>> +
>> +    log('')
>> +    log('--- Verifying Data ---')
>> +    log('')
>> +
>> +    for p in patterns:
>> +        cmd = "read -P%s %s %s" % p
>> +        log(cmd)
>> +        assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri)
>> == 0
> 
> Perhaps additional steps would be to then stop the NBD export, stop the
> block job, delete the tgtNode fleecing file, then stop qemu, and finally
> check that the overwritten patterns correctly show up in the source
> image (that is, also prove that we can tear down a job, and that the
> overwrites worked).  And we may want to enhance this test (or use it as
> a starting point to copy into a new test) to play with persistent dirty
> bitmaps thrown into the mix as well.  But what you have is already a
> great start to prevent regressions, so:
> 

Good suggestions. I'm working toward throwing bitmaps in now, but
actually cleaning up the VM properly and stopping the NBD server and
testing some of the latter-half paths would be nice. This was just a bit
of an RFC to get the bits out there sooner rather than later.

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

Too many changes I need to pepper in for v2, but thanks for the vote of
confidence :)

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 2/2] iotests: add 222 to test basic fleecing

[Kashyap Chamarthy]

On Wed, Jun 27, 2018 at 01:51:59AM -0400, John Snow wrote:

[...]

FWIW, I would also add a little blurb in the commit message about what
"fleecing" exactly is.  Something along the lines of: 

    "It provides an NBD export that serves you a (read-only?)
    point-in-time (PIT) snapshot of a disk."  

I'm sure you can come up with better phrasing.

> > The test looks valid - you are definitely reading data over NBD from the
> > point in time that you started the blockdev-backup job, even while the
> > source image continues to be modified.
> > 
> >> +    for p in overwrite:
> >> +        cmd = "write -P%s %s %s" % p
> >> +        log(cmd)
> >> +        log(vm.hmp_qemu_io(srcNode, cmd))
> >> +
> >> +    log('')
> >> +    log('--- Verifying Data ---')
> >> +    log('')
> >> +
> >> +    for p in patterns:
> >> +        cmd = "read -P%s %s %s" % p
> >> +        log(cmd)
> >> +        assert qemu_io_silent('-r', '-f', 'raw', '-c', cmd, nbd_uri)
> >> == 0
> > 
> > Perhaps additional steps would be to then stop the NBD export, stop the
> > block job, delete the tgtNode fleecing file, then stop qemu, and finally
> > check that the overwritten patterns correctly show up in the source
> > image (that is, also prove that we can tear down a job, and that the
> > overwrites worked).  And we may want to enhance this test (or use it as
> > a starting point to copy into a new test) to play with persistent dirty
> > bitmaps thrown into the mix as well.  But what you have is already a
> > great start to prevent regressions, so:
> > 
> 
> Good suggestions. I'm working toward throwing bitmaps in now, but
> actually cleaning up the VM properly and stopping the NBD server and
> testing some of the latter-half paths would be nice. This was just a bit
> of an RFC to get the bits out there sooner rather than later.

Yeah, they can be added later on.  For this test, you can just get away
by cleaning up the VM and the NBD export: log(vm.qmp("nbd-server-stop").

With that: 

    Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>

[...]

-- 
/kashyap