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 <vsement...@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
>
