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

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

[Alexander Ivanov]

Discard punches holes in place of freed clusters, but doesn't change
the file size. On postsuspend dm-qcow2 truncates the image file only
down to file_preallocated_area_start, so if clusters at the end of the
file are freed, 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 file is shrunk 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. Patches 3-5 fix existing cases where the refcounts or the
dirty bit on disk don't reflect real cluster usage, which the scan
relies on. Patch 6 does the actual truncation.

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

v7:
- Patch 5: track whether the image was opened dirty in a new opened_dirty
  flag instead of checking the parsed header, which is reread on resume
  and may carry the bit set by the kernel itself (e.g. on the lower image
  of a cancelled backward merge). A new kernel_dirty_bit flag tells
  whether the bit on disk is the kernel's own.
- Patch 6: use opened_dirty to skip the scan.

v6:
- Add Reviewed-by lines to patches 1 and 2.
- Patch 2: fix "pase" typo in a message touched by the patch.
- Patches 3-5: new patches.
- Patch 6: drop Reviewed-by, as the patch changed a lot.
- Patch 6: mark L1/L2 pages, whose failed writeback reverted refcounts
  of new clusters, with MD_LX_REVERTED and report it via the new
  lx_writeback_error flag, which skips the scan and is included in
  wants_check.
  Set md_writeback_error if relocate_refcount_table() fails
  after the new header is written, and clamp the scan start to
  reftable_max_file_size.
  Skip the scan for an image opened with the dirty bit set
  when kernel_sets_dirty_bit is enabled.
  Check the R1 entry before calc_r2_page_id(), so a corrupted
  one fails the scan without WARN.
  Reword the commit message to make clear that discard does
  free the clusters but doesn't shrink the file.

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 (6):
  dm-qcow2: add qcow2_md_page_read_sync() helper
  dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages
  dm-qcow2: extend the file in place_r2() before linking the refblock
  dm-qcow2: complete wbd before ending the data qio
  dm-qcow2: keep the dirty bit of an image opened dirty
  dm-qcow2: truncate unused tail of image on suspend

 drivers/md/dm-qcow2-cmd.c    |   4 +-
 drivers/md/dm-qcow2-map.c    | 109 ++++++++++++++++++++++++++++---
 drivers/md/dm-qcow2-target.c | 122 +++++++++++++++++++++++++++--------
 drivers/md/dm-qcow2.h        |  10 ++-
 4 files changed, 205 insertions(+), 40 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v7 1/6] 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 a later patch in the series.

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>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 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 v7 2/6] 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>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 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..b667adb5472a 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 parse 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 v7 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock

[Alexander Ivanov]

place_r2() links a new refblock into R1, but moves neither file_size
nor file_preallocated_area_start. The refblock stays at or above the
watermark if truncate_prealloc_safe() in allocate_cluster() fails
after place_r2(), or if a parallel mark_cluster_unused() lowers
free_cluster_search_pos before the second pass of
find_unused_cluster(). Truncation of preallocations on suspend then
cuts the file under the refblock: R1 on disk points past EOF, and
the next session gets -EIO on allocations in the range it covers.

Extend the file and raise the watermark to the end of the refblock
before linking it into R1, as allocate_cluster() does for data
clusters.

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>
---
 drivers/md/dm-qcow2-map.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..810dd22425ae 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -82,6 +82,7 @@ struct qcow2_bvec {
 static int qcow2_handle_r1r2_maps(struct qcow2 *qcow2, loff_t pos, struct qio **qio,
 	struct qcow2_map_item *r1, struct qcow2_map_item *r2, bool compressed);
 static int qcow2_punch_hole(struct file *file, loff_t pos, loff_t len);
+static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func);
 static void handle_cleanup_mask(struct qio *qio);
 static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
 			     struct qcow2_map *map);
@@ -2088,6 +2089,7 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
 		    struct qcow2_map_item *r2, loff_t r2_pos, struct qio **qio)
 {
 	struct dm_target *ti = qcow2->tgt->ti;
+	loff_t end = r2_pos + qcow2->clu_size;
 	u64 page_id = r2_pos >> PAGE_SHIFT;
 	int ret;
 
@@ -2100,6 +2102,12 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
 		return ret;
 	}
 
