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

[Pavel Tikhomirov]

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

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.


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

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