From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Thu, 20 Aug 2026 12:21:43 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards In-Reply-To: References: <20260812202005.288994-1-andrey.zhadchenko@virtuozzo.com> <20260812202005.288994-6-andrey.zhadchenko@virtuozzo.com> <2f68ea52-3b22-40da-bc6e-dff8f101e815@virtuozzo.com> Message-ID: <8d814934-31bc-4d33-8b4d-283e127cb92b@virtuozzo.com> List-Id: On 8/20/26 11:04, Andrey Zhadchenko wrote: > > > 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 >> >> 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. Ok let's leave it as is then. But yeh from user perspective if I have device and I wrote some data to it at some offset and then I discard this data and write zeroes. And after that I miraculously see the old state of the device instead of zeroes, that can be really confusing. At least as far as I understand it. I guess if it's by design then that's ok. > > >> >> 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; >>> ? >> > -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.