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 <ebl...@redhat.com> >>> > >>> _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 <vsement...@virtuozzo.com> -- Best regards, Vladimir