[Devel] [PATCH hci-8.1 v5 0/3] dm-qcow2: truncate unused tail of image on suspend

[Devel] [PATCH hci-8.1 v5 0/3] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

On postsuspend dm-qcow2 truncates the image file only down to
file_preallocated_area_start, so clusters at the end of the file freed
by discard are never given back and the file never shrinks.

This series makes postsuspend scan the refcount blocks backward and
truncate the file to the end of the last used cluster. Close and
reload to RO go through postsuspend too, so the tail is given back in
these cases as well.

Patch 1 adds qcow2_md_page_read_sync() helper, which is needed by the
scan. Patch 2 is a small cleanup of duplicate prefixes in QC_ERR/QC_INFO
messages. Patch 3 does the actual truncation.

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

v5:
- Add Reviewed-by line.

v4:
- Patch 1: document in the comment that the caller must ensure no read
  of the page is in flight.
- Patch 3: say in the commit message that close and reload to RO go
  through postsuspend too.

v3:
- Split from the common series with dm-ploop: dm-ploop patches are
  sent as a separate series.
- Patch 1: rename qcow2_get_md_page_sync() to qcow2_md_page_read_sync(),
  reword commit message and comment.
- Patch 2: new patch.
- Patch 3: start the scan from file_preallocated_area_start instead of
  the end of the file and clamp the result to it, as the last cluster
  may be partial; print the scan error with %pe and drop the duplicate
  "dm-qcow2:" prefix from it.

v2:
- Add dm-ploop support (patches 3-6), as requested in v1 review.
- dm-qcow2: move get_md_page_sync() to a separate patch as
  qcow2_md_page_read_sync() and use it in qcow2_parse_header() as well.
- dm-qcow2: start the refcount scan below file_preallocated_area_start
  instead of the end of the file, and truncate to the used end directly
  instead of min(file_preallocated_area_start, used end).

Alexander Ivanov (3):
  dm-qcow2: add qcow2_md_page_read_sync() helper
  dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages
  dm-qcow2: truncate unused tail of image on suspend

 drivers/md/dm-qcow2-map.c    | 56 ++++++++++++++++++++++++
 drivers/md/dm-qcow2-target.c | 83 +++++++++++++++++++++++++++---------
 drivers/md/dm-qcow2.h        |  3 ++
 3 files changed, 121 insertions(+), 21 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v5 1/3] dm-qcow2: add qcow2_md_page_read_sync() helper

[Alexander Ivanov]

Add a helper, which returns an up to date md page from md cache,
or allocates a new one, inserts it and reads it synchronously.
On read failure the page is erased from cache and freed.

Use it in qcow2_parse_header() instead of open-coded allocation
and read of page 0. It will also be used by the next patch.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-target.c | 41 +++++++++++++++++++++++++++++++-----
 drivers/md/dm-qcow2.h        |  2 ++
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index be09615eb8e2..311a23d63a5c 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -418,6 +418,41 @@ int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index, struct md_pag
 }
 ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO);
 
