On 14.06.19 11:04, Vladimir Sementsov-Ogievskiy wrote: > 13.06.2019 18:57, Max Reitz wrote: >> On 29.05.19 17:46, Vladimir Sementsov-Ogievskiy wrote: >>> Backup-top filter does copy-before-write operation. It should be >>> inserted above active disk and has a target node for CBW, like the >>> following: >>> >>> +-------+ >>> | Guest | >>> +-------+ >>> |r,w >>> v >>> +------------+ target +---------------+ >>> | backup_top |---------->| target(qcow2) | >>> +------------+ CBW +---------------+ >>> | >>> backing |r,w >>> v >>> +-------------+ >>> | Active disk | >>> +-------------+ >>> >>> The driver will be used in backup instead of write-notifiers. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>> --- >>> block/backup-top.h | 64 +++++++++ >>> block/backup-top.c | 322 ++++++++++++++++++++++++++++++++++++++++++++ >>> block/Makefile.objs | 2 + >>> 3 files changed, 388 insertions(+) >>> create mode 100644 block/backup-top.h >>> create mode 100644 block/backup-top.c >>> >>> diff --git a/block/backup-top.h b/block/backup-top.h >>> new file mode 100644 >>> index 0000000000..788e18c358 >>> --- /dev/null >>> +++ b/block/backup-top.h
[...] >>> +/* >>> + * bdrv_backup_top_append >>> + * >>> + * Append backup_top filter node above @source node. @target node will >>> receive >>> + * the data backed up during CBE operations. New filter together with >>> @target >>> + * node are attached to @source aio context. >>> + * >>> + * The resulting filter node is implicit. >> >> Why? It’s just as easy for the caller to just make it implicit if it >> should be. (And usually the caller should decide that.) > > Personally, I don't know what are the reasons for filters to bi implicit or > not, > I just made it like other job-filters.. I can move making-implicit to the > caller > or drop it at all (if it will work). Nodes are implicit if they haven’t been added consciously by the user. A node added by a block job can be non-implicit, too, as mirror shows; If the user specifies the filter-node-name option, they will know about the node, thus it is no longer implicit. If the user doesn’t know about the node (they didn’t give the filter-node-name option), the node is implicit. [...] >>> +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs) >>> +{ >>> + if (!bs->backing) { >>> + return 0; >>> + } >>> + >>> + return bdrv_co_flush(bs->backing->bs); >> >> Should we flush the target, too? > > Hm, you've asked it already, on previous version :) I wasn’t sure... > Backup don't do it, > so I just keep old behavior. And what is the reason to flush backup target > on any guest flush? Hm, well, what’s the reason not to do it? Also, there are not only guest flushes. bdrv_flush_all() exists, which is called when the guest is stopped. So who is going to flush the target if not its parent? [...] >>> + >>> + if (role == &child_file) { >>> + /* >>> + * Target child >>> + * >>> + * Share write to target (child_file), to not interfere >>> + * with guest writes to its disk which may be in target backing >>> chain. >>> + */ >>> + if (perm & BLK_PERM_WRITE) { >>> + *nshared = *nshared | BLK_PERM_WRITE; >> >> Why not always share WRITE on the target? > > Hmm, it's a bad thing to share writes on target, so I'm trying to reduce > number of > cases when we have share it. Is it? First of all, this filter doesn’t care. It doesn’t even read from the target (related note: we probably don’t need CONSISTENT_READ on the target). Second, there is generally going to be a parent on backup-top that has the WRITE permission taken. So this does not really effectively reduce that number of cases. >>> + } >>> + } else { >>> + /* Source child */ >>> + if (perm & BLK_PERM_WRITE) { >> >> Or WRITE_UNCHANGED, no? > > Why? We don't need doing CBW for unchanged write. But we will do it still, won’t we? (If an unchanging write comes in, this driver will handle it just like a normal write, will it not?) [...] >>> + >>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, >>> + BlockDriverState *target, >>> + HBitmap *copy_bitmap, >>> + Error **errp) >>> +{ [...] >>> +} >> >> I guess it would be nice if it users could blockdev-add a backup-top >> node to basically get a backup with sync=none. >> >> (The bdrv_open implementation should then create a new bitmap and >> initialize it to be fully set.) >> >> But maybe it wouldn’t be so nice and I just have feverish dreams. > > When series begun, I was trying to make exactly this - user-available filter, > which may be used in separate, but you was against) Past me is stupid. > Maybe, not totally against, but I decided not to argue long, and instead make > filter implicit and drop public api (like mirror and commit filters), but > place it > in a separate file (no one will argue against putting large enough and > complete > new feature, represented by object into a separate file :). And this actually > makes it easy to publish this filter at some moment. And now I think it was > good decision anyway, as we postponed arguing around API and around shared > dirty > bitmaps. > > So publishing the filter is future step. OK, sure. >>> +void bdrv_backup_top_set_progress_callback( >>> + BlockDriverState *bs, BackupTopProgressCallback progress_cb, >>> + void *progress_opaque) >>> +{ >>> + BDRVBackupTopState *s = bs->opaque; >>> + >>> + s->progress_cb = progress_cb; >>> + s->progress_opaque = progress_opaque; >>> +} >>> + >>> +void bdrv_backup_top_drop(BlockDriverState *bs) >>> +{ >>> + BDRVBackupTopState *s = bs->opaque; >>> + AioContext *aio_context = bdrv_get_aio_context(bs); >>> + >>> + aio_context_acquire(aio_context); >>> + >>> + bdrv_drained_begin(bs); >>> + >>> + bdrv_child_try_set_perm(bs->backing, 0, BLK_PERM_ALL, &error_abort); >> >> Pre-existing in other jobs, but I think calling this function is >> dangerous. (Which is why I remove it in my “block: Ignore loosening >> perm restrictions failures” series.) > > Hmm, good thing.. Should I rebase on it? It would help me at least. >>> + bdrv_replace_node(bs, backing_bs(bs), &error_abort); >>> + bdrv_set_backing_hd(bs, NULL, &error_abort); >> >> I think some of this function should be in a .bdrv_close() >> implementation, for example this bdrv_set_backing_hd() call. > > Why? We don't have .bdrv_open, so why to have .bdrv_close? I think, when > we publish this filter most of _add() and _drop() will be refactored to > open() and close(). Is there a real reason to implement .close() now? Not really if it isn’t a usable block driver yet, no. Max
signature.asc
Description: OpenPGP digital signature