+	ret = truncate_prealloc_safe(qcow2, end, __func__);
+	if (ret)
+		return ret;
+	if (end > qcow2->file_preallocated_area_start)
+		qcow2->file_preallocated_area_start = end;
+
 	ret = qcow2_alloc_and_insert_md_page(qcow2, page_id, &r2->md);
 	if (ret < 0) {
 		QC_ERR(ti, "Can't alloc: ret=%d, page_id=%llu", ret, page_id);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v7 4/6] dm-qcow2: complete wbd before ending the data qio

[Alexander Ivanov]

data_rw_complete() calls qio_endio() before complete_wbd(). qio_endio()
drops the inflight ref, so qcow2_flush_deferred_activity() may return
on suspend while complete_wbd() for a failed L1/L2 writeback is still
running in AIO completion context. revert_l_entries_update() then
marks clusters unused and dirties R2 after the flush, and the change
is never written back.

Complete the wbd first. The md writeback path already does it in this
order.

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>
---
 drivers/md/dm-qcow2-map.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 810dd22425ae..ef832813b718 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2884,13 +2884,14 @@ static void data_rw_complete(struct qio *qio)
 		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
 	}
 
+	/* qio_endio() drops the inflight ref, which suspend waits for */
+	if (finalize_wbd)
+		complete_wbd(qcow2, wbd);
 	if (call_endio) {
 		if (bi_status != BLK_STS_OK)
 			qio->bi_status = bi_status;
 		qio_endio(qio);
 	}
-	if (finalize_wbd)
-		complete_wbd(qcow2, wbd);
 }
 
 static void perform_rw_mapped(struct qcow2_map *map, struct qio *qio)
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v7 5/6] dm-qcow2: keep the dirty bit of an image opened dirty

[Alexander Ivanov]

With kernel_sets_dirty_bit, postsuspend clears the dirty bit
unconditionally. An image that is dirty at open, e.g. after a crash,
has unrepaired refcounts, which may be below real usage, yet it is
marked clean on disk after the first suspend. A userspace check before
the next open then has no reason to repair it.

Clear the bit only if the image was clean before the kernel set it.
The parsed header can't tell this: it is reread on every preresume and
may carry the kernel's own bit, e.g. on the lower image of a backward
merge cancelled by suspend in waiting_completion or of a merge with
depth > 0. So track two flags per image: kernel_dirty_bit is set only
while the last header write was a successful set and the reread header
still has the bit, opened_dirty is set when the header has the bit and
kernel_dirty_bit is not set. Clearing the bit of an image opened dirty
is left to userspace check.

After a failed write the bit found on disk is taken as opened dirty,
which costs an extra check, rather than as the kernel's, which could
clear a bit set by userspace while the device was suspended.

This only preserves the on-disk marker. The driver still accepts a
dirty image read-write and allocates from its refcounts, so
find_unused_cluster() may hand out clusters that L2 still references
until the image is checked.

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>
---
 drivers/md/dm-qcow2-target.c | 15 +++++++++++++--
 drivers/md/dm-qcow2.h        |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index b667adb5472a..2d69e39a3530 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -659,6 +659,7 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
 	struct QCowHeader *raw_hdr;
 	struct md_page *md;
 	bool is_ro;
+	int ret;
 
 	if (qcow2->hdr.version ==  2)
 		return 0;
@@ -676,12 +677,15 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
 	if (kernel_sets_dirty_bit) {
 		if (dirty)
 			raw_hdr->incompatible_features |= dirty_mask;
-		else
+		else if (!qcow2->opened_dirty)
 			raw_hdr->incompatible_features &= ~dirty_mask;
 	}
 	kunmap(md->page);
 
-	return qcow2_rw_page_sync(WRITE, qcow2, md->id, md->page);
+	ret = qcow2_rw_page_sync(WRITE, qcow2, md->id, md->page);
+	if (kernel_sets_dirty_bit)
+		qcow2->kernel_dirty_bit = dirty && !ret;
+	return ret;
 }
 ALLOW_ERROR_INJECTION(qcow2_set_image_file_features, ERRNO);
 
@@ -903,6 +907,13 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 		goto out;
 
 	qcow2_calc_cached_parameters(qcow2, hdr);
