Re: [Devel] [PATCH hci-8.1 v2 2/6] dm-qcow2: truncate unused tail of image on suspend

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: qcow2_calc_used_end() can return an end beyond file_size, so suspend grows the file
- [Low] dm-qcow2: duplicated prefix in used-end error message in qcow2_truncate_preallocations()
--

commit ce35554959d7d4a8d76e3913a469c5465e287b33
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-qcow2: truncate unused tail of image on suspend

This patch adds qcow2_calc_used_end(), which scans refcount blocks
backward from file_preallocated_area_start and returns the end of the
last cluster with a non-zero refcount. On postsuspend the image file is
then truncated to that end instead of only to
file_preallocated_area_start. The scan is skipped if md writeback
failed, and the old behaviour is used if the scan fails.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
> +{
> +	u32 clu_size = qcow2->clu_size;
> +	struct qcow2_map_item r1, r2;
> +	struct md_page *r1_md = NULL;
> +	loff_t pos;
> +	s32 i;
> +	int ret;
> +
> +	pos = round_down(qcow2->file_preallocated_area_start - 1, clu_size);

[ ... ]

> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
> +			if (get_r2_entry(qcow2, r2.md, i))
> +				return pos + clu_size;
> +		}

[Severity: Medium]
Can the value returned here exceed qcow2->file_size?

file_preallocated_area_start is only page aligned after
qcow2_parse_metadata(), not aligned to clu_size.  When clu_size is larger
than PAGE_SIZE and the file ends in the middle of a cluster, the first
cluster examined starts below file_preallocated_area_start but extends
past it.

If that cluster has a non-zero refcount, the function returns
pos + clu_size, which is greater than file_size.

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -1042,11 +1042,21 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)

[ ... ]

> +	/* Refcounts on disk are stale after failed md writeback */
> +	if (!tgt->md_writeback_error) {
> +		used_end = qcow2_calc_used_end(qcow2);
> +		if (used_end < 0)
> +			QC_ERR(ti, "dm-qcow2: Can't find used end: %lld", used_end);
> +		else
> +			end = used_end;
> +	}
> +
>  	if (end == qcow2->file_size)
>  		return;
>  
>  	ret = qcow2_truncate_safe(qcow2->file, end);

[Severity: Medium]
Is the check against file_size sufficient here?

Only end == qcow2->file_size is tested, so with used_end greater than
file_size the code goes on to call qcow2_truncate_safe(), and ftruncate
extends the file.  qcow2->file_size and file_preallocated_area_start are
then set to the larger value, although this patch is meant only to shrink
the image.

The growth is at most one cluster, but would it make sense to clamp with
min(used_end, qcow2->file_size), or to use end >= qcow2->file_size for the
early return?

[Severity: Low]
This isn't a bug, but QC_ERR already adds the "dm-qcow2: " prefix through
QCOW2_FMT, so this message prints the prefix twice:

    dm-qcow2: <dev>: dm-qcow2: Can't find used end: -5

The raw negative errno is also printed with %lld.  Neighbouring messages in
the same function repeat the prefix as well.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008073837.510678-1-alexander.ivanov@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel