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

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, armbru@redhat.com,
	jsnow@redhat.com, mreitz@redhat.com, den@openvz.org
Subject: Re: [PATCH v2 28/36] block: add bdrv_set_backing_noperm() transaction action
Date: Fri, 5 Feb 2021 19:06:10 +0300	[thread overview]
Message-ID: <9fe6a325-4af7-5d97-fc0d-06911f7c677a@virtuozzo.com> (raw)
In-Reply-To: <20210205140028.GC7072@merkur.fritz.box>

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