Re: [PATCH v14 13/13] block: apply COR-filter to block-stream jobs

[Vladimir Sementsov-Ogievskiy]

11.12.2020 20:21, Max Reitz wrote:
> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
> > From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> >
> > This patch completes the series with the COR-filter applied to
> > block-stream operations.
> >
> > Adding the filter makes it possible in future implement discarding
> > copied regions in backing files during the block-stream job, to reduce
> > the disk overuse (we need control on permissions).
> >
> > Also, the filter now is smart enough to do copy-on-read with specified
> > base, so we have benefit on guest reads even when doing block-stream of
> > the part of the backing chain.
> >
> > Several iotests are slightly modified due to filter insertion.
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/stream.c             | 78 ++++++++++++++++++++++++++------------
> >   tests/qemu-iotests/030     |  8 ++--
> >   tests/qemu-iotests/141.out |  2 +-
> >   tests/qemu-iotests/245     | 20 ++++++----
> >   4 files changed, 72 insertions(+), 36 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index a7fd8945ad..b92f7de55b 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
>
> [...]
>
> > @@ -295,17 +287,49 @@ void stream_start(const char *job_id, BlockDriverState 
> > *bs,
>
> [...]
>
> > +    opts = qdict_new();
> > +
> > +    qdict_put_str(opts, "driver", "copy-on-read");
> > +    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
> > +    /* Pass the base_overlay node name as 'bottom' to COR driver */
> > +    qdict_put_str(opts, "bottom", base_overlay->node_name);
>
> Hm.  Should we set this option even if no base was specified?
>
> On one hand, omitting this option would cor_co_preadv_part() a bit quicker.
>
> On the other, what happens when you add a backing file below the bottom node 
> during streaming (yes, a largely theoretical case)...

Yes, that's what I was thinking about.

And more: we are moving to using "bottom" and deprecate "base". So bottom is 
the main concept, and it can't be NULL. If user don't specify it, than default bottom - is the 
current bottom node in the chain.

I think, we are not going to introduce a different behavior for stream "without 
bottom", when user can add more nodes to the chain during the job, and all of them 
will be removed after the job. It will require rethinking of freezing and keeping 
references on intermediate nodes at least..

>   Now, all data from it is ignored.  That seemed a bit strange to me at first, 
> but on second thought, it makes more sense.  Doing anything else would produce 
> a garbage result basically, because stream_run() doesn’t take such a change 
> into account.

yes

> So...  After all I think I agree with setting @bottom unconditionally.
>
> And that’s the only comment I had. :)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks! v15 will come next week)

--
Best regards,
Vladimir