Re: [Qemu-block] [PATCH v2 1/6] nbd: Only require disabled bitmap for read-only exports

[Eric Blake]

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 <address@hidden>
>>

>>   _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