Re: [Devel] [PATCH VZ10 v2 09/10] drivers/md/dm-qcow2: allow shared L1 entries during merge from RO image

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

On 8/12/26 22:20, Andrey Zhadchenko wrote:
> For RO images with internal snapshots L1 entries are shared.
> Write-mode metadata parsing stops at such entries to avoid
> modifying a shared L2 table, which made prepare_backward_merge()
> see the cluster as unallocated and skip it with
> "nothing to merge": the merged result would silently lose all
> data under shared L1 entries.
> Allow such scenario in parse_l1() and ease WARN in
> prepare_backward_merge().
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-138288
> Feature: dm-qcow2: block device over QCOW2 files driver
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>  drivers/md/dm-qcow2-cmd.c |  2 --
>  drivers/md/dm-qcow2-map.c | 18 ++++++++++++++++--
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
> index f2c8f51e17af6..fa3762c58372b 100644
> --- a/drivers/md/dm-qcow2-cmd.c
> +++ b/drivers/md/dm-qcow2-cmd.c
> @@ -266,8 +266,6 @@ static int qcow2_merge_backward_start(struct qcow2_target *tgt, int efd, u32 dep
>  		return -ENOENT;
>  	if (!qcow2_file_is_writable(lower))
>  		return -EACCES;
> -	if (!qcow2_file_is_writable(qcow2) && qcow2->hdr.nb_snapshots)
> -		return -EACCES;

Having all these add in one patch remove in next one blocks does not feel right.
Let's try to merge all fixups in final version.

>  	if (qcow2->clu_size != lower->clu_size)
>  		return -EOPNOTSUPP;
>  	if (lower->hdr.size < qcow2->hdr.size)
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index ebe68e8d0d3d8..baa6778f1686c 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -1726,6 +1726,14 @@ static bool qio_is_fully_alloced(struct qcow2 *qcow2, struct qio *qio,
>  	return !(subclus_mask & ~alloced_mask);
>  }
>  
> +static bool qio_may_modify_image(struct qcow2 *qcow2, struct qio *qio)
> +{
> +	if (qcow2_file_is_writable(qcow2))
> +		return true;
> +
> +	return !fake_merge_qio(qio);
> +}
> +
>  static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>  		       struct qio **qio, bool write)
>  
> @@ -1762,7 +1770,8 @@ static loff_t parse_l1(struct qcow2 *qcow2, struct qcow2_map *map,
>  		goto out;
>  	if (delay_if_dirty(qcow2, l1->md, l1->index_in_page, qio))
>  		goto out;
> -	if (write && map->clu_is_cow)
> +	/* Don't refuse L1 parse for merge qios with readonly disks */
> +	if (write && map->clu_is_cow && qio_may_modify_image(qcow2, *qio))
>  		goto out; /* Avoid to return pos */
>  
>  	ret = pos;
> @@ -2061,6 +2070,10 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
>  	      qio_discard_unmaps_cluster(qcow2, *qio, map)))
>  		return 0;
>  
> +	/* Don't need refcount table if we don't modify the image */
> +	if (!qio_may_modify_image(qcow2, *qio))
> +		return 0;
> +
>  	/* Now refcounters table/block */
>  	ret = qcow2_handle_r1r2_maps(qcow2, pos, qio, &map->r1,
>  			       &map->r2, map->compressed);
> @@ -2749,7 +2762,8 @@ static int prepare_backward_merge(struct qcow2 *qcow2, struct qio **qio,
>  	}
>  
>  	if (!map->data_clu_alloced) {
> -		WARN_ON_ONCE(map->clu_is_cow); /* Strange COW at L1 */
> +		/* Strange COW at L1, except the merge from RO image */
> +		WARN_ON_ONCE(map->clu_is_cow && qio_may_modify_image(qcow2, *qio));
>  		if (fake_merge_qio(*qio)) {
>  			/* Nothing is to merge */
>  			goto endio;

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.