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 <ebl...@redhat.com> > > 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 <vsement...@virtuozzo.com> 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
