Re: [PATCH v2 09/10] block: Let replace_child_noperm free children

[Vladimir Sementsov-Ogievskiy]

15.11.2021 16:04, Hanna Reitz wrote:
> On 12.11.21 17:10, Vladimir Sementsov-Ogievskiy wrote:
> > 11.11.2021 15:08, Hanna Reitz wrote:
> > > In most of the block layer, especially when traversing down from other
> > > BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
> > > it becomes NULL, it is expected that the corresponding BdrvChild pointer
> > > also becomes NULL and the BdrvChild object is freed.
> > >
> > > Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
> > > pointer to NULL, it should also immediately set the corresponding
> > > BdrvChild pointer (like bs->file or bs->backing) to NULL.
> > >
> > > In that context, it also makes sense for this function to free the
> > > child.  Sometimes we cannot do so, though, because it is called in a
> > > transactional context where the caller might still want to reinstate the
> > > child in the abort branch (and free it only on commit), so this behavior
> > > has to remain optional.
> > >
> > > In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
> > > that the BdrvChild passed to bdrv_replace_child_tran() must have had a
> > > non-NULL .bs pointer initially.  Make a note of that and assert it.
> > >
> > > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> > > ---
> > >   block.c | 102 +++++++++++++++++++++++++++++++++++++++++++-------------
> > >   1 file changed, 79 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/block.c b/block.c
> > > index a40027161c..0ac5b163d2 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char 
> > > *filename,
> > >   static bool bdrv_recurse_has_child(BlockDriverState *bs,
> > >                                      BlockDriverState *child);
> > >   +static void bdrv_child_free(BdrvChild *child);
> > >   static void bdrv_replace_child_noperm(BdrvChild **child,
> > > -                                      BlockDriverState *new_bs);
> > > +                                      BlockDriverState *new_bs,
> > > +                                      bool free_empty_child);
> > >   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
> > >                                                 BdrvChild *child,
> > >                                                 Transaction *tran);
> > > @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
> > >       BdrvChild *child;
> > >       BdrvChild **childp;
> > >       BlockDriverState *old_bs;
> > > +    bool free_empty_child;
> > >   } BdrvReplaceChildState;
> > >     static void bdrv_replace_child_commit(void *opaque)
> > >   {
> > >       BdrvReplaceChildState *s = opaque;
> > >   +    if (s->free_empty_child && !s->child->bs) {
> > > +        bdrv_child_free(s->child);
> > > +    }
> > >       bdrv_unref(s->old_bs);
> > >   }
> > >   @@ -2278,22 +2284,26 @@ static void bdrv_replace_child_abort(void *opaque)
> > >        *     modify the BdrvChild * pointer we indirectly pass to it, i.e. it
> > >        *     will not modify s->child.  From that perspective, it does not 
> > > matter
> > >        *     whether we pass s->childp or &s->child.
> > > -     *     (TODO: Right now, bdrv_replace_child_noperm() never modifies that
> > > -     *     pointer anyway (though it will in the future), so at this point it
> > > -     *     absolutely does not matter whether we pass s->childp or &s->child.)
> > >        * (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
> > >        *     it here.
> > >        * (3) If new_bs is NULL, *s->childp will have been NULLed by
> > >        *     bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and 
> > > we
> > >        *     must not pass a NULL *s->childp here.
> > > -     *     (TODO: In its current state, bdrv_replace_child_noperm() will not
> > > -     *     have NULLed *s->childp, so this does not apply yet.  It will in the
> > > -     *     future.)
> >
> > What I don't like about this patch is that it does two different things: 
> > zeroing the pointer and clearing the object. And if we look at the latter in 
> > separate, it seems that it's not needed:
> >
> > Look: bdrv_replace_child_tran(): new parameter is set to true in two places, in 
> > both of them we are sure (and do assertion and comment) that new bs is not NULL 
> > and nothing will be freed.
> >
> > Similarly, bdrv_replace_child_noperm() is called with true in two places where 
> > we sure that new bs is not NULL.
> >
> > and only one place where new parameter set to true really do something:
> >
> > > @@ -2960,8 +3013,7 @@ static void bdrv_detach_child(BdrvChild **childp)
> > >   {
> > >       BlockDriverState *old_bs = (*childp)->bs;
> > >   -    bdrv_replace_child_noperm(childp, NULL);
> > > -    bdrv_child_free(*childp);
> > > +    bdrv_replace_child_noperm(childp, NULL, true);
> > >         if (old_bs) {
> > >           /*
> >
> > And it doesn't worth the whole complexity of new parameters for two functions.
> >
> > In this place we can simply do something like
> >
> > BdrvChild *child = *childp;
> >
> > bdrv_replace_child_noperm(childp, NULL);
> >
> > bdrv_child_free(child);
> >
> >
> > I understand the idea: it seems good and intuitive to do zeroing the pointer and clearing 
> > the object in one shot. But this patch itself shows that we just can't do it in 90% of 
> > cases. So, I think better is not do it and live with only "zeroing the pointer" 
> > part of this patch.
>
> I see your point, but I don’t find it too complex.  Passing `true` is the 
> default and then calling the function is easy.  Passing `false` means there’s a 
> catch, and then the caller is already complex anyway, so it doesn’t really make 
> things worse.
>
> I find the condition on when to pass `true` and when to pass `false` simple: 
> Always pass true, unless the child cannot be deleted yet.
>
> There are two reasons why I’d rather keep the parameter:
> (1) That’s how it’s already merged, and I’m biased against respinning given 
> that Kevin will be on PTO beginning tomorrow, and that we’re in freeze, so I’d 
> rather not miss an RC.

OK, that of course makes sense)

> (2) I really dislike a function that takes a pointer, NULLs it, and then 
> doesn’t free the object it belongs to.  I find that a bad interface. 
> Unfortunately we sometimes need this behavior, though, hence the additional 
> parameter.  And this parameter basically asks the caller whether they want the 
> reasonable interface (`true`) or the weird one where the pointer is NULLed but 
> the object isn’t freed (`false`).  I find this makes the interface palatable to 
> me.
>
> > Another idea that come to my mind while reviewing this series: did you consider zeroing 
> > bs->file / bs->backing in .detach, like you do with bs->children list at start of 
> > the series?  We can argue the same way that file and backing pointers are property of 
> > parent, and they should be zeroed in .detach, where element is removed from bs->children.
>
> Yes, I did.  The problem is that to make this right, .attach() would symmetrically need to 
> put the child into bs->file or bs->backing (e.g. when the removal transaction is 
> aborted).  That would not only be more invasive (we’d have to deal with and modify the 
> places where bs->file or bs->backing is set), you’re then also facing the problem of 
> giving .attach() this information.
>
> Perhaps we could let .detach() clear the pointer and not set it in .attach(), 
> but that seemed sufficiently wrong to me that I didn’t consider it further.

OK, reasonable.

If I have a good idea, I'll come with patches on top of this.

--
Best regards,
Vladimir