+	/* The header is reread on resume and may carry our own dirty bit */
+	if (!(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
+		qcow2->opened_dirty = false;
+		qcow2->kernel_dirty_bit = false;
+	} else if (!qcow2->kernel_dirty_bit) {
+		qcow2->opened_dirty = true;
+	}
 	ret = -EOPNOTSUPP;
 	if (qcow2->clu_size < PAGE_SIZE ||
 	    (qcow2->ext_l2 && qcow2->clu_size < PAGE_SIZE * 32))
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 69d3ae6fca8a..a2d951ee5541 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -257,6 +257,8 @@ struct qcow2 {
 	u32 refblock_bits;
 	u32 refblock_entries;
 	bool ext_l2;
+	bool opened_dirty; /* Dirty bit was set before we set it */
+	bool kernel_dirty_bit; /* We set the dirty bit on disk */
 
 	bool pause_submitting_qios; /* This is used only on top qcow2 image */
 	bool backward_merge_in_process;
-- 
2.53.0

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

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

[Alexander Ivanov]

Discard punches holes in place of freed clusters, so they hold no
physical extents, but it doesn't change the file size. On postsuspend
we truncate the image file only down to file_preallocated_area_start,
i.e. we drop preallocations only. So if clusters at the end of the
file are freed (refcount dropped to zero), the file size stays the
same and the image 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. A failed relocation of the
reftable leaves file_preallocated_area_start beyond the space covered
by the reftable, so the scan starts at most from its end. If the
relocation fails after the new header is written, refcounts no longer
match the header, so set md_writeback_error there.

Truncate the file to the used end. Fall back to the old behaviour on
scan error and after any failed md writeback. A failed R1/R2 write
leaves refcounts on disk stale. A failed L1/L2 write reverts the
refcount of a newly allocated cluster to zero, while L1/L2 on disk may
still point to it. Mark such an L1/L2 page with MD_LX_REVERTED and
clear the mark on its next successful writeback, which rewrites the
whole page, e.g. when a request delayed on ENOSPC is resubmitted. On
md writeback flush a still marked page sets a new lx_writeback_error
flag, which is reported in get_errors and logged when the scan is
skipped. The flag is lost on target reload, after which the scan would
trust the reverted refcount, so include it in wants_check like
md_writeback_error: resume is refused until the image is checked and
the target reloaded.

Refcounts may also be below real usage after a crash, since both QEMU
with lazy_refcounts and this driver write L2 before refcounts. The scan
would then cut off clusters L2 still points to, even in a session
without guest writes. With kernel_sets_dirty_bit such an image has the
dirty bit set at open, and the disabled dirty bit check lets it be
opened RW, so skip the scan for it. Otherwise userspace sets the dirty
bit on every RW open, so the bit doesn't mean the image is
inconsistent. Userspace checks the image before opening it RW, which
find_unused_cluster() already relies on when it reuses clusters with
zero refcount.

Close and reload to RO go through postsuspend too. On remove,
__dm_destroy() calls the presuspend and postsuspend hooks if the live
table isn't suspended yet. It neither stops the queue nor waits for
I/O, but the device is idle there, because dm_lock_for_deletion()
refuses to remove an open device. On table swap, do_resume() suspends
the old (RW) table before dm_swap_table(). So the file is shrunk 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>
---
 drivers/md/dm-qcow2-cmd.c    |  4 +-
 drivers/md/dm-qcow2-map.c    | 96 +++++++++++++++++++++++++++++++++---
 drivers/md/dm-qcow2-target.c | 38 +++++++++++---
 drivers/md/dm-qcow2.h        |  6 ++-
 4 files changed, 127 insertions(+), 17 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index 2865940cff30..1520733c962c 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -20,9 +20,9 @@ static int qcow2_get_errors(struct qcow2_target *tgt, char *result,
 	int ret;
 
 	ret = DMEMIT("wants_check=%d\nmd_writeback_error=%d\ntruncate_error=%d\n"
-		     "merge_backward_error=%d\n",
+		     "merge_backward_error=%d\nlx_writeback_error=%d\n",
 		      wants_check, tgt->md_writeback_error, tgt->truncate_error,
-		      tgt->backward_merge.error);
+		      tgt->backward_merge.error, tgt->lx_writeback_error);
 
 	return ret ? 1 : 0;
 }
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index ef832813b718..860095a59f4f 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1091,6 +1091,11 @@ static int calc_refcounters_map(struct qcow2 *qcow2, loff_t pos,
 	return -EIO;
 }
 
+static bool r1_entry_is_valid(struct qcow2 *qcow2, u64 entry)
+{
+	return !(entry & R1_RESERVED_ZERO_MASK) && CLU_OFF(qcow2, entry) == 0;
+}
+
 static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
 						struct qcow2_map_item *r2)
 {
@@ -1098,8 +1103,7 @@ static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
 	u32 bits = qcow2->refblock_bits;
 	loff_t pos;
 
-	if (WARN_ON_ONCE((entry & R1_RESERVED_ZERO_MASK) ||
-			 (CLU_OFF(qcow2, entry) != 0)))
+	if (WARN_ON_ONCE(!r1_entry_is_valid(qcow2, entry)))
 		return -EIO;
 
 	/* The corresponding refcount block has not yet been allocated */
@@ -1363,6 +1367,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 		if (cleared)
 			continue; /* no cluster was allocated */
 
+		/* L1/L2 on disk may point to the cluster */
+		wbd->md->status |= MD_LX_REVERTED;
 		spin_unlock(&qcow2->md_pages_lock);
 		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
 
@@ -1386,6 +1392,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
 	lockdep_assert_held(&qcow2->md_pages_lock);
 
 	md->status &= ~(MD_WRITEBACK|MD_WRITEBACK_ERROR);
+	if (!ret)
+		md->status &= ~MD_LX_REVERTED;
 	list_splice_init(&md->wait_list, wait_list);
 	if (ret && !md->wbd) {
 		/*
@@ -2216,6 +2224,74 @@ 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. Failed relocate_refcount_table() leaves it beyond
+ * reftable_max_file_size, so clamp the start to the reftable coverage.
+ * 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 = min(qcow2->file_preallocated_area_start,
+		  qcow2->reftable_max_file_size);
+	pos = round_down(pos - 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;
+
+		/*
+		 * The scan walks R1 entries normal I/O may never touch,
+		 * so don't let calc_r2_page_id() WARN on a corrupted one.
+		 */
+		if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
+							r1.index_in_page)))
+			return -EIO;
+
+		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;
@@ -2387,21 +2463,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 		if (!WARN_ON_ONCE(!md))
 			qcow2_md_make_dirty(qcow2, md, true);
 		if (!md)