+/*
+ * Returns up to date md page from cache, or allocates
+ * a new one and reads it synchronously.
+ *
+ * The caller must ensure no read of this page is in flight
+ * (e.g., the device is suspended): if the page is cached but
+ * not MD_UPTODATE yet, qcow2_md_page_find_or_postpone() returns
+ * NULL and the following insert fails.
+ */
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id, struct md_page **md)
+{
+	int ret;
+
+	spin_lock_irq(&qcow2->md_pages_lock);
+	*md = qcow2_md_page_find_or_postpone(qcow2, id, NULL);
+	spin_unlock_irq(&qcow2->md_pages_lock);
+	if (*md)
+		return 0;
+
+	ret = qcow2_alloc_and_insert_md_page(qcow2, id, md);
+	if (ret)
+		return ret;
+
+	ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);
+	spin_lock_irq(&qcow2->md_pages_lock);
+	if (ret)
+		qcow2_md_page_erase(qcow2, *md);
+	else
+		(*md)->status |= MD_UPTODATE;
+	spin_unlock_irq(&qcow2->md_pages_lock);
+	if (ret)
+		qcow2_free_md_page(*md);
+	return ret;
+}
+
 static void inflight_ref_exit0(struct percpu_ref *ref)
 {
 	struct qcow2_target *tgt = container_of(ref, struct qcow2_target,
@@ -851,13 +886,9 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 	}
 	qcow2->file_preallocated_area_start = qcow2->file_size;
 
-	ret = qcow2_alloc_and_insert_md_page(qcow2, 0, &md);
-	if (ret)
-		return ret;
-	ret = qcow2_rw_page_sync(READ, qcow2, md->id, md->page);
+	ret = qcow2_md_page_read_sync(qcow2, 0, &md);
 	if (ret)
 		return ret;
-	md->status |= MD_UPTODATE;
 
 	raw_hdr = kmap(md->page);
 	min_len = to_bytes(ti->len);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index d9b8c38e093f..69d3ae6fca8a 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -386,6 +386,8 @@ void do_qcow2_work(struct work_struct *ws);
 void do_qcow2_fsync_work(struct work_struct *ws);
 int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index,
 					struct md_page **md);
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id,
+			    struct md_page **md);
 struct md_page *qcow2_md_page_find_or_postpone(struct qcow2 *qcow2,
 					       u64 id, struct qio **qio);
 struct md_page *qcow2_md_page_renumber(struct qcow2 *qcow2, u64 id,
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v5 2/3] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages

[Alexander Ivanov]

QCOW2_FMT already adds the "dm-qcow2: <dev>: " prefix and the trailing
newline, so several messages were printed like:

    dm-qcow2: <dev>: dm-qcow2: Can't parse metadata

and some of them were followed by an empty line. Drop the redundant
prefix and newlines.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-target.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 311a23d63a5c..bb11b8a74598 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -192,7 +192,7 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
 		md = rb_entry(node, struct md_page, node);
 		/* FIXME: call md_make_dirty() and try once again? */
 		if (md->status & MD_WRITEBACK_ERROR) {
-			QC_ERR(tgt->ti, "qcow2: Failed to write dirty pages");
+			QC_ERR(tgt->ti, "Failed to write dirty pages");
 			tgt->md_writeback_error = true;
 			break;
 		}
