Re: block-dirty-bitmap-add fails with "Operation not supported" in 4.2 rc5 (worked in 4.1)

[Eric Blake]

On 12/12/19 3:04 PM, John Snow wrote:
> On 12/12/19 1:32 PM, Nir Soffer wrote:
> > On Thu, Dec 12, 2019 at 2:04 PM Nir Soffer <address@hidden> wrote:
> > > We have a test for full backup flow testing that we can consume the
> > > data using our nbd client.
> > >
> > > The test[0] is starting a full backup flow, based on Eric examples
> > > from [1] and [2].

Note that  my examples in [1] and [2] were written without -blockdev, 
and included significant contortions to determine the proper node names 
rather than relying on block names.  Meanwhile, the current code now 
checked in to upstream libvirt with Peter's edits to my code relies on 
-blockdev, and does not suffer from the problems caused by drive names.

> > Looking at qemu-iotests/256, I switch the order of the actions in the
> > transaction
> > from:
> >
> > actions = [
> >      {"type": 'blockdev-backup", "data": ...},
> >      { "type": "block-dirty-bitmap-add", "data": ...},
> > ]
> >
> > To:
> >
> >   actions = [
> >      { "type": "block-dirty-bitmap-add", "data": ...},
> >      {"type": 'blockdev-backup", "data": ...},
> > ]
> >
> > And now the the tests pass with 4.2 rc5.
> >
> > I'm not sure why it was ok to add the bitmap after starting the block
> > job before and now it is not, but it makes sense to me.
>
> This ... might have something to do with filter nodes, I bet.

Makes sense as the explanation to me as well.

> ide0-hd0 is the name of a device[*], not the name of a block graph node.
> When you use such names for "node" or "node-name" parameters, they
> (often) resolve to the root of the graph attached to the device.[**]
>
> The problem is that the root node of the graph can change, especially
> during the runtime of a block job, where filters might be added above
> the existing graph.
>
> Before blockdev-backup prepares itself, "ide0-hd0" has a qcow2 node at
> the root of its tree. This node can be used to store persistent bitmaps.
>
> After blockdev-backup prepares, "ide0-hd0" now has a filter node at its
> root -- it no longer implicitly refers to the qcow2 node.

And that filter node is new behavior to qemu 4.2, which my original 
tests did not exercise.

> And that node, you may be surprised to learn, does not support adding
> persistent dirty bitmaps. Oops.
>
> What I recommend is using blockdev configuration top-to-bottom:

And that's what Peter's libvirt patches do.

> - Either on the CLI by using -blockdev, or
> - At runtime using QMP, see the "create_target" function in iotest 256
> for how I create a two-node qcow2 storage graph.
>
> Using blockdev, you can give the nodes meaningful IDs that never change
> out from under you. Then, I believe that your transaction should work in
> either order.

You'll notice even in my libvirt emails that I used $node rather than 
$device in my steps, by using query-block to scrape out the generated 
node name rather than the device name.  Looking at your trace...

> 13:55:11 DEBUG   (MainThread) [qmp] Received return: {}
> 13:55:11 DEBUG   (MainThread) [qmp] Sending {'execute': 'query-block'}
> 13:55:11 DEBUG   (MainThread) [qmp] Received return: [{'io-status':
> 'ok', 'device': 'ide0-hd0', 'locked': False, 'removable': False,
> 'inserted': {'iops_rd': 0, 'detect_zeroes': 'off', 'image':
> {'virtual-size': 5368709120, 'filename':
> '/var/tmp/imageio-storage/file-512-ext4-mount/file', 'cluster-size':
> 65536, 'format': 'qcow2', 'actual-size': 860160, 'format-specific':
> {'type': 'qcow2', 'data': {'compat': '1.1', 'lazy-refcounts': False,
> 'refcount-bits': 16, 'corrupt': False}}, 'dirty-flag': False},
> 'iops_wr': 0, 'ro': False, 'node-name': '#block126',

you had an assigned node name #block126...

> 13:55:11 DEBUG   (MainThread) [qmp] Sending {'execute': 'transaction',
> 'arguments': {'actions': [{'type': 'blockdev-backup', 'data':
> {'device': 'ide0-hd0', 'job-id': 'backup-sda', 'sync': 'none',
> 'target': 'backup-sda'}}, {'type': 'block-dirty-bitmap-add', 'data':
> {'name': 'check1', 'node': 'ide0-hd0', 'persistent': True}}]}}

so try using #block126 instead of 'ide0-hd0' if the transaction remains 
in this order.  But yes, better yet is to match what libvirt will do and 
use -blockdev everywhere (upstream libvirt will refuse to use bitmaps 
with any qemu older than 4.2).

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