Re: [PATCH v8 2/7] copy-on-read: add filter append/drop functions

[Andrey Shinkevich]

On 04.09.2020 14:22, Max Reitz wrote:

> On 28.08.20 18:52, Andrey Shinkevich wrote:
> > Provide API for the COR-filter insertion/removal.
> >       
>     
    ...
> > Also, drop the filter child permissions for an inactive state when the
> > filter node is being removed.
> >       
> Do we need .active for that?  Shouldn’t it be sufficient to just not
> require BLK_PERM_WRITE_UNCHANGED when no permissions are taken on the
> node (i.e. perm == 0 in cor_child_perm())?
>
> Of course, using an .active field works, too.  But Vladimir wanted a
> similar field in the preallocate filter, and there already is in
> backup-top.  I feel like there shouldn’t be a need for this.
>
> .bdrv_child_perm() should generally be able to decide what permissions
> it needs based on the information it gets.  If every driver needs to
> keep track of whether it has any parents and feed that information into
> .bdrv_child_perm() as external state, then something feels wrong.
>
> If perm == 0 is not sufficient to rule out any parents, we should just
> explicitly add a boolean that tells .bdrv_child_perm() whether there are
> any parents or not – shouldn’t be too difficult.
>

The issue is that we fail in the bdrv_check_update_perm() with ""Conflicts with use by..." if the *nperm = 0 and the *nshared = BLK_PERM_ALL are not set before the call to the bdrv_replace_node(). The filter is still in the backing chain by that moment and has a parent with child->perm != 0.

The implementation of  the .bdrv_child_perm() in the given patch is similar to one in the block/mirror.c.

How could we resolve the issue at the generic layer?

Andrey

> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  block/copy-on-read.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  block/copy-on-read.h |  35 +++++++++++++++++
> >  2 files changed, 139 insertions(+)
> >  create mode 100644 block/copy-on-read.h
> >       
> [...]
>