Re: [Qemu-devel] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports

[Eric Blake]

On 1/14/19 3:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2019 22:47, 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 in
>> order to capture copy-on-write of old contents, and the
>> source in order 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 track enough state to actually enforce that).
>>
>> However, consider the case of a bitmap contained in a read-only
>> node (including when the bitmap is found in a backing layer of
>> the active image).  Because the node can't be modified, the
>> bitmap won't change due to writes, regardless of whether it is
>> still enabled.  Forbidding the export unless the bitmap is
>> disabled is awkward, paritcularly since we can't change the
>> bitmap to be disabled (because the node is read-only).
>>
>> Alternatively, consider the case of live storage migration,
>> where management directs the destination to create a writable
>> NBD server, then performs a drive-mirror from the source to
>> the mirror, prior to doing the rest of the live migration.
> 
> s/mirror/target ?

Yes.

> 
>> Since storage migration can be time-consuming, it may be wise
>> to let the destination include a dirty bitmap to track which
>> portions it has already received, where even if the migration
>> is interrupted and restarted, the source can query the
>> destination block status in order to minimize re-sending
>> data that has not changed in the meantime on a second attempt.
> 
> hmm, dirty bitmap dirtiness don't guarantee that data was written,
> due to:
> 
> 1. philosofy: we generally don't care, when something is reported
>     as dirty, when actually it is clean, we assume that it is always
>     safe to consider things to be dirty.
> 
> 2. granularity: really, at least granularity of target bitmap should
>     correspond to mirror cluster size and target block size, if one of
>     these three things is out of sync, we definitely get [1]
> 
> 3. errors: bitmaps are set dirty for failed writes too, so [1]

Good points.

> 
> I'm not totally against, I just note that the feature you mean is
> at least not as simple to implement. And for me it looks like it is more
> correct to track progress of mirror on source, assuming all success
> nbd write requests as successful story. At least, as all vm metadata
> is on source, when storage migration not finished yet.

I'm fine adding a caveat that while allowing reads from a modifying
dirty bitmap may help, they may not be trivial to use correctly.

> 
>> Such code has not been written, but it makes sense that
>> letting an active dirty bitmap be exposed and changing
>> alongside writes may prove useful in the future.

to go along with my caveat that such code does not yet exist anyways :)

>>
>> 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
>> (whether or not it is the BDS handed to nbd_export_new() or
>> from its backing chain) is still writable.
>>
>> Update iotest 223 to show the looser behavior by leaving
>> a bitmap enabled the whole run; note that we have to tear
>> down and re-export a node when handling an error.
>>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> anyway, I'm OK with new if-condition, and for me something like
> 
>    No real reasons to forbid export of enabled bitmap, user should understand,
>    that block-status result may not correspond to reality if bitmap may be
>    changed by guest-write or something in parallel (including client's self 
> requests).
>    So, it should be OK to drop the restriction at all, but let's keep a 
> smaller
>    one to prevent dangerous wrong management error..
> 
> would be enough description so,
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Okay. Thanks for the review!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature