[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter drive
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver |
Date: |
Fri, 14 Jun 2019 14:57:38 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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 <address@hidden>
>>> ---
>>> 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
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Max Reitz, 2019/06/13
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Vladimir Sementsov-Ogievskiy, 2019/06/14
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver,
Max Reitz <=
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Vladimir Sementsov-Ogievskiy, 2019/06/14
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Max Reitz, 2019/06/14
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Vladimir Sementsov-Ogievskiy, 2019/06/17
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Max Reitz, 2019/06/17
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Kevin Wolf, 2019/06/17
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Max Reitz, 2019/06/17
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Kevin Wolf, 2019/06/17
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Vladimir Sementsov-Ogievskiy, 2019/06/18
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Kevin Wolf, 2019/06/18
- Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver, Vladimir Sementsov-Ogievskiy, 2019/06/18