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

[Andrey Shinkevich]

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