@@ -834,19 +834,19 @@ static int qcow2_parse_extensions(struct dm_target *ti, struct qcow2 *qcow2,
 			struct Qcow2BitmapHeaderExt bitmaps_ext;
 
 			if (ext.len != sizeof(bitmaps_ext)) {
-				QC_ERR(ti, "unexpected bitmap extension size\n");
+				QC_ERR(ti, "unexpected bitmap extension size");
 				return -EINVAL;
 			}
 
 			if (offset + sizeof(bitmaps_ext) > end) {
-				QC_ERR(ti, "unexpected end of extension area\n");
+				QC_ERR(ti, "unexpected end of extension area");
 				return -EINVAL;
 			}
 
 			memcpy(&bitmaps_ext, addr + offset, sizeof(bitmaps_ext));
 			ret = qcow2_parse_bitmap_ext(ti, qcow2, &bitmaps_ext);
 			if (ret) {
-				QC_ERR(ti, "unable to pase bitmap extension\n");
+				QC_ERR(ti, "unable to pase bitmap extension");
 				return ret;
 			}
 			break;
@@ -879,7 +879,7 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 		new_size = PAGE_ALIGN(qcow2->file_size);
 		ret = qcow2_truncate_safe(file, new_size);
 		if (ret) {
-			QC_ERR(ti, "qcow2: Can't truncate file");
+			QC_ERR(ti, "Can't truncate file");
 			return ret;
 		} /* See md_page_read_complete() */
 		qcow2->file_size = new_size;
@@ -910,14 +910,14 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 	ret = -EXDEV;
 	if (upper) {
 		if (upper->clu_size != qcow2->clu_size) {
-			QC_ERR(ti, "dm-qcow2: clu_size mismatch\n");
+			QC_ERR(ti, "clu_size mismatch");
 			goto out;
 		}
 		if (upper->ext_l2 != qcow2->ext_l2) {
 			if (is_bottom && upper->ext_l2 && !qcow2->ext_l2) {
-				QC_INFO(ti, "dm-qcow2: using bottom delta without ext_l2\n");
+				QC_INFO(ti, "using bottom delta without ext_l2");
 			} else {
-				QC_ERR(ti, "dm-qcow2: upper delta without ext_l2 is unsupported\n");
+				QC_ERR(ti, "upper delta without ext_l2 is unsupported");
 				goto out; /* This is not supported yet */
 			}
 		}
@@ -939,11 +939,11 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 		ext_end = PAGE_SIZE;
 
 	if (ext_end > PAGE_SIZE) {
-		QC_INFO(ti, "extensions are too long, skipping them\n");
+		QC_INFO(ti, "extensions are too long, skipping them");
 	} else {
 		ret = qcow2_parse_extensions(ti, qcow2, ext_start, ext_end, raw_hdr);
 		if (ret) {
-			QC_ERR(ti, "failed to parse extensions\n");
+			QC_ERR(ti, "failed to parse extensions");
 			goto out;
 		}
 	}
@@ -976,7 +976,7 @@ static int qcow2_parse_metadata(struct dm_target *ti, struct qcow2_target *tgt)
 	ret = 0;
 out:
 	if (ret)
-		QC_ERR(ti, "dm-qcow2: Can't parse metadata");
+		QC_ERR(ti, "Can't parse metadata");
 	return ret;
 }
 
@@ -1057,7 +1057,7 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
 
 	ret = qcow2_truncate_safe(qcow2->file, end);
 	if (ret) {
-		QC_ERR(ti, "dm-qcow2: Can't truncate preallocations");
+		QC_ERR(ti, "Can't truncate preallocations");
 		tgt->truncate_error = true;
 		return;
 	}
@@ -1143,7 +1143,7 @@ static void qcow2_postsuspend(struct dm_target *ti)
 	if (dm_table_get_mode(ti->table) & FMODE_WRITE) {
 		ret = qcow2_set_image_file_features(qcow2, false);
 		if (ret)
-			QC_ERR(ti, "qcow2: Can't set features");
+			QC_ERR(ti, "Can't set features");
 	}
 }
 static int qcow2_preresume(struct dm_target *ti)
@@ -1152,7 +1152,7 @@ static int qcow2_preresume(struct dm_target *ti)
 	int ret = 0;
 
 	if (qcow2_wants_check(tgt)) {
-		QC_ERR(ti, "qcow2: image check and target reload are required");
+		QC_ERR(ti, "image check and target reload are required");
 		return -EIO;
 	}
 
@@ -1173,7 +1173,7 @@ static int qcow2_preresume(struct dm_target *ti)
 	if (dm_table_get_mode(ti->table) & FMODE_WRITE) {
 		ret = qcow2_set_image_file_features(tgt->top, true);
 		if (ret)
-			QC_ERR(ti, "qcow2: Can't set features");
+			QC_ERR(ti, "Can't set features");
 	}
 	if (!ret)
 		qcow2_set_wants_suspend(ti, false);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

On postsuspend we truncate the image file only down to
file_preallocated_area_start, i.e. we drop preallocations only.
Clusters at the end of the file freed by discard (refcount dropped
to zero) are never given back, so the file size never shrinks.

Add qcow2_calc_used_end(), which scans refcount blocks backward from
file_preallocated_area_start and returns the end of the last cluster
with non-zero refcount. Refcount ranges with no allocated refblock
are skipped at once. Metadata pages are taken from md cache or read
synchronously. This is called on suspended device after md writeback,
so R1 and R2 can't change under us.

Truncate the file to the used end. Skip the scan if md writeback
failed, as refcounts on disk are stale then, and fall back to the old
behaviour on scan error.

Close and reload to RO go through postsuspend too: on remove,
__dm_destroy() suspends the live table if it isn't suspended yet,
and on table swap do_resume() suspends the old (RW) table before
dm_swap_table(). So the tail is given back in these cases as well.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-qcow2-map.c    | 56 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-qcow2-target.c | 12 +++++++-
 drivers/md/dm-qcow2.h        |  1 +
 3 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..65ee6ae144bf 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2208,6 +2208,62 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
 	return do_ftruncate(file, new_len, 0);
 }
 