-			break; /* goto err_free_r2_pages */
+			break; /* goto err_hdr_written */
 	}
 	spin_unlock_irq(&qcow2->md_pages_lock);
 	if (i != nr_pages)
-		goto err_free_r2_pages;
+		goto err_hdr_written;
 
 	/* Connect new R2 to new R1 */
 	for (i = end; i < r2_end; i += clu_size) {
 		if (calc_refcounters_map(qcow2, i, &r1, &r2) < 0) {
 			WARN_ON_ONCE(1);
-			goto err_free_r2_pages;
+			goto err_hdr_written;
 		}
 		ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
 		if (WARN_ON_ONCE(ret <= 0))
-			goto err_free_r2_pages;
+			goto err_hdr_written;
 		spin_lock_irq(&qcow2->md_pages_lock);
 		set_u64_to_be_page(md->page, r1.index_in_page, i);
 		qcow2_md_make_dirty(qcow2, md, true);
@@ -2412,7 +2488,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	for (i = pos; i < r2_end; i += clu_size) {
 		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
 		if (WARN_ON_ONCE(ret <= 0))
-			goto err_free_r2_pages;
+			goto err_hdr_written;
 		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
 	}
 
@@ -2420,12 +2496,16 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	for (i = old_pos; i < old_end; i += clu_size) {
 		ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
 		if (WARN_ON_ONCE(ret <= 0))
-			goto err_free_r2_pages;
+			goto err_hdr_written;
 		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
 	}
 
 	return 1;
 
+err_hdr_written:
+	/* Header on disk points to new R1, refcounts may be inconsistent */
+	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
+	qcow2->tgt->md_writeback_error = true;
 err_free_r2_pages:
 	for (i = end; i < r2_end; i += clu_size) {
 		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 2d69e39a3530..7895fea531ae 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
 void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)
 {
 	struct rb_node *node;
+	bool wb_error = false;
 	struct md_page *md;
 	int i;
 
@@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
 	     node; node = rb_next(node)) {
 		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, "Failed to write dirty pages");
-			tgt->md_writeback_error = true;
-			break;
-		}
+		if (md->status & MD_WRITEBACK_ERROR)
+			wb_error = true;
+		if (md->status & MD_LX_REVERTED)
+			tgt->lx_writeback_error = true;
 	}
 	spin_unlock_irq(&qcow2->md_pages_lock);
