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

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

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


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.