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

[Vladimir Sementsov-Ogievskiy]

27.04.2020 7:08, Andrey Shinkevich wrote:
> 1. The mirror technology instantiates its own BLK, sets 0/BLK_PERM_ALL for it 
> on exit and then, via subsequent call to bdrv_child_refresh_perms(), calls 
> bdrv_child_try_set_perm(). We don't have the extra BLK for our filter in stream 
> and do call the same function directly.

deal with blk is unrelated, as I understand. Changing permissions shoud go 
through bdrv_child_refresh_perms and modification of the state of the node, 
like it is done in upstream mirror-top and backup-top. Just call 
bdrv_child_try_set_perm() is very risky: on any subsequent call of 
bdrv_child_refresh_perms (may be implicit, by other functions) you'll lose the 
permissions you set by hand. So, again, setting permissions by hand is wrong 
thing.

> 2. The function remove_filter() can't be used in the stream job on exit because 
> we should remove the filter and change a backing file in the same critical 
> 'drain' section.

Hmm. It's a question of refactoring. Why not create function 
remove_filter_drained?

> 3. Due to the specifics above, I suggest that we make insert/remove filter as 
> an interface when there gets another user of it.

Please look at backup-top filter. Could we be as close to it as possible? I'm 
sure, that finally we should have one API for filter insertion/removing, not 
per filter.

> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Vladimir Sementsov-Ogievskiy <address@hidden>
> *Sent:* Tuesday, April 21, 2020 3:58 PM
> *To:* Andrey Shinkevich <address@hidden>; address@hidden <address@hidden>
> *Cc:* address@hidden <address@hidden>; address@hidden <address@hidden>; address@hidden <address@hidden>; 
> address@hidden <address@hidden>; address@hidden <address@hidden>; address@hidden <address@hidden>; 
> address@hidden <address@hidden>; Denis Lunev <address@hidden>
> *Subject:* Re: [PATCH 7/7] block: apply COR-filter to block-stream jobs
> 20.04.2020 21:36, Andrey Shinkevich wrote:
> > The patch completes the series with the COR-filter insertion to any
> > block-stream operation. It also makes changes to the iotests 030, 141
> > and 245.
> >
> > Signed-off-by: Andrey Shinkevich <address@hidden>
> > ---
> >    block/stream.c             | 151 
> > +++++++++++++++++++++++++++++++++++++++------
> >    tests/qemu-iotests/030     |   6 +-
> >    tests/qemu-iotests/141.out |   2 +-
> >    tests/qemu-iotests/245     |   5 +-
> >    4 files changed, 141 insertions(+), 23 deletions(-)
> >
> > diff --git a/block/stream.c b/block/stream.c
> > index fab7923..af14ba8 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
>
>
> [..]
>
> > +static int stream_exit(Job *job, bool abort)
> > +{
> > +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> > +    BlockJob *bjob = &s->common;
> > +    BlockDriverState *target_bs = s->target_bs;
> > +    int ret = 0;
> > +
> > +    if (s->chain_frozen) {
> > +        bdrv_unfreeze_chain(s->target_bs, s->bottom_cow_node);
> > +        s->chain_frozen = false;
> > +    }
> > +
> > +    /* Retain the BDS until we complete the graph change. */
> > +    bdrv_ref(target_bs);
> > +    /* Hold a guest back from writing while permissions are being reset. */
> > +    bdrv_drained_begin(target_bs);
> > +    /* Drop permissions before the graph change. */
> > +    bdrv_child_try_set_perm(bdrv_filtered_rw_child(s->cor_filter_bs),
> > +                            0, BLK_PERM_ALL, &error_abort);
>
> Hmm. I don't remember what's wrong with it, but something is. Neither mirror 
> nor backup do this now,
> instead they use some flag, that permissions are not needed anymore and call 
> bdrv_child_refresh_perms()
>
> Also, strange that you have insert_filter function, but don't use it here.
>
> Also, could we keep add/remove filter api in block/copy-on-read.c, like it is 
> done for backup-top ?
>
> --
> Best regards,
> Vladimir


--
Best regards,
Vladimir