+
+	if (wb_error) {
+		QC_ERR(tgt->ti, "Failed to write dirty pages");
+		tgt->md_writeback_error = true;
+	}
 }
 
 static void qcow2_flush_deferred_activity_all(struct qcow2_target *tgt)
@@ -1058,11 +1063,32 @@ 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 don't match L1/L2 on disk after failed md writeback.
+	 * They may also be below real usage in an image opened dirty,
+	 * e.g. after a crash. If userspace sets the dirty bit, it is set
+	 * on every RW open and userspace checks the image before that.
+	 */
+	if (tgt->md_writeback_error) {
+		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
+	} else if (tgt->lx_writeback_error) {
+		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
+	} else if (kernel_sets_dirty_bit && qcow2->opened_dirty) {
+		QC_INFO(ti, "Skip shrinking to used end of dirty image");
+	} else {
+		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 a2d951ee5541..7db2d9301cad 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -134,6 +134,7 @@ struct md_page {
 #define MD_DIRTY	(1U << 1) /* Page contains changes and wants writeback */
 #define MD_WRITEBACK	(1U << 2) /* Writeback was submitted */
 #define MD_WRITEBACK_ERROR (1U << 3) /* Last writeback failed with error */
+#define MD_LX_REVERTED	(1U << 4) /* L1/L2 on disk may point to freed cluster */
 	unsigned int status;
 	struct page *page;
 	struct list_head wait_list;
@@ -200,6 +201,7 @@ struct qcow2_target {
 	bool service_operations_allowed;
 	bool wants_suspend;
 	bool md_writeback_error;
+	bool lx_writeback_error;
 	bool truncate_error;
 	bool event_enospc;
 
@@ -413,6 +415,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);
@@ -461,7 +464,8 @@ static inline bool fake_service_qio(struct qio *qio)
 static inline bool qcow2_wants_check(struct qcow2_target *tgt)
 {
 
-	return !!(tgt->md_writeback_error|tgt->truncate_error);
+	return !!(tgt->md_writeback_error|tgt->lx_writeback_error|
+		  tgt->truncate_error);
 }
 
 static inline bool qcow2_file_is_writable(struct qcow2 *qcow2)
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 v7 2/6] 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/20261022145032.2128679-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 v7 1/6] 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 a later patch in the series.
> 
> 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/20261022145032.2128679-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 v7 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock

[no-reply]

> place_r2() links a new refblock into R1, but moves neither file_size
> nor file_preallocated_area_start. The refblock stays at or above the
> watermark if truncate_prealloc_safe() in allocate_cluster() fails
> after place_r2(), or if a parallel mark_cluster_unused() lowers
> free_cluster_search_pos before the second pass of
> find_unused_cluster(). Truncation of preallocations on suspend then
> cuts the file under the refblock: R1 on disk points past EOF, and
> the next session gets -EIO on allocations in the range it covers.
> 
> Extend the file and raise the watermark to the end of the refblock
> before linking it into R1, as allocate_cluster() does for data
> clusters.
> 
> 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/20261022145032.2128679-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 v7 4/6] dm-qcow2: complete wbd before ending the data qio

[no-reply]

> data_rw_complete() calls qio_endio() before complete_wbd(). qio_endio()
> drops the inflight ref, so qcow2_flush_deferred_activity() may return
> on suspend while complete_wbd() for a failed L1/L2 writeback is still
> running in AIO completion context. revert_l_entries_update() then
> marks clusters unused and dirties R2 after the flush, and the change
> is never written back.
> 
> Complete the wbd first. The md writeback path already does it in this
> order.
> 
> 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/20261022145032.2128679-1-alexander.ivanov@virtuozzo.com?part=4


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

Re: [Devel] [PATCH hci-8.1 v7 5/6] dm-qcow2: keep the dirty bit of an image opened dirty

[no-reply]

