Re: [PATCH v3 22/36] block: add bdrv_remove_filter_or_cow transaction action

[Vladimir Sementsov-Ogievskiy]

26.04.2021 19:26, Kevin Wolf wrote:
> Am 17.03.2021 um 15:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 11f7ad0818..2fca1f2ad5 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2929,12 +2929,19 @@ static void bdrv_replace_child(BdrvChild *child, 
> > BlockDriverState *new_bs)
> >       }
> >   }
> >   
> > +static void bdrv_child_free(void *opaque)
> > +{
> > +    BdrvChild *c = opaque;
> > +
> > +    g_free(c->name);
> > +    g_free(c);
> > +}
> > +
> >   static void bdrv_remove_empty_child(BdrvChild *child)
> >   {
> >       assert(!child->bs);
> >       QLIST_SAFE_REMOVE(child, next);
> > -    g_free(child->name);
> > -    g_free(child);
> > +    bdrv_child_free(child);
> >   }
> >   
> >   typedef struct BdrvAttachChildCommonState {
> > @@ -4956,6 +4963,73 @@ static bool should_update_child(BdrvChild *c, 
> > BlockDriverState *to)
> >       return ret;
> >   }
> >   
> > +typedef struct BdrvRemoveFilterOrCowChild {
> > +    BdrvChild *child;
> > +    bool is_backing;
> > +} BdrvRemoveFilterOrCowChild;
> > +
> > +/* this doesn't restore original child bs, only the child itself */
>
> Hm, this comment tells me that it's intentional, but why is it correct?

that's because bdrv_remove_filter_or_cow_child_abort() aborts only part of  
bdrv_remove_filter_or_cow_child().

Look: bdrv_remove_filter_or_cow_child() firstly do 
bdrv_replace_child_safe(child, NULL, tran);, so bs would be restored by 
.abort() of bdrv_replace_child_safe() action.


So, improved comment may look like:

This doesn't restore original child bs, only the child itself. The bs would be 
restored by .abort() bdrv_replace_child_safe() subation of 
bdrv_remove_filter_or_cow_child() action.

Probably it would be more correct to rename

BdrvRemoveFilterOrCowChild -> BdrvRemoveFilterOrCowChildNoBs
bdrv_remove_filter_or_cow_child_abort -> 
bdrv_remove_filter_or_cow_child_no_bs_abort
bdrv_remove_filter_or_cow_child_commit -> 
bdrv_remove_filter_or_cow_child_no_bs_commit

and assert on .abort() and .commit() that s->child->bs is NULL.

> > +static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
> > +{
> > +    BdrvRemoveFilterOrCowChild *s = opaque;
> > +    BlockDriverState *parent_bs = s->child->opaque;
> > +
> > +    QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
> > +    if (s->is_backing) {
> > +        parent_bs->backing = s->child;
> > +    } else {
> > +        parent_bs->file = s->child;
> > +    }
> > +}
>
> Kevin


--
Best regards,
Vladimir