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

[Vladimir Sementsov-Ogievskiy]

10.01.2019 17:38, Eric Blake wrote:
> 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.
> 

Ok.
with or without dropped unrelated error paths:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


-- 
Best regards,
Vladimir