From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Tue, 25 Aug 2026 19:10:08 +0200 Subject: Re: [Devel] [PATCH VZ10 v3 06/11] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards In-Reply-To: <20260825122430.252094-7-andrey.zhadchenko@virtuozzo.com> References: <20260825122430.252094-1-andrey.zhadchenko@virtuozzo.com> <20260825122430.252094-7-andrey.zhadchenko@virtuozzo.com> Message-ID: <1573003d-4ee6-432c-a488-0cab4a1106eb@virtuozzo.com> List-Id: 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 > --- > 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; >