[Devel] [PATCH VZ10 v2 05/10] drivers/md/dm-qcow2: set L2_READS_ALL_ZEROES after some discards
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Thu Aug 20 13:21:43 MSK 2026
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 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.
>
> 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.
More information about the Devel
mailing list