Re: [PATCH v7 17/47] block: Re-evaluate backing file handling in reopen

[Max Reitz]

On 10.07.20 21:42, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Reopening a node's backing child needs a bit of special handling because
>> the "backing" child has different defaults than all other children
>> (among other things).  Adding filter support here is a bit more
>> difficult than just using the child access functions.  In fact, we often
>> have to directly use bs->backing because these functions are about the
>> "backing" child (which may or may not be the COW backing file).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 712230ef5c..8131d0b5eb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4026,26 +4026,56 @@ static int
>> bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>>           }
>>       }
>>   +    /*
>> +     * Ensure that @bs can really handle backing files, because we are
>> +     * about to give it one (or swap the existing one)
>> +     */
>> +    if (bs->drv->is_filter) {
>> +        /* Filters always have a file or a backing child */
>> +        if (!bs->backing) {
>> +            error_setg(errp, "'%s' is a %s filter node that does not
>> support a "
>> +                       "backing child", bs->node_name,
>> bs->drv->format_name);
>> +            return -EINVAL;
>> +        }
>> +    } else if (!bs->drv->supports_backing) {
>> +        error_setg(errp, "Driver '%s' of node '%s' does not support
>> backing "
>> +                   "files", bs->drv->format_name, bs->node_name);
>> +        return -EINVAL;
>> +    }
>> +
>>       /*
>>        * Find the "actual" backing file by skipping all links that point
>>        * to an implicit node, if any (e.g. a commit filter node).
>> +     * We cannot use any of the bdrv_skip_*() functions here because
>> +     * those return the first explicit node, while we are looking for
>> +     * its overlay here.
>>        */
>>       overlay_bs = bs;
>> -    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
>> -        overlay_bs = backing_bs(overlay_bs);
>> +    while (bdrv_filter_or_cow_bs(overlay_bs) &&
>> +           bdrv_filter_or_cow_bs(overlay_bs)->implicit)
>> +    {
>> +        overlay_bs = bdrv_filter_or_cow_bs(overlay_bs);
>>       }
> 
> I believe that little optimization would work properly:
> 
> 
> for (BlockDriverState *below_bs = bdrv_filter_or_cow_bs(overlay_bs);
>        below_bs && below_bs->implicit;
>        below_bs = bdrv_filter_or_cow_bs(overlay_bs)) {
>          overlay_bs = below_bs;
> }

Looks good, thanks.

>>         /* If we want to replace the backing file we need some extra
>> checks */
>> -    if (new_backing_bs != backing_bs(overlay_bs)) {
>> +    if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) {
>>           /* Check for implicit nodes between bs and its backing file */
>>           if (bs != overlay_bs) {
>>               error_setg(errp, "Cannot change backing link if '%s' has "
>>                          "an implicit backing file", bs->node_name);
>>               return -EPERM;
>>           }
>> -        /* Check if the backing link that we want to replace is
>> frozen */
>> -        if (bdrv_is_backing_chain_frozen(overlay_bs,
>> backing_bs(overlay_bs),
>> -                                         errp)) {
>> +        /*
>> +         * Check if the backing link that we want to replace is frozen.
>> +         * Note that
>> +         * bdrv_filter_or_cow_child(overlay_bs) == overlay_bs->backing,
>> +         * because we know that overlay_bs == bs, and that @bs
>> +         * either is a filter that uses ->backing or a COW format BDS
>> +         * with bs->drv->supports_backing == true.
>> +         */
>> +        if (bdrv_is_backing_chain_frozen(overlay_bs,
>> +                                        
>> child_bs(overlay_bs->backing), errp))
> What would be wrong with bdrv_filter_or_cow_bs(overlay_bs) here?

As the comment says, it’s the same thing.

I prefer ->backing here, because this function is about reopening the
->backing child.

>> +        {
>>               return -EPERM;
>>           }
>>           reopen_state->replace_backing_bs = true;
>> @@ -4196,7 +4226,7 @@ int bdrv_reopen_prepare(BDRVReopenState
>> *reopen_state, BlockReopenQueue *queue,
>>        * its metadata. Otherwise the 'backing' option can be omitted.
>>        */
>>       if (drv->supports_backing && reopen_state->backing_missing &&
>> -        (backing_bs(reopen_state->bs) ||
>> reopen_state->bs->backing_file[0])) {
> = BlockDriverState*
>> +        (reopen_state->bs->backing ||
>> reopen_state->bs->backing_file[0])) {
> 
> = BdrvChild*
> 
> Are we OK with that?

Sure, the question is whether it’s non-NULL, and BdrvChild.bs is always
non-NULL.

Max

signature.asc
Description: OpenPGP digital signature