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

[Nir Soffer]

On Thu, Dec 12, 2019 at 11:34 PM Eric Blake <address@hidden> wrote:
>
> 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).

Thank you for the detailed answer!

I'm sure blockdev is the greatest thing since sliced bread but it
looks like overkill
for our use case. I'm happy to use the node name generated by qemu.

What I need is a simple way to start and stop backups and create
bitmaps so I can
test the code getting and merging base:allocation and qemu:dirty-bitmap extents.

We already support base:allocation and use it to access backup data:
https://github.com/oVirt/ovirt-imageio/blob/dbadeac5ca6db604af014a234a894503a3482a76/common/test/nbd_test.py#L513

I changed test rig to use node name instead of the device name:
https://gerrit.ovirt.org/c/105597

Nir