On 1/10/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote: > 10.01.2019 10:13, Eric Blake wrote: >> Our initial implementation of x-nbd-server-add-bitmap put >> in a restriction because of incremental backups: in that >> usage, we are exporting one qcow2 file (the temporary overlay >> target of a blockdev-backup sync:none job) and a dirty bitmap >> owned by a second qcow2 file (the source of the >> blockdev-backup, which is the backing file of the temporary). >> While both qcow2 files are still writable (the target capture >> copy-on-write of old contents, the source to track live guest >> writes in the meantime), the NBD client expects to see >> constant data, including the dirty bitmap. An enabled bitmap >> in the source would be modified by guest writes, which is at >> odds with the NBD export being a read-only constant view, >> hence the initial code choice of enforcing a disabled bitmap >> (the intent is that the exposed bitmap was disabled in the >> same transaction that started the blockdev-backup job, >> although we don't want to actually track enough state to >> actually enforce that). >> >> However, in the case of image migration, where we WANT a >> writable export, it makes total sense that the NBD client >> could expect writes to change the status visible through >> the exposed dirty bitmap, > > Don't follow.. Which kind of migration do you mean?
When libvirt does block migration, it opens up an NBD server on the
destination, does a block-mirror from the source to copy the contents to
the destination, then when the two are in sync does the actual VM state
migration. There may be a use case where the destination already has
some of the state (say that we started a migration, then got
interrupted, and now want to restart but not lose the progress of what
has already been sync'd previously) - in which case, the destination
would expose a dirty bitmap of what has been already transferred, and
the source can use that information to avoid re-sending data that has
not changed since the previous partial transfer. There may be other
uses for exposing a live dirty bitmap for a writeable NBD export; and
it's more a question of not forbidding this case even if I don't have a
strong use case for why it will be useful.
>
> and thus permitting an enabled
>> bitmap for an export that is not read-only makes sense;
>> although the current restriction prevents that usage.
>>
>> Alternatively, consider the case when the active layer does
>> not contain the bitmap but the backing layer does. If the
>> backing layer is read-only (which is different from the
>> backing layer of a blockdev-backup job being writable),
>> we know the backing layer cannot be modified,
>
> hmm, sounds a bit strange "in case of read-only backing, we know
> that it cannot be modified". It's true for any read-only node,
> so you can say just something like "read-only node (for example
> regular backing file)"
Nicer wording indeed. And, since my first paragraph was harder to
justify, I'll swap the two in order to emphasize the case I actually hit
over the one that is just hand-wavy "we might find a use for it".
>
> and thus the
>> bitmap will not change contents even if it is still marked
>> enabled (for that matter, since the backing file is
>> read-only, we couldn't change the bitmap to be disabled
>> in the first place). Again, the current restriction
>> prevents this usage.
>>
>> Solve both issues by gating the restriction against a
>> disabled bitmap to only happen when the caller has
>> requested a read-only export, and where the BDS that owns
>> the bitmap (which might be a backing file of the BDS
>> handed to nbd_export_new) is still writable.
>>
>> Update iotest 223 to add some tests of the error paths.
>>
>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>
>> _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
>> "arguments":{"device":"n"}}' "return"
>> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
>> + "arguments":{"device":"n"}}' "error"
>
> twice add?
>
>
> aha, I've just realized that it's "some tests of the error paths" not related
> to bitmaps..
>
> So, I'd prefer to keep them in separate patch
Could do. I can split it if I have to respin.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