+/*
+ * Returns the end of the last cluster, which is used according
+ * to refcount table, clamped to file_preallocated_area_start since
+ * the last cluster may be partial. Clusters freed by discard may
+ * leave the tail of file unused. Everything at or above
+ * file_preallocated_area_start is unused by construction, so the scan
+ * starts below it. This must be called on suspended device after md
+ * writeback, so nobody changes R1 and R2.
+ */
+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);
+	while (pos >= 0) {
+		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
+			return -EIO;
+
+		if (!r1_md || r1_md->id != r1.page_id) {
+			ret = qcow2_md_page_read_sync(qcow2, r1.page_id, &r1_md);
+			if (ret)
+				return ret;
+		}
+		r1.md = r1_md;
+
+		ret = calc_r2_page_id(qcow2, &r1, &r2);
+		if (ret == -ENOENT) {
+			/* No R2 cluster, so all clusters it covers are unused */
+			pos = (loff_t)r1.index * qcow2->refblock_entries * clu_size;
+			pos -= clu_size;
+			continue;
+		}
+
+		if (ret < 0)
+			return ret;
+
+		ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md);
+		if (ret)
+			return ret;
+
+		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
+			/* Last cluster is partial if size isn't cluster-aligned */
+			if (get_r2_entry(qcow2, r2.md, i))
+				return min_t(loff_t, pos + clu_size,
+					     qcow2->file_preallocated_area_start);
+		}
+	}
+
+	return -EIO;
+}
+
 static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func)
 {
 	loff_t prealloc_len, max_prealloc_len = qcow2->reftable_max_file_size;
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index bb11b8a74598..8555494a0c1c 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -1047,11 +1047,21 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
 {
 	struct qcow2_target *tgt = to_qcow2_target(ti);
 	struct qcow2 *qcow2 = top_qcow2_protected(ti);
-	loff_t end = qcow2->file_preallocated_area_start;
+	loff_t used_end, end = qcow2->file_preallocated_area_start;
 	int ret;
 
 	if (!(dm_table_get_mode(ti->table) & FMODE_WRITE))
 		return;
+
+	/* 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, "Can't find used end: %pe", ERR_PTR(used_end));
+		else
+			end = used_end;
+	}
+
 	if (end == qcow2->file_size)
 		return;
 
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 69d3ae6fca8a..73d3cf175bbf 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -411,6 +411,7 @@ void qcow2_ref_dec(struct qcow2_target *tgt, u8 ref_index);
 int qcow2_inflight_ref_switch(struct qcow2_target *tgt);
 void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2);
 int qcow2_truncate_safe(struct file *file, loff_t new_len);
+loff_t qcow2_calc_used_end(struct qcow2 *qcow2);
 
 void qcow2_merge_backward_work(struct work_struct *work);
 void qcow2_merge_backward_cancel(struct qcow2_target *tgt);
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 v5 2/3] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages

[no-reply]

> QCOW2_FMT already adds the "dm-qcow2: <dev>: " prefix and the trailing
> newline, so several messages were printed like:
> 
>     dm-qcow2: <dev>: dm-qcow2: Can't parse metadata
> 
> and some of them were followed by an empty line. Drop the redundant
> prefix and newlines.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


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

Re: [Devel] [PATCH hci-8.1 v5 1/3] dm-qcow2: add qcow2_md_page_read_sync() helper

[no-reply]

> Add a helper, which returns an up to date md page from md cache,
> or allocates a new one, inserts it and reads it synchronously.
> On read failure the page is erased from cache and freed.
> 
> Use it in qcow2_parse_header() instead of open-coded allocation
> and read of page 0. It will also be used by the next patch.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


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

Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[no-reply]

> On postsuspend we truncate the image file only down to
> file_preallocated_area_start, i.e. we drop preallocations only.
> Clusters at the end of the file freed by discard (refcount dropped
> to zero) are never given back, so the file size never shrinks.
> 
> Add qcow2_calc_used_end(), which scans refcount blocks backward from
> file_preallocated_area_start and returns the end of the last cluster
> with non-zero refcount. Refcount ranges with no allocated refblock
> are skipped at once. Metadata pages are taken from md cache or read
> synchronously. This is called on suspended device after md writeback,
> so R1 and R2 can't change under us.
> 
> Truncate the file to the used end. Skip the scan if md writeback
> failed, as refcounts on disk are stale then, and fall back to the old
> behaviour on scan error.
> 
> Close and reload to RO go through postsuspend too: on remove,
> __dm_destroy() suspends the live table if it isn't suspended yet,
> and on table swap do_resume() suspends the old (RW) table before
> dm_swap_table(). So the tail is given back in these cases as well.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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


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

Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Andrey Zhadchenko]

On 10/10/26 14:46, Alexander Ivanov wrote:
> On postsuspend we truncate the image file only down to
> file_preallocated_area_start, i.e. we drop preallocations only.
> Clusters at the end of the file freed by discard (refcount dropped
> to zero) are never given back, so the file size never shrinks.

Maybe somehow reword this first paragraph to be more specific? From the 
first reading looks like discard doesn't work correctly because it 
'never gives back clusters' - which is untrue, they are punched and do 
not hold any physical extents afterwards. What discard doesn't do is 
truncate the file end down to used offset which is another problem that 
this patch tries to solve.

> 
> Add qcow2_calc_used_end(), which scans refcount blocks backward from
> file_preallocated_area_start and returns the end of the last cluster
> with non-zero refcount. Refcount ranges with no allocated refblock
> are skipped at once. Metadata pages are taken from md cache or read
> synchronously. This is called on suspended device after md writeback,
> so R1 and R2 can't change under us.
> 
> Truncate the file to the used end. Skip the scan if md writeback
> failed, as refcounts on disk are stale then, and fall back to the old
> behaviour on scan error.
> 
> Close and reload to RO go through postsuspend too: on remove,
> __dm_destroy() suspends the live table if it isn't suspended yet,
> and on table swap do_resume() suspends the old (RW) table before
> dm_swap_table(). So the tail is given back in these cases as well.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files driver

Generally speaking I would prefer to avoid this code. Shrinking file end 
up to used data mostly looks good. The problem is image correctness: the 
cluster may be present in L2 pages but absent from refcount. After each 
umount we call qemu-img check anyway which properly fixes all this cases 
as it rebuilds refcount structure. And it also shrinks file end if it can.
We can of course assume that every inserted image is good (e.g. we call 
qemu-img check beforehand) and we may never corrupt qcow2 image in 
dm-qcow2. Then we can 100% rely on refcounts.
But I do not see the need to do it here if qemu-img will do that anyway 
and in safer manner.


> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>   drivers/md/dm-qcow2-map.c    | 56 ++++++++++++++++++++++++++++++++++++
>   drivers/md/dm-qcow2-target.c | 12 +++++++-
>   drivers/md/dm-qcow2.h        |  1 +
>   3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 78ee7cf5e881..65ee6ae144bf 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -2208,6 +2208,62 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
>   	return do_ftruncate(file, new_len, 0);
>   }
>   
> +/*
> + * Returns the end of the last cluster, which is used according
> + * to refcount table, clamped to file_preallocated_area_start since
> + * the last cluster may be partial. Clusters freed by discard may
> + * leave the tail of file unused. Everything at or above
> + * file_preallocated_area_start is unused by construction, so the scan
> + * starts below it. This must be called on suspended device after md
> + * writeback, so nobody changes R1 and R2.
> + */
> +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);
> +	while (pos >= 0) {
> +		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
> +			return -EIO;
> +
> +		if (!r1_md || r1_md->id != r1.page_id) {
> +			ret = qcow2_md_page_read_sync(qcow2, r1.page_id, &r1_md);
> +			if (ret)
> +				return ret;
> +		}
> +		r1.md = r1_md;
> +
> +		ret = calc_r2_page_id(qcow2, &r1, &r2);
> +		if (ret == -ENOENT) {
> +			/* No R2 cluster, so all clusters it covers are unused */
> +			pos = (loff_t)r1.index * qcow2->refblock_entries * clu_size;
> +			pos -= clu_size;
> +			continue;
> +		}
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md);
> +		if (ret)
> +			return ret;
> +
> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
> +			/* Last cluster is partial if size isn't cluster-aligned */
> +			if (get_r2_entry(qcow2, r2.md, i))
> +				return min_t(loff_t, pos + clu_size,
> +					     qcow2->file_preallocated_area_start);
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
>   static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func)
>   {
>   	loff_t prealloc_len, max_prealloc_len = qcow2->reftable_max_file_size;
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index bb11b8a74598..8555494a0c1c 100644
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -1047,11 +1047,21 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
>   {
>   	struct qcow2_target *tgt = to_qcow2_target(ti);
>   	struct qcow2 *qcow2 = top_qcow2_protected(ti);
> -	loff_t end = qcow2->file_preallocated_area_start;
> +	loff_t used_end, end = qcow2->file_preallocated_area_start;
>   	int ret;
>   
>   	if (!(dm_table_get_mode(ti->table) & FMODE_WRITE))
>   		return;
> +
> +	/* 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, "Can't find used end: %pe", ERR_PTR(used_end));
> +		else
> +			end = used_end;
> +	}
> +
>   	if (end == qcow2->file_size)
>   		return;
>   
> diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
> index 69d3ae6fca8a..73d3cf175bbf 100644
> --- a/drivers/md/dm-qcow2.h
> +++ b/drivers/md/dm-qcow2.h
> @@ -411,6 +411,7 @@ void qcow2_ref_dec(struct qcow2_target *tgt, u8 ref_index);
>   int qcow2_inflight_ref_switch(struct qcow2_target *tgt);
>   void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2);
>   int qcow2_truncate_safe(struct file *file, loff_t new_len);
> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2);
>   
>   void qcow2_merge_backward_work(struct work_struct *work);
>   void qcow2_merge_backward_cancel(struct qcow2_target *tgt);

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

Re: [Devel] [PATCH hci-8.1 v5 0/3] dm-qcow2: truncate unused tail of image on suspend

[Andrey Zhadchenko]

Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

On 10/10/26 14:46, Alexander Ivanov wrote:
> On postsuspend dm-qcow2 truncates the image file only down to
> file_preallocated_area_start, so clusters at the end of the file freed
> by discard are never given back and the file never shrinks.
> 
> This series makes postsuspend scan the refcount blocks backward and
> truncate the file to the end of the last used cluster. Close and
> reload to RO go through postsuspend too, so the tail is given back in
> these cases as well.
> 
> Patch 1 adds qcow2_md_page_read_sync() helper, which is needed by the
> scan. Patch 2 is a small cleanup of duplicate prefixes in QC_ERR/QC_INFO
> messages. Patch 3 does the actual truncation.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> 
> v5:
> - Add Reviewed-by line.
> 
> v4:
> - Patch 1: document in the comment that the caller must ensure no read
>    of the page is in flight.
> - Patch 3: say in the commit message that close and reload to RO go
>    through postsuspend too.
> 
> v3:
> - Split from the common series with dm-ploop: dm-ploop patches are
>    sent as a separate series.
> - Patch 1: rename qcow2_get_md_page_sync() to qcow2_md_page_read_sync(),
>    reword commit message and comment.
> - Patch 2: new patch.
> - Patch 3: start the scan from file_preallocated_area_start instead of
>    the end of the file and clamp the result to it, as the last cluster
>    may be partial; print the scan error with %pe and drop the duplicate
>    "dm-qcow2:" prefix from it.
> 
> v2:
> - Add dm-ploop support (patches 3-6), as requested in v1 review.
> - dm-qcow2: move get_md_page_sync() to a separate patch as
>    qcow2_md_page_read_sync() and use it in qcow2_parse_header() as well.
> - dm-qcow2: start the refcount scan below file_preallocated_area_start
>    instead of the end of the file, and truncate to the used end directly
>    instead of min(file_preallocated_area_start, used end).
> 
> Alexander Ivanov (3):
>    dm-qcow2: add qcow2_md_page_read_sync() helper
>    dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages
>    dm-qcow2: truncate unused tail of image on suspend
> 
>   drivers/md/dm-qcow2-map.c    | 56 ++++++++++++++++++++++++
>   drivers/md/dm-qcow2-target.c | 83 +++++++++++++++++++++++++++---------
>   drivers/md/dm-qcow2.h        |  3 ++
>   3 files changed, 121 insertions(+), 21 deletions(-)
> 

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

Re: [Devel] [PATCH hci-8.1 v5 0/3] dm-qcow2: truncate unused tail of image on suspend

[Konstantin Khorenko]

Whatever we decide about patch 3, the review turned up problems in
dm-qcow2 itself. They need fixing either way.

1. A failed L1/L2 write frees a cluster that L2 on disk still uses.

   If the L2 page is written but vfs_fsync() in
   complete_metadata_writeback() fails, complete_wbd() reverts the
   update: revert_l_entries_update() drops the refcount of the new
   cluster to zero. The page may already be on disk with the new
   entry. The allocator then hands the cluster out again, and two L2
   entries point to the same cluster.

   md_writeback_error is not set in this case (clear_writeback_status()
   flags R1/R2 pages only), so nothing asks for a check either.

   Fix: on a failed L1/L2 writeback, do not free the clusters. Leak
   them and set md_writeback_error, so preresume refuses to resume
   until "qemu-img check -r all" has run. A leak is safe, and the
   check repairs it. This is the "FIXME: We should reread md page on
   error" case.

2. Metadata writes give up on ENOSPC.

   Data writes that fail with ENOSPC are delayed and retried
   (qcow2_try_delay_enospc()). Metadata writeback is not:
   - R1/R2: md_writeback_error is set, and the device can't be resumed
     until it is reloaded;
   - L1/L2: the update is reverted (see 1), and the guest write fails.

   Fix: on ENOSPC, redirty the md page and retry it after
   ENOSPC_TIMEOUT_JI, the way data writes are retried. This is the
   "FIXME: call md_make_dirty() and try once again?" case.

3. place_r2() leaves a refblock above file_preallocated_area_start.

   place_r2() links a new refblock into R1, but moves neither the
   watermark nor file_size. Only allocate_cluster() raises the
   watermark, to the end of the cluster it returns after place_r2().
   The refblock stays above it if the file extend in
   allocate_cluster() fails, or if a parallel mark_cluster_unused()
   lowers free_cluster_search_pos.

   Today the postsuspend truncation cuts such a refblock only when
   file_size is above the watermark. With patch 3 it is cut whenever
   the clusters below it were discarded.

   Fix: place_r2() extends the file and raises the watermark to
   r2_pos + clu_size before it links the refblock into R1, and fails
   before R1 is dirtied.

If patch 3 is dropped, ploop needs one change too.
test_release_should_give_back_the_discarded_tail from ploop PR 486
expects an empty stderr on umount. Without the kernel truncation,
qemu-img check cuts the tail itself and prints "Repairing space leaked
at the end of the image", so the test has to expect that instead.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v5 1/3] dm-qcow2: add qcow2_md_page_read_sync() helper

[Konstantin Khorenko]

> + * The caller must ensure no read of this page is in flight
> + * (e.g., the device is suspended): if the page is cached but
> + * not MD_UPTODATE yet, qcow2_md_page_find_or_postpone() returns
> + * NULL and the following insert fails.

The requirement is stronger: nobody else may look this page up while
the helper runs. A qio that finds the page before MD_UPTODATE is set
gets queued on md->wait_list, which this helper never wakes. On the
error path the page is freed with that qio still on it. Neither caller
can hit this today, but the comment should say "no concurrent lookup"
rather than "no read in flight".

> +     ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);
> +     spin_lock_irq(&qcow2->md_pages_lock);
> +     if (ret)
> +             qcow2_md_page_erase(qcow2, *md);
> +     else
> +             (*md)->status |= MD_UPTODATE;
> +     spin_unlock_irq(&qcow2->md_pages_lock);
> +     if (ret)
> +             qcow2_free_md_page(*md);
> +     return ret;

On both error paths *md is left pointing to freed memory: here, and on
-EEXIST from qcow2_alloc_and_insert_md_page(). Please set *md = NULL
there.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v5 2/3] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages

[Konstantin Khorenko]

> -                             QC_ERR(ti, "unable to pase bitmap extension\n");
> +                             QC_ERR(ti, "unable to pase bitmap extension");

While you are on this line: "pase" -> "parse".

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Konstantin Khorenko]

I'd rather not truncate the image in the kernel at all. My comments
below are for the case we keep it.

The scan trusts the refcounts and never checks them against L1/L2. A
wrong refcount or a wrong scan start then costs data on the first
suspend: stop, snapshot or compact.

Without this patch such an image can still be repaired:
- ploop runs "qemu-img check -r all" on umount, compact and resize;
- our qemu-img (qcow2_check_extra_preallocation(), not in mainline)
  rebuilds the refcounts from L1/L2 and truncates the tail only if the
  image is clean;
- qcow2_preresume() rereads the metadata, so this works on a suspended
  device too.

This review alone found two ways the scan cuts live data, see below. I
don't think they are the last ones, so I suggest we keep the truncation
in userspace.

> + * leave the tail of file unused. Everything at or above
> + * file_preallocated_area_start is unused by construction, so the scan
> + * starts below it. This must be called on suspended device after md

This is not true, and the idea came from my v1 review.

place_r2() links a new refblock into R1, but moves neither
file_preallocated_area_start nor file_size. The refblock stays at or
above the watermark if:
- truncate_prealloc_safe() in allocate_cluster() fails after
  place_r2();
- a parallel mark_cluster_unused() lowers free_cluster_search_pos
  before the second pass of find_unused_cluster().

The scan then starts below the refblock and may truncate the file under
it. R1 on disk points past EOF, and the next session gets -EIO on
allocations in that range. In the first case the old code did nothing,
so this is a regression.

Please make place_r2() extend the file and raise the watermark to
r2_pos + clu_size before it links the refblock into R1. The old code
can cut such a refblock too, so this is a separate patch before this
one.

> +	pos = round_down(qcow2->file_preallocated_area_start - 1, clu_size);
> +	while (pos >= 0) {
> +		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
> +			return -EIO;

After a failed relocate_refcount_table(), file_preallocated_area_start
stays at r2_end, beyond reftable_max_file_size. The first pos is then
outside the reftable, and calc_refcounters_map() hits WARN_ONCE(), which
panics with panic_on_warn.

Please start from min(file_preallocated_area_start,
reftable_max_file_size).

> +	/* Refcounts on disk are stale after failed md writeback */
> +	if (!tgt->md_writeback_error) {

md_writeback_error is set for R1/R2 pages only. If an L2 write fails
but the page did reach the disk, revert_l_entries_update() drops the
refcount of the new cluster to zero, while L2 on disk still points to
it. If the cluster is at the tail, the scan cuts it.

Please skip the scan after any failed md writeback, L1/L2 included.

> on remove, __dm_destroy() suspends the live table if it isn't
> suspended yet

__dm_destroy() only calls the presuspend and postsuspend hooks: it does
not stop the queue or wait for I/O. The device is idle there only
because dm_lock_for_deletion() refuses to remove an open device. Please
say that in the commit message.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Denis V. Lunev]

On 10/21/26 18:42, Konstantin Khorenko wrote:
> I'd rather not truncate the image in the kernel at all. My comments
> below are for the case we keep it.
>
> The scan trusts the refcounts and never checks them against L1/L2. A
> wrong refcount or a wrong scan start then costs data on the first
> suspend: stop, snapshot or compact.
Wrong refcount already means data loss during ordinary cluster
allocation.

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

Re: [Devel] [PATCH hci-8.1 v5 3/3] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

> The scan trusts the refcounts and never checks them against L1/L2. A
> wrong refcount or a wrong scan start then costs data on the first
> suspend: stop, snapshot or compact.

By checking the refcounts, we only process the unused tail. If we were
to check the L1/L2 tables, we would have to process the entire image
from beginning to end.

I'll fix other issues.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel