Re: [Devel] [PATCH VZ10 v3 06/11] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards

[Konstantin Khorenko <khorenko@virtuozzo.com>]

prepare_cluster_discard() initializes new_ext_l2 = map->ext_l2 and
only overwrites it when a backing file is present.

On an ext_l2 image without backing, a whole-cluster discard therefore writes L2 entry = 0
while keeping the old subcluster allocation bits.

Readers treat alloc bits as "mapped" regardless of the offset,
so the next read of that cluster goes through perform_rw_mapped() with data_clu_pos == 0 and
returns the qcow2 header/L1 area to the guest;
a write corrupts the image header at file offset 0.

Patch 7 changes the initializer to 0. Suggest fixing patch 6 in place or squashing 6+7.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 8/25/26 14:24, 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>
> ---
>  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 a2e5c37adb7b9..390331554b7ac 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;
>  }
> @@ -4053,6 +4066,7 @@ static void process_indexes_write(struct qcow2 *qcow2,
>  	struct qio *qio;
>  	bool discard;
>  	u32 arg_mask;
> +	u64 entry;
>  	int ret;
>  
>  	while (1) {
> @@ -4076,11 +4090,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;
>