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

[Nir Soffer]

On Fri, Dec 13, 2019 at 10:39 PM Nir Soffer <address@hidden> wrote:
>
> On Fri, Dec 13, 2019 at 11:46 AM Kevin Wolf <address@hidden> wrote:
> >
> > Am 13.12.2019 um 00:12 hat Nir Soffer geschrieben:
> > > 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.
> >
> > If you care about more than the top layer, you need to control the block
> > graph. And -blockdev is the best way to be sure that the graph looks
> > what you want it to look, and to assign node names everywhere.
> >
> > > I'm happy to use the node name generated by qemu.
> >
> > You're not supposed to. That's why they contain a random element and are
> > unpredictable. If you do this against all warnings,
>
> Using "#block126" is a warning? Looks just like /dev/dm-42 or /dev/sdx to me.
>
> > don't blame us for
> > any pain you'll incur. The assumption in QEMU is that if you use node
> > names, you take responsibility to manage the graph.
>
> So qemu may behave differently if I added the disk using blockdev-add
> with my own
> node-name or if qemu assigned the node-name?
>
> > If you absolutely can't see yourself using -blockdev for now, you can
> > still set explicit node names with -drive at least.
>
> it sounds better than looking up the node name.

I update the patch setting node name in -drive, and using during backup:
https://github.com/nirs/ovirt-imageio/commit/f33d27eea09855e1a1202b79e9acd2f862b5df57