> With kernel_sets_dirty_bit, postsuspend clears the dirty bit
> unconditionally. An image that is dirty at open, e.g. after a crash,
> has unrepaired refcounts, which may be below real usage, yet it is
> marked clean on disk after the first suspend. A userspace check before
> the next open then has no reason to repair it.
> 
> Clear the bit only if the image was clean before the kernel set it.
> The parsed header can't tell this: it is reread on every preresume and
> may carry the kernel's own bit, e.g. on the lower image of a backward
> merge cancelled by suspend in waiting_completion or of a merge with
> depth > 0. So track two flags per image: kernel_dirty_bit is set only
> while the last header write was a successful set and the reread header
> still has the bit, opened_dirty is set when the header has the bit and
> kernel_dirty_bit is not set. Clearing the bit of an image opened dirty
> is left to userspace check.
> 
> After a failed write the bit found on disk is taken as opened dirty,
> which costs an extra check, rather than as the kernel's, which could
> clear a bit set by userspace while the device was suspended.
> 
> This only preserves the on-disk marker. The driver still accepts a
> dirty image read-write and allocates from its refcounts, so
> find_unused_cluster() may hand out clusters that L2 still references
> until the image is checked.
> 
> 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/20261022145032.2128679-1-alexander.ivanov@virtuozzo.com?part=5


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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: truncate-to-used-end trusts on-disk refcounts with no in-kernel guard by default
--

commit 7e932ded12fef03394ffccf5b1a73618dc7cd205
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 the refcount blocks backward and returns the end of the last cluster with a non-zero refcount. qcow2_truncate_preallocations() then truncates the image file to that point on postsuspend. The scan is skipped after md or L1/L2 writeback errors. It is also skipped for an image opened dirty, but only when kernel_sets_dirty_bit is set.

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)
> +{

[ ... ]

> +		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);
> +		}

[Severity: Medium]
The truncation point returned here comes directly from the on-disk
refcounts. Can those refcounts be below real usage after a crash, since
L2 is written before refcounts?

If so, qcow2_calc_used_end() returns an end that is too small. The caller
then truncates the file below clusters that L2 still references. That
would be silent data loss, and it could happen in a session with no guest
writes.

> 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

[ ... ]

> +	if (tgt->md_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
> +	} else if (tgt->lx_writeback_error) {
> +		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
> +	} else if (kernel_sets_dirty_bit && qcow2->opened_dirty) {
> +		QC_INFO(ti, "Skip shrinking to used end of dirty image");
> +	} else {
> +		used_end = qcow2_calc_used_end(qcow2);

[Severity: Medium]
The dirty-image guard in qcow2_truncate_preallocations() only applies when
kernel_sets_dirty_bit is true, and that parameter defaults to false. With
the default, qcow2_parse_metadata() still records qcow2->opened_dirty, but
nothing consults it.

The open-time dirty bit check in qcow2_check_convert_hdr() is commented
out:

    //	if (!is_ro && kernel_sets_dirty_bit !=
    //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
    //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;

So in the default configuration, can a crashed image that has not been
checked be opened RW? If it is, the postsuspend, remove or table swap path
would run qcow2_calc_used_end() and truncate the file.

The commit message says userspace checks the image before opening it RW.
Is that requirement documented or enforced anywhere in the repository?

find_unused_cluster() also trusts the refcounts. It only does so when a
guest write needs a cluster, though. This truncation runs earlier and can
cut off clusters that no write touches.

> +		if (used_end < 0)
> +			QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
> +		else
> +			end = used_end;
> +	}

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

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

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

[Alexander Ivanov]

>> +		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);
>> +		}
> [Severity: Medium]
> The truncation point returned here comes directly from the on-disk
> refcounts. Can those refcounts be below real usage after a crash, since
> L2 is written before refcounts?
>
> If so, qcow2_calc_used_end() returns an end that is too small. The caller
> then truncates the file below clusters that L2 still references. That
> would be silent data loss, and it could happen in a session with no guest
> writes.

This case is covered in the commit message (third paragraph).


