Re: [Devel] [PATCH VZ10 6/6] drivers/md/dm-qcow2: seek unallocated L2 entries in one pass within md

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 8/12/26 14:42, Vasileios Almpanis wrote:
> 
> On 8/10/26 2:30 PM, Andrey Zhadchenko wrote:
>> When a cluster is unallocated but its L2 table exists, the seek code
>> walks it one cluster at a time: SEEK_DATA re-parses metadata for
>> every cluster of the range, and with a backing image each cluster
>> additionally costs a qio and completion allocation for the lower
>> delta descent.
>>
>> If the unmapped range reaches the cluster border, prolong it over the
>> following unallocated L2 entries: they are cached in the very md page
>> the current entry was parsed from, so this is just scanning for
>> non-zero words. Then the lower delta is scanned in one pass over the
>> whole range, and SEEK_DATA without a backing image skips it in one go.
>>
>> Entries with any subcluster or "reads as zeroes" bits set stop the
>> scan and spin parse_metadata() machinery.
>>
>> On a 16G image (128K clusters, extended L2) with all L2 tables
>> allocated but no clusters mapped over a backing image with data at
>> 15G, warm-cache SEEK_DATA improves from 44.8 ms to 0.4 ms.
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-139407
>> Feature: dm-qcow2: block device over QCOW2 files driver
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>> ---
>> ? drivers/md/dm-qcow2-map.c | 44 +++++++++++++++++++++++++++++++++++++--
>> ? 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> index 6a73c32689eca..3bda108db725f 100644
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
>> @@ -4441,6 +4441,34 @@ static inline void seek_qio_next_clu(struct qio 
>> *qio, struct qcow2_map *map)
>> ????? seek_qio_set_sector(qio, bi_sector);
>> ? }
>> +/*
>> + * Manually scan L2 metadata pages until we hit something interesting.
>> + * We do not take md_pages_lock here because we have no promises about
>> + * concurrent writes anyway.
>> + */
>> +static loff_t seek_extend_unmapped_end(struct qio *qio, struct 
>> qcow2_map *map,
>> +?????????????????????? loff_t end)
>> +{
>> +??? struct qcow2 *qcow2 = qio->qcow2;
>> +??? loff_t lim = SEEK_QIO_DATA(qio)->lim;
>> +??? u32 step = 1 + qcow2->ext_l2;
>> +??? u32 index = map->l2.index_in_page + step;
>> +??? u64 *entries;
>> +
>> +??? entries = kmap_local_page(map->l2.md->page);
>> +??? for (; index + step <= PAGE_SIZE / sizeof(u64) && end < lim;
>> +???????? index += step) {
>> +??????? /* Zero test does not need be64_to_cpu() */
>> +??????? if (READ_ONCE(entries[index]) ||
>> +??????????? (qcow2->ext_l2 && READ_ONCE(entries[index + 1])))
>> +??????????? break;
>> +??????? end += qcow2->clu_size;
>> +??? }
>> +??? kunmap_local(entries);
>> +
>> +??? return min_t(loff_t, end, lim);
>> +}
>> +
>> ? static struct qio *advance_and_spawn_lower_seek_qio(struct qio 
>> *old_qio, loff_t end)
>> ? {
>> ????? struct qcow2 *lower = old_qio->qcow2->lower;
>> @@ -4503,12 +4531,15 @@ static int qcow2_llseek_hole_qio(struct qio 
>> *qio, int whence, loff_t *result)
>> ????????????????? struct qio *new_qio;
>> ????????????????? loff_t end;
>> -??????????????? if (!(map.level & L2_LEVEL))
>> +??????????????? if (!(map.level & L2_LEVEL)) {
>> ????????????????????? end = min_t(loff_t,
>> ????????????????????????????? to_bytes(get_next_l2(qio)),
>> ????????????????????????????? SEEK_QIO_DATA(qio)->lim);
>> -??????????????? else
>> +??????????????? } else {
>> ????????????????????? end = to_bytes(qio->bi_iter.bi_sector) + size;
>> +??????????????????? if (!CLU_OFF(qio->qcow2, end))
>> +??????????????????????? end = seek_extend_unmapped_end(qio, &map, end);
>> +??????????????? }
>> ????????????????? new_qio = advance_and_spawn_lower_seek_qio(qio, end);
>> ????????????????? if (!new_qio) {
>> @@ -4552,6 +4583,15 @@ static int qcow2_llseek_hole_qio(struct qio 
>> *qio, int whence, loff_t *result)
>> ????????????????? qio->bi_iter.bi_sector += to_sector(size);
>> ????????????????? qio->bi_iter.bi_size -= size;
>> ????????????????? goto calc_subclu;
>> +??????????? } else if (arg.unmapped && (map.level & L2_LEVEL)) {
>> +??????????????? loff_t end = to_bytes(qio->bi_iter.bi_sector) + size;
>> +
>> +??????????????? /* Skip the following unallocated entries in one go */
>> +??????????????? if (!CLU_OFF(qio->qcow2, end)) {
>> +??????????????????? seek_qio_set_sector(qio,
> 
> shouldn't you wrap the result of seek_extend_unmapped_end in to_sector? 
> It returns bytes and seek_qio_set_sector takes sector_t and we would 
> immediately do to_bytes on something that is already bytes.

You are totally right. No idea how I missed that! Probably tests I 
drafted into ploop do not cover that case.

> 
>> +??????????????????????? seek_extend_unmapped_end(qio, &map, end));
>> +??????????????????? continue;
>> +??????????????? }
>> ????????????? }
>> ????????? }
>