Re: [Qemu-devel] [PATCH v6 25/42] mirror: Deal with filters

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5317 bytes --]

On 03.09.19 10:32, Vladimir Sementsov-Ogievskiy wrote:
> 02.09.2019 17:35, Max Reitz wrote:
>> On 31.08.19 11:57, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.08.2019 19:13, Max Reitz wrote:
>>>> This includes some permission limiting (for example, we only need to
>>>> take the RESIZE permission for active commits where the base is smaller
>>>> than the top).
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    block/mirror.c | 117 ++++++++++++++++++++++++++++++++++++++-----------
>>>>    blockdev.c     |  47 +++++++++++++++++---
>>>>    2 files changed, 131 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>> index 54bafdf176..6ddbfb9708 100644
>>>> --- a/block/mirror.c
>>>> +++ b/block/mirror.c
>>>> @@ -42,6 +42,7 @@ typedef struct MirrorBlockJob {
>>>>        BlockBackend *target;
>>>>        BlockDriverState *mirror_top_bs;
>>>>        BlockDriverState *base;
>>>> +    BlockDriverState *base_overlay;
>>>>    
>>>>        /* The name of the graph node to replace */
>>>>        char *replaces;
>>>> @@ -665,8 +666,10 @@ static int mirror_exit_common(Job *job)
>>>>                                 &error_abort);
>>>>        if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>>>>            BlockDriverState *backing = s->is_none_mode ? src : s->base;
>>>> -        if (backing_bs(target_bs) != backing) {
>>>> -            bdrv_set_backing_hd(target_bs, backing, &local_err);
>>>> +        BlockDriverState *unfiltered_target = bdrv_skip_rw_filters(target_bs);
>>>> +
>>>> +        if (bdrv_filtered_cow_bs(unfiltered_target) != backing) {
>>>> +            bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
>>>>                if (local_err) {
>>>>                    error_report_err(local_err);
>>>>                    ret = -EPERM;
>>>> @@ -715,7 +718,7 @@ static int mirror_exit_common(Job *job)
>>>>         * valid.
>>>>         */
>>>>        block_job_remove_all_bdrv(bjob);
>>>> -    bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
>>>> +    bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort);
>>>>    
>>>>        /* We just changed the BDS the job BB refers to (with either or both of the
>>>>         * bdrv_replace_node() calls), so switch the BB back so the cleanup does
>>>> @@ -812,7 +815,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>>>                return 0;
>>>>            }
>>>>    
>>>> -        ret = bdrv_is_allocated_above(bs, base, false, offset, bytes, &count);
>>>> +        ret = bdrv_is_allocated_above(bs, s->base_overlay, true, offset, bytes,
>>>> +                                      &count);
>>>>            if (ret < 0) {
>>>>                return ret;
>>>>            }
>>>> @@ -908,7 +912,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>>>>        } else {
>>>>            s->target_cluster_size = BDRV_SECTOR_SIZE;
>>>>        }
>>>> -    if (backing_filename[0] && !target_bs->backing &&
>>>> +    if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
>>>>            s->granularity < s->target_cluster_size) {
>>>>            s->buf_size = MAX(s->buf_size, s->target_cluster_size);
>>>>            s->cow_bitmap = bitmap_new(length);
>>>> @@ -1088,8 +1092,9 @@ static void mirror_complete(Job *job, Error **errp)
>>>>        if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
>>>>            int ret;
>>>>    
>>>> -        assert(!target->backing);
>>>> -        ret = bdrv_open_backing_file(target, NULL, "backing", errp);
>>>> +        assert(!bdrv_backing_chain_next(target));
>>>
>>> Preexisting, but seems we may crash here, I don't see where it is checked before, to
>>> return error if there is some backing. And even if we do so, we don't prevent appearing
>>> of target backing during mirror operation.
>>
>> The idea is that MIRROR_OPEN_BACKING_CHAIN is set only when using
>> drive-mirror with mode=existing.  In this case, we also set
>> BDRV_O_NO_BACKING for the target.
>>
>> You’re right that a user could add a backing chain to the target during
>> the operation.  They really have to make an effort to shoot themselves
>> in the foot for this because the target must have an auto-generated node
>> name.
>>
>> I suppose the best would be not to open the backing chain if the target
>> node already has a backing child?
> 
> Hmm, but we still should generate an error, as we can't do what was requested.

But the user didn’t request anything.  They just specified an existing
file as the target with mode=existing, then (for whatever reason) made a
real effort to add a backing node to it (i.e. they had to look up the
target’s auto-generated node name), and then the job finishes.

The user didn’t request us to open the backing chain of the target.
They just requested to mirror to an existing file.  If they manually
override that existing file’s backing chain, I think it makes sense to
keep it that way.

Max

>>>> +        ret = bdrv_open_backing_file(bdrv_skip_rw_filters(target), NULL,
>>>> +                                     "backing", errp);
>>>>            if (ret < 0) {
>>>>                return;
>>>>            }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]