Re: [PATCH v2 28/36] block: add bdrv_set_backing_noperm() transaction action

[Kevin Wolf <kwolf@redhat.com>]

Am 05.02.2021 um 17:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 05.02.2021 17:00, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Split out no-perm part of bdrv_set_backing_hd() as a separate
> > > transaction action. Note the in case of existing BdrvChild we reuse it,
> > > not recreate, just to do less actions.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > >   block.c | 111 +++++++++++++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 89 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 54fb6d24bd..617cba9547 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -101,6 +101,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
> > >                                       uint64_t perm, uint64_t shared_perm,
> > >                                       void *opaque, BdrvChild **child,
> > >                                       GSList **tran, Error **errp);
> > > +static void bdrv_remove_backing(BlockDriverState *bs, GSList **tran);
> > >   static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue
> > >                                  *queue, Error **errp);
> > > @@ -3194,45 +3195,111 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
> > >       }
> > >   }
> > > +typedef struct BdrvSetBackingNoPermState {
> > > +    BlockDriverState *bs;
> > > +    BlockDriverState *backing_bs;
> > > +    BlockDriverState *old_inherits_from;
> > > +    GSList *attach_tran;
> > > +} BdrvSetBackingNoPermState;
> > 
> > Why do we need the nested attach_tran instead of just including these
> > actions in the outer transaction?
> > 
> > > +static void bdrv_set_backing_noperm_abort(void *opaque)
> > > +{
> > > +    BdrvSetBackingNoPermState *s = opaque;
> > > +
> > > +    if (s->backing_bs) {
> > > +        s->backing_bs->inherits_from = s->old_inherits_from;
> > > +    }
> > > +
> > > +    tran_abort(s->attach_tran);
> > > +
> > > +    bdrv_refresh_limits(s->bs, NULL);
> > > +    if (s->old_inherits_from) {
> > > +        bdrv_refresh_limits(s->old_inherits_from, NULL);
> > > +    }
> > 
> > How is bs->inherits_from related to limits? I don't see a
> > bdrv_refresh_limits() call in bdrv_set_backing_noperm() that this would
> > undo.
> > 
> > > +}
> > > +
> > > +static void bdrv_set_backing_noperm_commit(void *opaque)
> > > +{
> > > +    BdrvSetBackingNoPermState *s = opaque;
> > > +
> > > +    tran_commit(s->attach_tran);
> > > +}
> > > +
> > > +static TransactionActionDrv bdrv_set_backing_noperm_drv = {
> > > +    .abort = bdrv_set_backing_noperm_abort,
> > > +    .commit = bdrv_set_backing_noperm_commit,
> > > +    .clean = g_free,
> > > +};
> > > +
> > >   /*
> > >    * Sets the bs->backing link of a BDS. A new reference is created; callers
> > >    * which don't need their own reference any more must call bdrv_unref().
> > >    */
> > > -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> > > -                         Error **errp)
> > > +static int bdrv_set_backing_noperm(BlockDriverState *bs,
> > > +                                   BlockDriverState *backing_bs,
> > > +                                   GSList **tran, Error **errp)
> > >   {
> > > -    bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
> > > -        bdrv_inherits_from_recursive(backing_hd, bs);
> > > +    int ret = 0;
> > > +    bool update_inherits_from = bdrv_chain_contains(bs, backing_bs) &&
> > > +        bdrv_inherits_from_recursive(backing_bs, bs);
> > > +    GSList *attach_tran = NULL;
> > > +    BdrvSetBackingNoPermState *s;
> > >       if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
> > > -        return;
> > > +        return -EPERM;
> > >       }
> > > -    if (backing_hd) {
> > > -        bdrv_ref(backing_hd);
> > > +    if (bs->backing && backing_bs) {
> > > +        bdrv_replace_child_safe(bs->backing, backing_bs, tran);
> > > +    } else if (bs->backing && !backing_bs) {
> > > +        bdrv_remove_backing(bs, tran);
> > > +    } else if (backing_bs) {
> > > +        assert(!bs->backing);
> > > +        ret = bdrv_attach_child_noperm(bs, backing_bs, "backing",
> > > +                                       &child_of_bds, bdrv_backing_role(bs),
> > > +                                       &bs->backing, &attach_tran, errp);
> > > +        if (ret < 0) {
> > > +            tran_abort(attach_tran);
> > 
> > This looks wrong to me, we'll call tran_abort() a second time through
> > bdrv_set_backing_noperm_abort() when the outer transaction aborts.
> > 
> > I also notice that the other two if branches do just add things to the
> > outer 'tran', it's just this branch that gets a nested one.
> > 
> > > +            return ret;
> > > +        }
> > >       }
> > > -    if (bs->backing) {
> > > -        /* Cannot be frozen, we checked that above */
> > > -        bdrv_unref_child(bs, bs->backing);
> > > -        bs->backing = NULL;
> > > -    }
> > > +    s = g_new(BdrvSetBackingNoPermState, 1);
> > > +    *s = (BdrvSetBackingNoPermState) {
> > > +        .bs = bs,
> > > +        .backing_bs = backing_bs,
> > > +        .old_inherits_from = backing_bs ? backing_bs->inherits_from : NULL,
> > > +    };
> > > +    tran_prepend(tran, &bdrv_set_backing_noperm_drv, s);
> > > -    if (!backing_hd) {
> > > -        goto out;
> > > +    /*
> > > +     * If backing_bs was already part of bs's backing chain, and
> > > +     * inherits_from pointed recursively to bs then let's update it to
> > > +     * point directly to bs (else it will become NULL).
> > 
> > Setting it to NULL was previously done by bdrv_unref_child().
> > 
> > bdrv_replace_child_safe() and bdrv_remove_backing() don't seem to do
> > this any more.
> 
> Hmm, yes.. May be we should move bdrv_unset_inherts_from() from
> bdrv_unref_child() to bdrv_replace_child_noperm() ?

Sounds good to me. This should hopefully be called for all graph changes
that could possibly happen.

Kevin

> > 
> > > +     */
> > > +    if (backing_bs && update_inherits_from) {
> > > +        backing_bs->inherits_from = bs;
> > >       }
> > > -    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
> > > -                                    bdrv_backing_role(bs), errp);
> > > -    /* If backing_hd was already part of bs's backing chain, and
> > > -     * inherits_from pointed recursively to bs then let's update it to
> > > -     * point directly to bs (else it will become NULL). */
> > > -    if (bs->backing && update_inherits_from) {
> > > -        backing_hd->inherits_from = bs;
> > > +    bdrv_refresh_limits(bs, NULL);
> > > +
> > > +    return 0;
> > > +}
> > 
> > Kevin
> > 
> 
> 
> -- 
> Best regards,
> Vladimir
>