On Fri, Dec 13, 2019 at 11:46 AM Kevin Wolf <[email protected]> wrote: > > Am 13.12.2019 um 00:12 hat Nir Soffer geschrieben: > > On Thu, Dec 12, 2019 at 11:34 PM Eric Blake <[email protected]> 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 <[email protected]> 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. Nir
