[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v2 1/6] nbd: Only require disabled bitmap for read-only exports
Date: Thu, 10 Jan 2019 08:38:33 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]