>> +	if (tgt->md_writeback_error) {
>> +		QC_INFO(ti, "Skip shrinking to used end after md writeback error");
>> +	} else if (tgt->lx_writeback_error) {
>> +		QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
>> +	} else if (kernel_sets_dirty_bit && qcow2->opened_dirty) {
>> +		QC_INFO(ti, "Skip shrinking to used end of dirty image");
>> +	} else {
>> +		used_end = qcow2_calc_used_end(qcow2);
> [Severity: Medium]
> The dirty-image guard in qcow2_truncate_preallocations() only applies when
> kernel_sets_dirty_bit is true, and that parameter defaults to false. With
> the default, qcow2_parse_metadata() still records qcow2->opened_dirty, but
> nothing consults it.
>
> The open-time dirty bit check in qcow2_check_convert_hdr() is commented
> out:
>
>      //	if (!is_ro && kernel_sets_dirty_bit !=
>      //	    !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
>      //		return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;
>
> So in the default configuration, can a crashed image that has not been
> checked be opened RW? If it is, the postsuspend, remove or table swap path
> would run qcow2_calc_used_end() and truncate the file.
>
> The commit message says userspace checks the image before opening it RW.
> Is that requirement documented or enforced anywhere in the repository?
>
> find_unused_cluster() also trusts the refcounts. It only does so when a
> guest write needs a cluster, though. This truncation runs earlier and can
> cut off clusters that no write touches.
>
>> +		if (used_end < 0)
>> +			QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
>> +		else
>> +			end = used_end;
>> +	}
The guard is intentionally limited to kernel_sets_dirty_bit: in the
default mode userspace sets the dirty bit on every RW open, so
opened_dirty carries no information about consistency there.
The open-time check has been commented out since the driver
was introduced; this patch doesn't change that.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v7 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock

[Andrey Zhadchenko]

This patch definitely solves the problems but feels too hacky for me. I 
think it will be better to step back and rethink why 
find_unused_cluster() calls modification function (place_r2) while being 
named 'find'. Won't it be better if we can notify allocate_cluster() 
that we need new r2 and it will punch space + call place_r2 for us and 
then retry find_unused_cluster?
This way we will have all image size metadata bookkeeping in one place 
instead of having a secret second 'allocate refcount cluster if we run 
out of space'

On 10/22/26 16:50, Alexander Ivanov wrote:
> place_r2() links a new refblock into R1, but moves neither file_size
> nor file_preallocated_area_start. The refblock stays at or above the
> watermark if truncate_prealloc_safe() in allocate_cluster() fails
> after place_r2(), or if a parallel mark_cluster_unused() lowers
> free_cluster_search_pos before the second pass of
> find_unused_cluster(). Truncation of preallocations on suspend then
> cuts the file under the refblock: R1 on disk points past EOF, and
> the next session gets -EIO on allocations in the range it covers.
> 
> Extend the file and raise the watermark to the end of the refblock
> before linking it into R1, as allocate_cluster() does for data
> clusters.
> 
> 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>
> ---
>   drivers/md/dm-qcow2-map.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 78ee7cf5e881..810dd22425ae 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
> @@ -82,6 +82,7 @@ struct qcow2_bvec {
>   static int qcow2_handle_r1r2_maps(struct qcow2 *qcow2, loff_t pos, struct qio **qio,
>   	struct qcow2_map_item *r1, struct qcow2_map_item *r2, bool compressed);
>   static int qcow2_punch_hole(struct file *file, loff_t pos, loff_t len);
> +static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func);
>   static void handle_cleanup_mask(struct qio *qio);
>   static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
>   			     struct qcow2_map *map);
> @@ -2088,6 +2089,7 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
>   		    struct qcow2_map_item *r2, loff_t r2_pos, struct qio **qio)
>   {
>   	struct dm_target *ti = qcow2->tgt->ti;
> +	loff_t end = r2_pos + qcow2->clu_size;
>   	u64 page_id = r2_pos >> PAGE_SHIFT;
>   	int ret;
>   
> @@ -2100,6 +2102,12 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
>   		return ret;
>   	}
>   
> +	ret = truncate_prealloc_safe(qcow2, end, __func__);
> +	if (ret)
> +		return ret;
> +	if (end > qcow2->file_preallocated_area_start)
> +		qcow2->file_preallocated_area_start = end;
> +
>   	ret = qcow2_alloc_and_insert_md_page(qcow2, page_id, &r2->md);
>   	if (ret < 0) {
>   		QC_ERR(ti, "Can't alloc: ret=%d, page_id=%llu", ret, page_id);

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