Re: [PATCH 06/22] block: Add bdrv_recurse_can_replace()

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

20.09.2019 18:27, Max Reitz wrote:
> After a couple of follow-up patches, this function will replace
> bdrv_recurse_is_first_non_filter() in check_to_replace_node().
> 
> bdrv_recurse_is_first_non_filter() is both not sufficiently specific for
> check_to_replace_node() (it allows cases that should not be allowed,
> like replacing child nodes of quorum with dissenting data that have more
> parents than just quorum), and it is too restrictive (it is perfectly
> fine to replace filters).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/block/block_int.h | 10 ++++++++++
>   block.c                   | 38 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 48 insertions(+)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5fd4f17d93..0be7d12f04 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -103,6 +103,13 @@ struct BlockDriver {
>        */
>       bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
>                                                BlockDriverState *candidate);
> +    /*
> +     * Return true if @to_replace can be replaced by a BDS with the
> +     * same data as @bs without it affecting @bs's behavior (that is,
> +     * without it being visible to @bs's parents).
> +     */
> +    bool (*bdrv_recurse_can_replace)(BlockDriverState *bs,
> +                                     BlockDriverState *to_replace);
>   
>       int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>       int (*bdrv_probe_device)(const char *filename);
> @@ -1254,6 +1261,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>                                  uint64_t perm, uint64_t shared,
>                                  uint64_t *nperm, uint64_t *nshared);
>   
> +bool bdrv_recurse_can_replace(BlockDriverState *bs,
> +                              BlockDriverState *to_replace);
> +
>   /*
>    * Default implementation for drivers to pass bdrv_co_block_status() to
>    * their file.
> diff --git a/block.c b/block.c
> index 7d99ca692c..a2deca4ac9 100644
> --- a/block.c
> +++ b/block.c
> @@ -6206,6 +6206,44 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
>       return false;
>   }
>   
> +/*
> + * This function checks whether the given @to_replace is allowed to be
> + * replaced by a node that always shows the same data as @bs.  This is
> + * used for example to verify whether the mirror job can replace
> + * @to_replace by the target mirrored from @bs.
> + * To be replaceable, @bs and @to_replace may either be guaranteed to
> + * always show the same data (because they are only connected through
> + * filters), or some driver may allow replacing one of its children
> + * because it can guarantee that this child's data is not visible at
> + * all (for example, for dissenting quorum children that have no other
> + * parents).
> + */
> +bool bdrv_recurse_can_replace(BlockDriverState *bs,
> +                              BlockDriverState *to_replace)
> +{
> +    if (!bs || !bs->drv) {
> +        return false;
> +    }
> +
> +    if (bs == to_replace) {
> +        return true;
> +    }
> +
> +    /* For filters, we can recurse on our own */
> +    if (bs->drv->is_filter) {
> +        BdrvChild *child = bs->file ?: bs->backing;

then, maybe asset(!bs->drv->bdrv_recurse_can_replace)

> +        return bdrv_recurse_can_replace(child->bs, to_replace);
> +    }

or, this may be filter-skipping loop instead of recursion, like

while (bs && bs->drv && bs->drv->is_filter) {
   bs = (bs->file ?: bs->backing)->bs;
}

at function start.

either way:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +
> +    /* See what the driver can do */
> +    if (bs->drv->bdrv_recurse_can_replace) {
> +        return bs->drv->bdrv_recurse_can_replace(bs, to_replace);
> +    }
> +
> +    /* Safe default */
> +    return false;
> +}
> +
>   BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
>                                           const char *node_name, Error **errp)
>   {
> 


-- 
Best regards,
Vladimir