Re: [Qemu-devel] [PATCH v5 15/42] block: Re-evaluate backing file handling in reopen

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

13.06.2019 1:09, 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 | 36 +++++++++++++++++++++++++++++-------
>   1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 505b3e9a01..db2759c10d 100644
> --- a/block.c
> +++ b/block.c
> @@ -3542,17 +3542,39 @@ 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;
> +    }

hmm, shouldn't we have these checks for overlay_bs?

> +
>       /*
>        * 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_filtered_bs(overlay_bs) &&
> +           bdrv_filtered_bs(overlay_bs)->implicit)
> +    {
> +        overlay_bs = bdrv_filtered_bs(overlay_bs);
>       }

here, overlay_bs may be some filter with file child ..

>   
>       /* 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_filtered_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 "
> @@ -3560,8 +3582,8 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state,
>               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)) {
> +        if (bdrv_is_backing_chain_frozen(overlay_bs,
> +                                         child_bs(overlay_bs->backing), errp)) {

.. and here we are doing wrong thing, as it don't have backing child

Aha, you use the fact that we now don't have implicit filters with file child. Then, should
we add an assertion for this?

>               return -EPERM;
>           }
>           reopen_state->replace_backing_bs = true;
> @@ -3712,7 +3734,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])) {
> +        (reopen_state->bs->backing || reopen_state->bs->backing_file[0])) {
>           error_setg(errp, "backing is missing for '%s'",
>                      reopen_state->bs->node_name);
>           ret = -EINVAL;
> @@ -3857,7 +3879,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) {
> 


-- 
Best regards,
Vladimir