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

[Andrey Shinkevich]

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;
}
>   
>       /* 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?
> +        {
>               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?

>           error_setg(errp, "backing is missing for '%s'",
>                      reopen_state->bs->node_name);
>           ret = -EINVAL;
> @@ -4337,7 +4367,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>        * from bdrv_set_backing_hd()) has the new values.
>        */
>       if (reopen_state->replace_backing_bs) {
> -        BlockDriverState *old_backing_bs = backing_bs(bs);
> +        BlockDriverState *old_backing_bs = child_bs(bs->backing);
>           assert(!old_backing_bs || !old_backing_bs->implicit);
>           /* Abort the permission update on the backing bs we're detaching */
>           if (old_backing_bs) {