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/21/26 12:43, Pavel Tikhomirov wrote:
> 
> 
> 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.

Sorry, maybe I'm wrong, and it's more of a separate feature here again.

> 
>>  	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.