Re: [PATCH v2 25/36] block: introduce bdrv_drop_filter()

[Vladimir Sementsov-Ogievskiy]

04.02.2021 14:31, Kevin Wolf wrote:
> Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Using bdrv_replace_node() for removing filter is not good enough: it
> > keeps child reference of the filter, which may conflict with original
> > top node during permission update.
> >
> > Instead let's create new interface, which will do all graph
> > modifications first and then update permissions.
> >
> > Let's modify bdrv_replace_node_common(), allowing it additionally drop
> > backing chain child link pointing to new node. This is quite
> > appropriate for bdrv_drop_intermediate() and makes possible to add
> > new bdrv_drop_filter() as a simple wrapper.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   include/block/block.h |  1 +
> >   block.c               | 42 ++++++++++++++++++++++++++++++++++++++----
> >   2 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 8f6100dad7..0f21ef313f 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -348,6 +348,7 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState 
> > *bs_top,
> >                   Error **errp);
> >   int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> >                         Error **errp);
> > +int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
> >   
> >   int bdrv_parse_aio(const char *mode, int *flags);
> >   int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
> > diff --git a/block.c b/block.c
> > index b1394b721c..e835a78f06 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4919,7 +4919,6 @@ static TransactionActionDrv bdrv_remove_backing_drv = {
> >       .commit = bdrv_child_free,
> >   };
> >   
> > -__attribute__((unused))
> >   static void bdrv_remove_backing(BlockDriverState *bs, GSList **tran)
> >   {
> >       if (!bs->backing) {
> > @@ -4968,15 +4967,30 @@ static int bdrv_replace_node_noperm(BlockDriverState 
> > *from,
> >    *
> >    * With auto_skip=false the error is returned if from has a parent which 
> > should
> >    * not be updated.
> > + *
> > + * With detach_subchain to must be in a backing chain of from. In this case
>
> @to and @from make it easier to read.
>
> > + * backing link of the cow-parent of @to is removed.
> >    */
> >   static int bdrv_replace_node_common(BlockDriverState *from,
> >                                       BlockDriverState *to,
> > -                                    bool auto_skip, Error **errp)
> > +                                    bool auto_skip, bool detach_subchain,
> > +                                    Error **errp)
> >   {
> >       int ret = -EPERM;
> >       GSList *tran = NULL;
> >       g_autoptr(GHashTable) found = NULL;
> >       g_autoptr(GSList) refresh_list = NULL;
> > +    BlockDriverState *to_cow_parent;
> > +
> > +    if (detach_subchain) {
> > +        assert(bdrv_chain_contains(from, to));
>
> The loop below also relies on from != to, so maybe assert that, too.
>
> > +        for (to_cow_parent = from;
> > +             bdrv_filter_or_cow_bs(to_cow_parent) != to;
> > +             to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
> > +        {
> > +            ;
> > +        }
> > +    }
> >   
> >       /* Make sure that @from doesn't go away until we have successfully attached
> >        * all of its parents to @to. */
> > @@ -4997,6 +5011,10 @@ static int bdrv_replace_node_common(BlockDriverState 
> > *from,
> >           goto out;
> >       }
> >   
> > +    if (detach_subchain) {
> > +        bdrv_remove_backing(to_cow_parent, &tran);
> > +    }
>
> So bdrv_drop_filter() only works for filters that go through
> bs->backing?
>
> Wouldn't it have been more useful to make it bdrv_remove_filter_or_cow()
> like you use already use in other places in this patch?
>
> If not, the limitation needs to be documented for bdrv_drop_filter().

bdrv_append supports only bs->backing based filters too.. So for now it's 
enough. And probably in future we'll refactor it all again when implement 
multi-reopen qmp command. Will look at it when prepare new version and either 
improve or document limitation.


--
Best regards,
Vladimir