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;
+
+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);
+    }
+}
+
+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);
+            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).
+     */
+    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;
+}
+
+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+                         Error **errp)
+{
+    int ret;
+    GSList *tran = NULL;
+
+    ret = bdrv_set_backing_noperm(bs, backing_hd, &tran, errp);
+    if (ret < 0) {
+        goto out;
     }
 
+    ret = bdrv_refresh_perms(bs, errp);
 out:
-    bdrv_refresh_limits(bs, NULL);
+    tran_finalize(tran, ret);
 }
 
 /*
-- 
2.21.3

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.

> +     */
> +    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

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() ?

> 
>> +     */
>> +    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

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
> 

05.02.2021 19:30, Kevin Wolf wrote:
> 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.
> 

This will break current "feature":

block jobs don't break inherits_from at all: when filter inserted inherits_from is broken. But when filter removed, it works again, as .inherits_from is not changed by bdrv_append().. So, I'll just try to keep current behavior around inherits_from as is.



-- 
Best regards,
Vladimir

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>

>  /*
>   * 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);

The old code with separate bdrv_unref_child() and then
bdrv_attach_child() tried to make the AioContests of bs and backing_bs
compatible by moving one of the nodes if necessary.

bdrv_replace_child_safe() doesn't seem to do that, but it only asserts
that both nodes are already in the same context.

I see that iotest 245 doesn't crash, which I think it should if this
were broken, but where does the switch happen now?

Kevin

05.02.2021 19:26, 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>
> 
>>   /*
>>    * 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);
> 
> The old code with separate bdrv_unref_child() and then
> bdrv_attach_child() tried to make the AioContests of bs and backing_bs
> compatible by moving one of the nodes if necessary.
> 
> bdrv_replace_child_safe() doesn't seem to do that, but it only asserts
> that both nodes are already in the same context.
> 
> I see that iotest 245 doesn't crash, which I think it should if this
> were broken, but where does the switch happen now?

Hmm. Seems on path "if (bs->backing && backing_bs) {" we really miss aio context handling. Probably 245 doesn't check this branch? Or if leaves different aio contexts in one subtree..


-- 
Best regards,
Vladimir