Re: [PATCH v7 33/47] mirror: Deal with filters

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 24.07.2020 12:49, Max Reitz wrote:
> On 22.07.20 20:31, Andrey Shinkevich wrote:
>> On 25.06.2020 18:22, 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).
>>>
>>> Use this opportunity to rename qmp_drive_mirror()'s "source" BDS to
>>> "target_backing_bs", because that is what it really refers to.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>    qapi/block-core.json |   6 ++-
>>>    block/mirror.c       | 118 +++++++++++++++++++++++++++++++++----------
>>>    blockdev.c           |  36 +++++++++----
>>>    3 files changed, 121 insertions(+), 39 deletions(-)
>>>
>> ...
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 469acf4600..770de3b34e 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;
>>> @@ -677,8 +678,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_filters(target_bs);
>>> +
>>> +        if (bdrv_cow_bs(unfiltered_target) != backing) {
>>
>> I just worry about a filter node of the concurrent job right below the
>> unfiltered_target.
> Having a concurrent job on the target sounds extremely problematic in
> itself (because at least for most of the mirror job, the target isn’t in
> a consistent state).  Is that a real use case?


It might be at the TestParallelOps of iotests #30 but I am not sure now. 
I am going to apply my series with copy-on-read filter for the stream 
job above this one and will see then.

Andrey


>
>> The filter has unfiltered_target in its parent list.
>> Will that filter node be replaced correctly then?
> I’m also not quite sure what you mean.  We need to attach the source’s
> backing chain to the target here, so we go down to the first node that
> might accept COW backing files (by invoking bdrv_skip_filters()).  That
> should be correct no matter what kind of filters are on it.


I ment when a filter is removed with the bdrv_replace_node() afterwards. 
As I mentioned above, I am going to test the case later.

Andrey


>>> +        /*
>>> +         * The topmost node with
>>> +         * bdrv_skip_filters(filtered_target) ==
>>> bdrv_skip_filters(target)
>>> +         */
>>> +        filtered_target = bdrv_cow_bs(bdrv_find_overlay(bs, target));
>>> +
>>> +        assert(bdrv_skip_filters(filtered_target) ==
>>> +               bdrv_skip_filters(target));
>>> +
>>> +        /*
>>> +         * XXX BLK_PERM_WRITE needs to be allowed so we don't block
>>> +         * ourselves at s->base (if writes are blocked for a node,
>>> they are
>>> +         * also blocked for its backing file). The other options
>>> would be a
>>> +         * second filter driver above s->base (== target).
>>> +         */
>>> +        iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
>>> +
>>> +        for (iter = bdrv_filter_or_cow_bs(bs); iter != target;
>>> +             iter = bdrv_filter_or_cow_bs(iter))
>>> +        {
>>> +            if (iter == filtered_target) {
>>
>> For one filter node only?
> No, iter_shared_perms is never reset, so it retains the
> BLK_PERM_CONSISTENT_READ flag until the end of the loop.


Yes, that's right. Clear.

Andrey


>
>>> +                /*
>>> +                 * From here on, all nodes are filters on the base.
>>> +                 * This allows us to share BLK_PERM_CONSISTENT_READ.
>>> +                 */
>>> +                iter_shared_perms |= BLK_PERM_CONSISTENT_READ;
>>> +            }
>>> +
>>>                ret = block_job_add_bdrv(&s->common, "intermediate
>>> node", iter, 0,
>>> -                                     BLK_PERM_WRITE_UNCHANGED |
>>> BLK_PERM_WRITE,
>>> -                                     errp);
>>> +                                     iter_shared_perms, errp);
>>>                if (ret < 0) {
>>>                    goto fail;
>>>                }
>> ...
>>> @@ -3042,6 +3053,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error
>>> **errp)
>>>                                 " named node of the graph");
>>>                goto out;
>>>            }
>>> +        replaces_node_name = arg->replaces;
>>
>> What is the idea behind the variables substitution?
> Looks like a remnant from v6, where there was an
>
> if (arg->has_replaces) {
>      ...
>      replaces_node_name = arg->replaces;
> } else if (unfiltered_bs != bs) {
>      replaces_node_name = unfiltered_bs->node_name;
> }
>
> But I moved that logic to blockdev_mirror_common() in this version.
>
> So it’s just useless now and replaces_node_name shouldn’t exist.
>
> Max
>