[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 3/8] nbd: Only require disabled bitmap for re
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports |
Date: |
Mon, 14 Jan 2019 09:58:17 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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
- [Qemu-block] [PATCH v3 0/8] Promote x-nbd-server-add-bitmap to stable, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 2/8] nbd: Forbid nbd-server-stop when server is not running, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 1/8] nbd: Add some error case testing to iotests 223, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 3/8] nbd: Only require disabled bitmap for read-only exports, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 4/8] nbd: Merge nbd_export_set_name into nbd_export_new, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 8/8] qemu-nbd: Add --bitmap=NAME option, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 7/8] nbd: Merge nbd_export_bitmap into nbd_export_new, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 5/8] nbd: Allow bitmap export during QMP nbd-server-add, Eric Blake, 2019/01/11
- [Qemu-block] [PATCH v3 6/8] nbd: Remove x-nbd-server-add-bitmap, Eric Blake, 2019/01/11
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/8] Promote x-nbd-server-add-bitmap to stable, Eric Blake, 2019/01/11