Re: [Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 8/19/26 13:05, Pavel Tikhomirov wrote:
> 
> 
> On 8/12/26 22:20, Andrey Zhadchenko wrote:
>> Discard does not guarantee zero data. Therefore, to safely erase
>> data, users may write zeroes and then discard. Imagine we have
>> a backing file. If we write zeroes and do the discard, next read
>> will give the stale data from backing image.
>> Probably this is rather an edge case, but to be sure let's just
>> set L2 entry to L2_READS_ALL_ZEROES so the device looks more
>> consistent to users.
>>
>> Feature: dm-qcow2: block device over QCOW2 files driver
>> https://virtuozzo.atlassian.net/browse/VSTOR-139406
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> 
> I'm not fully understating it all together, but this sounds like a regression fix for:
> 
> [PATCH VZ10 v2 03/10] drivers/md/dm-qcow2: update metadata on whole cluster discard (1)
> 
> rather then a fix of something preexisting, before this patch we had allocated cluster
> with punch-holed data there and never went to backing image, after it we start see stale
> data. So suggestion here is to somehow reorder or merge this fix into the introducing
> patch in the series to fix bisectability.

Yes and no :)
Yes in the sense that this 'problem' didn't exist beforehand. But 
previously discard always led to COW and small discards would pull the 
whole cluster from backing image.
No in the sense that contents after DISCARD are formally undefined so we 
are just being extra cautious here. So this can be viewed as an extra 
feature.

So I have split the code and arranged the patches for easier 
feature-by-feature review. I can still merge this patch in metadata 
processing patch if you wish.

Also note that even after this patch reads after discard may still show 
lower delta data. For example discarding less than a cluster (or 
subcluster after next patches) wouldn't cause 'reads as zeroes' metadata 
changes. If the image has no allocated clu/subclu subsequent reads will 
return lower delta data. It is again fine, as discard is advisory.


> 
> AI thoughts on (1):
> 
> A whole-cluster discard on a delta with a lower image now returns the backing image's data on the next read. The new path zeroes the L2 entry (and the ext_l2 bitmap), so parse_l2() leaves data_clu_alloced and all_zeroes clear, qio_unmapped_size() reports the whole cluster, and calc_front_qio_bytes() sets try_lower = maybe_mapped_in_lower_delta() ? process_read_qio() redirects the read to qcow2->lower.

 From what I see in calc_front_qio_bytes():

...
         arg->try_lower = false;

         size = qio_all_zeroes_size(qcow2, qio, map);
         if (size) {
                 arg->zeroes = true;
                 return size;
         }
...

So no, I don't think this leaks.


> 
> 
>> ---
>>   drivers/md/dm-qcow2-map.c | 24 +++++++++++++++++++++---
>>   drivers/md/dm-qcow2.h     |  1 +
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> index dcc65735881d4..3e3d10c3ebec4 100644
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
>> @@ -1309,6 +1309,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   	struct page *pe_page;
>>   	bool skip_odd;
>>   	u64 pos, old;
>> +	bool cleared;
>>   	int i, ret;
>>   
>>   	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
>> @@ -1317,6 +1318,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>>   		pos = get_u64_from_be_page(wbd->md->page, i);
>>   
>> +		cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
>> +
>>   		/* Here we restore prealloced and compressed clu mappings */
>>   		pe_page = wbd->pe_page;
>>   		if (pe_page) { /* Only L2 has this. */
>> @@ -1330,7 +1333,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   		set_u64_to_be_page(wbd->md->page, i, 0);
>>   		if (skip_odd && (i & 1))
>>   			continue; /* pos contains ext_l2 part of L2 entry */
>> -		if (!pos)
>> +		if (cleared)
>>   			continue; /* no cluster was allocated */
>>   
>>   		spin_unlock(&qcow2->md_pages_lock);
>> @@ -3368,12 +3371,16 @@ static bool qio_discard_updates_metadata(struct qcow2 *qcow2, struct qio *qio,
>>    *
>>    * Discard covering the whole cluster replaces the entry and unuses
>>    * the discarded (or COW source) cluster.
>> + * If the backing is present, set 'reads as zeroes' to avoid exposing
>> + * stale data.
>>    */
>>   static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   				   struct qcow2_map *map)
>>   {
>> +	bool zeroes = maybe_mapped_in_lower_delta(qcow2, *qio);
>>   	u32 index_in_page = map->l2.index_in_page;
>>   	struct md_page *md = map->l2.md;
>> +	u64 new_ext_l2 = map->ext_l2;
>>   	loff_t unuse_pos, unuse_end;
>>   	struct qio_ext *ext;
>>   	int ret;
>> @@ -3399,6 +3406,8 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   		unuse_pos = map->data_clu_pos;
>>   		unuse_end = map->data_clu_pos + qcow2->clu_size;
>>   	}
>> +	if (zeroes && qcow2->ext_l2)
>> +		new_ext_l2 = (u64)U32_MAX << 32;
>>   
>>   	ret = prepare_l_entry_replace(qcow2, map, *qio, md, index_in_page,
>>   				      unuse_pos, unuse_end, L2_LEVEL);
>> @@ -3408,6 +3417,10 @@ static int prepare_cluster_discard(struct qcow2 *qcow2, struct qio **qio,
>>   	ext = (*qio)->ext;
>>   	ext->lx_md = md;
>>   
>> +	ext->new_ext_l2 = new_ext_l2;
>> +	if (zeroes && !qcow2->ext_l2)
>> +		ext->set_all_zeroes = true;
>> +
>>   	(*qio)->flags |= QIO_IS_DISCARD_FL;
>>   	return 1;
>>   }
>> @@ -4050,6 +4063,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>   	struct qio *qio;
>>   	bool discard;
>>   	u32 arg_mask;
>> +	u64 entry;
>>   	int ret;
>>   
>>   	while (1) {
>> @@ -4074,11 +4088,15 @@ static void process_indexes_write(struct qcow2 *qcow2,
>>   			goto set_ext_l2;
>>   		}
>>   
>> +		entry = ext->allocated_clu_pos;
>> +		if (unlikely(ext->set_all_zeroes))
>> +			entry = L2_READS_ALL_ZEROES;
>> +
>>   		/* XXX: check prealloced_pos ==> revert */
>>   		ret = prepare_l_entry_update(qcow2, qio, lx_md,
>>   					     ext->lx_index_in_page,
>> -					     &ext->allocated_clu_pos,
>> -					     arg_mask, ext->lx_level);
>> +					     &entry, arg_mask,
>> +					     ext->lx_level);
>>   		if (ret < 0) {
>>   			qio->bi_status = errno_to_blk_status(ret);
>>   			qio_endio(qio);
>> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
>> index 8a24e04130e4d..0f006f1ae48cc 100644
>> --- a/drivers/md/dm-qcow2.h
>> +++ b/drivers/md/dm-qcow2.h
>> @@ -312,6 +312,7 @@ struct qio_ext {
>>   	u64 new_ext_l2;
>>   	u32 cow_mask;
>>   	bool only_set_ext_l2:1;
>> +	bool set_all_zeroes:1;
>>   
>>   	u8 lx_level;
>>   
>