[Devel] [PATCH hci-8.1 v9 00/10] dm-qcow2: truncate unused tail of image on suspend

[Devel] [PATCH hci-8.1 v9 00/10] 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-9 fix existing cases where the refcounts or the
dirty bit on disk don't reflect real cluster usage, or where such an
image is not flagged for check, which the scan relies on. Patch 10 does
the actual truncation.

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

v9:
- Patch 5: new patch.
- Patch 6: also keep the dirty bit while wants_check is set, as
  md_writeback_error is kept only in the target and is lost on table
  reload or host restart. Rename kernel_dirty_bit to
  kernel_dirty_bit_set. Retitled accordingly.
- Patch 7: new patch.
- Patches 8, 9: split from patch 10: setting md_writeback_error on a
  late reftable relocation failure and the MD_LX_REVERTED /
  lx_writeback_error tracking are separate fixes now.
- Patch 10: rename qcow2_truncate_preallocations() to
  qcow2_truncate_unused_tail() and update the dm-ploop comment that
  refers to it. Move the rationale from code comments to the commit
  message.

v8:
- Patch 3: instead of extending the file in place_r2(), make
  find_unused_cluster() return the position for the new refblock and
  let allocate_cluster() prepare it the same way as a data cluster
  (punch, extend, watermark update moved into prepare_unused_cluster())
  before calling place_r2(). Retitled accordingly.

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 (10):
  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 for a new refblock in allocate_cluster()
  dm-qcow2: complete wbd before ending the data qio
  dm-qcow2: flush lower metadata before clearing its dirty bit on merge
    failure
  dm-qcow2: keep the dirty bit of an image that needs check
  dm-qcow2: fix md page cleanup on failed reftable relocation
  dm-qcow2: flag the image for check if reftable relocation fails late
  dm-qcow2: flag the image for check after a reverted L1/L2 writeback
  dm-qcow2: truncate unused tail of image on suspend

 drivers/md/dm-ploop-target.c |   2 +-
 drivers/md/dm-qcow2-cmd.c    |   5 +-
 drivers/md/dm-qcow2-map.c    | 180 +++++++++++++++++++++++++----------
 drivers/md/dm-qcow2-target.c | 119 +++++++++++++++++------
 drivers/md/dm-qcow2.h        |  10 +-
 5 files changed, 234 insertions(+), 82 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v9 01/10] 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 v9 02/10] 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 v9 03/10] dm-qcow2: extend the file for a new refblock in allocate_cluster()

[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.

Make find_unused_cluster() report a position not covered by R2
instead of placing the refblock there itself. allocate_cluster()
prepares that cluster the same way as a data cluster, with the punch,
extend and watermark update moved into prepare_unused_cluster(), and
calls place_r2() before searching again.

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 | 88 ++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 38 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..5952b5feb8bf 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2085,21 +2085,12 @@ static int parse_metadata(struct qcow2 *qcow2, struct qio **qio,
  * and connects it to R1 table entry.
  */
 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 qcow2_map_item *r2, loff_t r2_pos)
 {
 	struct dm_target *ti = qcow2->tgt->ti;
 	u64 page_id = r2_pos >> PAGE_SHIFT;
 	int ret;
 
-	if (delay_if_writeback(qcow2, r1->md, r1->index_in_page, qio, true))
-		return 0;
-
-	ret = qcow2_punch_hole(qcow2->file, r2_pos, qcow2->clu_size);
-	if (ret) {
-		QC_ERR(ti, "punch hole: %d", ret);
-		return ret;
-	}
-
 	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);
@@ -2115,7 +2106,7 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
 	spin_unlock_irq(&qcow2->md_pages_lock);
 
 	mark_cluster_used(qcow2, r2->md, r2->index_in_page);
-	return 1;
+	return 0;
 }
 
 static s32 find_unused_block_entry(struct qcow2 *qcow2, struct md_page *md,
@@ -2137,11 +2128,13 @@ static s32 find_unused_block_entry(struct qcow2 *qcow2, struct md_page *md,
 
 static loff_t find_unused_cluster(struct qcow2 *qcow2, struct qio **qio,
 				  struct qcow2_map_item *r1,
-				  struct qcow2_map_item *r2)
+				  struct qcow2_map_item *r2, bool *needs_r2)
 {
 	u32 clu_size = qcow2->clu_size;
 	s32 index, ret;
 	loff_t pos;
+
+	*needs_r2 = false;
 again:
 	pos = READ_ONCE(qcow2->free_cluster_search_pos);
 	if (pos >= qcow2->reftable_max_file_size)
@@ -2153,15 +2146,15 @@ static loff_t find_unused_cluster(struct qcow2 *qcow2, struct qio **qio,
 			return ret;
 		/*
 		 * Since pos is not covered by R2, the whole cluster
-		 * must be unused. Use it to store R2 cluster.
+		 * must be unused. The caller stores R2 cluster there.
 		 * Both indexes must be 0 here, because of we allocate
 		 * clusters from small to big.
 		 */
 		WARN_ON_ONCE(r2->index_in_page != 0 || r2->index != 0);
-		ret = place_r2(qcow2, r1, r2, pos, qio);
-		if (ret <= 0)
-			return ret;
-		goto again;
+		if (delay_if_writeback(qcow2, r1->md, r1->index_in_page, qio, true))
+			return 0;
+		*needs_r2 = true;
+		return pos;
 	}
 
 	spin_lock_irq(&qcow2->md_pages_lock);
@@ -2432,6 +2425,37 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	return -EIO;
 }
 
+/*
+ * Zero the unused cluster at @pos and make the file size and
+ * file_preallocated_area_start cover it.
+ */
+static int prepare_unused_cluster(struct qcow2 *qcow2, loff_t pos)
+{
+	loff_t off, end = pos + qcow2->clu_size;
+	loff_t old_size = qcow2->file_size;
+	int ret;
+
+	if (pos < qcow2->file_preallocated_area_start) {
+		/* Clu at @pos may contain dirty data */
+		off = min_t(loff_t, old_size, end);
+		ret = qcow2_punch_hole(qcow2->file, pos, off - pos);
+		if (ret) {
+			QC_ERR(qcow2->tgt->ti, "punch hole: %d", ret);
+			return ret;
+		}
+	}
+
+	if (end > old_size) {
+		ret = truncate_prealloc_safe(qcow2, end, __func__);
+		if (ret)
+			return ret;
+	}
+
+	if (end > qcow2->file_preallocated_area_start)
+		qcow2->file_preallocated_area_start = end;
+	return 0;
+}
+
 /*
  * This function is aimed to be called only from main work.
  * In case of wish to use it from more places, it's needed
@@ -2441,13 +2465,12 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 static loff_t allocate_cluster(struct qcow2 *qcow2, struct qio *qio,
 		      struct md_page **r2_md, u32 *r2_index_in_page)
 {
-	u32 clu_size = qcow2->clu_size;
-	struct file *file = qcow2->file;
-	loff_t pos, off, end, old_size;
 	struct qcow2_map_item r1, r2;
+	bool needs_r2;
+	loff_t pos;
 	int ret;
 again:
-	pos = find_unused_cluster(qcow2, &qio, &r1, &r2);
+	pos = find_unused_cluster(qcow2, &qio, &r1, &r2, &needs_r2);
 	if (unlikely(pos == -ENOENT)) {
 		ret = relocate_refcount_table(qcow2, &qio);
 		if (ret <= 0)
@@ -2457,28 +2480,17 @@ static loff_t allocate_cluster(struct qcow2 *qcow2, struct qio *qio,
 	if (pos <= 0)
 		return pos;
 
-	end = pos + clu_size;
-	old_size = qcow2->file_size;
-
-	if (pos < qcow2->file_preallocated_area_start) {
-		/* Clu at @pos may contain dirty data */
-		off = min_t(loff_t, old_size, end);
-		ret = qcow2_punch_hole(file, pos, off - pos);
-		if (ret) {
-			QC_ERR(qcow2->tgt->ti, "punch hole: %d", ret);
-			return ret;
-		}
-	}
+	ret = prepare_unused_cluster(qcow2, pos);
+	if (ret)
+		return ret;
 
-	if (end > old_size) {
-		ret = truncate_prealloc_safe(qcow2, end, __func__);
+	if (needs_r2) {
+		ret = place_r2(qcow2, &r1, &r2, pos);
 		if (ret)
 			return ret;
+		goto again;
 	}
 
-	if (end > qcow2->file_preallocated_area_start)
-		qcow2->file_preallocated_area_start = end;
-
 	mark_cluster_used(qcow2, r2.md, r2.index_in_page);
 	if (r2_md)
 		*r2_md = r2.md;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v9 04/10] 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 5952b5feb8bf..fa63bd6acfb8 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2888,13 +2888,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 v9 05/10] dm-qcow2: flush lower metadata before clearing its dirty bit on merge failure

[Alexander Ivanov]

When qcow2_merge_common() fails, qcow2_merge_backward_work() clears the
dirty bit of the lower image right away. qcow2_service_iter() waits only
for the service qios, so delayed md writeback of the lower image may
still be pending. With kernel_sets_dirty_bit, a crash at that point
leaves the lower image marked clean on disk with L2 tables and refblocks
that lag behind the merged data.

A failed md writeback of the lower image is also missed:
md_writeback_error is set only by qcow2_flush_deferred_activity(), so
the next resume is not refused.

Flush the deferred activity of the lower image before writing its
header, as qcow2_merge_backward_complete() does.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index 2865940cff30..28c2166fd47a 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -338,6 +338,7 @@ void qcow2_merge_backward_work(struct work_struct *work)
 	ret = qcow2_merge_common(tgt, qcow2);
 	if (ret) {
 		set_backward_merge_in_process(tgt, qcow2, false);
+		qcow2_flush_deferred_activity(tgt, lower);
 		ret2 = qcow2_set_image_file_features(lower, false);
 		if (ret2 < 0)
 			QC_ERR(tgt->ti, "Can't unuse lower (%d)", ret2);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v9 06/10] dm-qcow2: keep the dirty bit of an image that needs check

[Alexander Ivanov]

The dm-qcow2 module parameter kernel_sets_dirty_bit makes the kernel set
the dirty bit on resume and clear it on postsuspend. It is off by
default and Virtuozzo does not set it: ploop manages the dirty bit from
userspace itself. So this is a hardening patch for the non-default mode.

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. The same happens after
an error that sets wants_check, e.g. md_writeback_error: it is kept only
in the target, so resume is refused, but after a table reload or a host
restart the image is reopened with a clean header.

Clear the bit only if the image was clean before the kernel set it and
wants_check is not set. The parsed header can't tell the former: 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_set 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_set 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.

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

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index b667adb5472a..dc083572de5e 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 && !qcow2_wants_check(qcow2->tgt))
 			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_set = dirty && !ret;
+	return ret;
 }
 ALLOW_ERROR_INJECTION(qcow2_set_image_file_features, ERRNO);
 
@@ -903,6 +907,12 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 		goto out;
 
 	qcow2_calc_cached_parameters(qcow2, hdr);
+	if (!(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
+		qcow2->opened_dirty = false;
+		qcow2->kernel_dirty_bit_set = false;
+	} else if (!qcow2->kernel_dirty_bit_set) {
+		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..93c573c9a284 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;
+	bool kernel_dirty_bit_set;
 
 	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 v9 07/10] dm-qcow2: fix md page cleanup on failed reftable relocation

[Alexander Ivanov]

relocate_refcount_table() inserts an md page for every page of the new
reftable tail and of the new refblocks, starting at
pos + old_clus * clu_size, but its error path erases only one page per
cluster starting at end. The remaining pages stay in qcow2->md_pages.
Since a failed relocation leaves the header and reftable_max_file_size
unchanged, the next relocation picks the same pos, gets -EEXIST from
md_page_try_insert() and fails with -EIO. So a single -ENOMEM or header
write error makes every later relocation fail until the target is
reloaded.

Erase every page the allocation loop inserted.

Failures after the in-memory header is switched to the new reftable
also jumped there. At that point the new pages back the live reftable
and may be dirty and linked in slow_wb_batch_list, so freeing them
leaves writeback with freed pages. Just return -EIO on these paths, as
freeing with the fixed range would also free the dirty reftable tail.

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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index fa63bd6acfb8..f6e66799bf27 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2372,21 +2372,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;
 	}
 	spin_unlock_irq(&qcow2->md_pages_lock);
 	if (i != nr_pages)
-		goto err_free_r2_pages;
+		return -EIO;
 
 	/* 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;
+			return -EIO;
 		}
 		ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
 		if (WARN_ON_ONCE(ret <= 0))
-			goto err_free_r2_pages;
+			return -EIO;
 		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);
@@ -2397,7 +2397,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;
+			return -EIO;
 		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
 	}
 
@@ -2405,14 +2405,15 @@ 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;
+			return -EIO;
 		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
 	}
 
 	return 1;
 
 err_free_r2_pages:
-	for (i = end; i < r2_end; i += clu_size) {
+	while (i > pos + (u64)old_clus * clu_size) {
+		i -= PAGE_SIZE;
 		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
 		if (ret <= 0)
 			break;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v9 08/10] dm-qcow2: flag the image for check if reftable relocation fails late

[Alexander Ivanov]

Once relocate_refcount_table() has issued the header write pointing to
the new reftable, any failure returns -EIO. The header on disk may
already point to the new reftable even if its own write fails: a short
write is reported as -ENODATA, and the old reftable position is
restored only in the cached header page, which a v2 image never
rewrites on suspend. The new reftable on disk may then be unfilled or
miss the new refblocks, while refcounts of the new and old reftable
clusters may not be updated. Refcounts no longer match the header, but
nothing reports that, so the device can be resumed and the image
reopened as if it were consistent.

Set md_writeback_error on these paths, so wants_check reports the image
and resume is refused until it is checked, and log the failure.

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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index f6e66799bf27..185243584e0c 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2351,6 +2351,8 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	if (ret) {
 		/* Restore old R1 */
 		set_reftable_in_raw_hdr(md0->page, old_pos, old_clus);
+		QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
+		qcow2->tgt->md_writeback_error = true;
 		goto err_free_r2_pages;
 	}
 
@@ -2376,17 +2378,17 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 	}
 	spin_unlock_irq(&qcow2->md_pages_lock);
 	if (i != nr_pages)
-		return -EIO;
+		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);
-			return -EIO;
+			goto err_hdr_written;
 		}
 		ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
 		if (WARN_ON_ONCE(ret <= 0))
-			return -EIO;
+			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);
@@ -2397,7 +2399,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))
-			return -EIO;
+			goto err_hdr_written;
 		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
 	}
 
@@ -2405,12 +2407,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))
-			return -EIO;
+			goto err_hdr_written;
 		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
 	}
 
 	return 1;
 
+err_hdr_written:
+	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
+	qcow2->tgt->md_writeback_error = true;
+	return -EIO;
 err_free_r2_pages:
 	while (i > pos + (u64)old_clus * clu_size) {
 		i -= PAGE_SIZE;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[Alexander Ivanov]

A failed L1/L2 writeback reverts the page in memory and drops the
refcount of each cluster allocated for it back to zero. The write may
still have partially reached the disk, so L1/L2 on disk may point to
such a cluster. If the page is not rewritten before suspend, the image
is left with a reference to a free cluster, which may also have been
reused for another offset meanwhile. Nothing reports that: L1/L2
writeback errors don't set MD_WRITEBACK_ERROR, as the revert is
considered safe.

Mark such a 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 included in wants_check, so resume is refused until the
image is checked and the target reloaded. The flush loop no longer
stops at the first page with MD_WRITEBACK_ERROR, so a marked page after
it is not missed.

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

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index 28c2166fd47a..0f5d61f94f26 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 185243584e0c..1ecbb525b6aa 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1362,6 +1362,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 		if (cleared)
 			continue; /* no cluster was allocated */
 
+		wbd->md->status |= MD_LX_REVERTED;
 		spin_unlock(&qcow2->md_pages_lock);
 		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
 
@@ -1385,6 +1386,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) {
 		/*
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index dc083572de5e..a9bdad198fde 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)
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 93c573c9a284..4113783b9d15 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;
 
@@ -461,7 +463,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

[Devel] [PATCH hci-8.1 v9 10/10] 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, clamped to file_preallocated_area_start since
the last cluster may be partial. Refcount ranges with no allocated
refblock are skipped at once. The scan walks R1 entries normal I/O may
never touch, so it validates them itself instead of letting
calc_r2_page_id() WARN on a corrupted one. 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.

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. After a reverted L1/L2 writeback
(lx_writeback_error) L1/L2 on disk may point to a cluster whose
refcount was reverted to zero.

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-ploop-target.c |  2 +-
 drivers/md/dm-qcow2-map.c    | 63 ++++++++++++++++++++++++++++++++++--
 drivers/md/dm-qcow2-target.c | 23 ++++++++++---
 drivers/md/dm-qcow2.h        |  1 +
 4 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3b589579741c..a94460c32500 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -447,7 +447,7 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
  *
  * FIXME: We must implement metadata reload on resume. After that we can
  * implement preallocation truncation on suspend (see dm-qcow2 as it already
- * implements that in qcow2_parse_metadata and qcow2_truncate_preallocations).
+ * implements that in qcow2_parse_metadata and qcow2_truncate_unused_tail).
  */
 static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 1ecbb525b6aa..7391531211d9 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1090,6 +1090,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)
 {
@@ -1097,8 +1102,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 */
@@ -2204,6 +2208,61 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
 	return do_ftruncate(file, new_len, 0);
 }
 
+/*
+ * Return the end of the last cluster with non-zero refcount. Must be
+ * called on a suspended device after md writeback.
+ */
+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;
+
+		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) {
+			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) {
+			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 a9bdad198fde..4d92e5abfb5d 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -1058,21 +1058,36 @@ static void qcow2_dtr(struct dm_target *ti)
 	qcow2_tgt_destroy(tgt);
 }
 
-static void qcow2_truncate_preallocations(struct dm_target *ti)
+static void qcow2_truncate_unused_tail(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;
+
+	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;
 
 	ret = qcow2_truncate_safe(qcow2->file, end);
 	if (ret) {
-		QC_ERR(ti, "Can't truncate preallocations");
+		QC_ERR(ti, "Can't truncate unused tail");
 		tgt->truncate_error = true;
 		return;
 	}
@@ -1153,7 +1168,7 @@ static void qcow2_postsuspend(struct dm_target *ti)
 	int ret;
 
 	qcow2_flush_deferred_activity_all(to_qcow2_target(ti));
-	qcow2_truncate_preallocations(ti);
+	qcow2_truncate_unused_tail(ti);
 
 	if (dm_table_get_mode(ti->table) & FMODE_WRITE) {
 		ret = qcow2_set_image_file_features(qcow2, false);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 4113783b9d15..1909931e1598 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -415,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);
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 v9 02/10] 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/20261028150645.3222391-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 v9 03/10] dm-qcow2: extend the file for a new refblock in allocate_cluster()

[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.
> 
> Make find_unused_cluster() report a position not covered by R2
> instead of placing the refblock there itself. allocate_cluster()
> prepares that cluster the same way as a data cluster, with the punch,
> extend and watermark update moved into prepare_unused_cluster(), and
> calls place_r2() before searching again.
> 
> 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/20261028150645.3222391-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 v9 01/10] 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/20261028150645.3222391-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 v9 04/10] 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/20261028150645.3222391-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 v9 05/10] dm-qcow2: flush lower metadata before clearing its dirty bit on merge failure

[no-reply]

> When qcow2_merge_common() fails, qcow2_merge_backward_work() clears the
> dirty bit of the lower image right away. qcow2_service_iter() waits only
> for the service qios, so delayed md writeback of the lower image may
> still be pending. With kernel_sets_dirty_bit, a crash at that point
> leaves the lower image marked clean on disk with L2 tables and refblocks
> that lag behind the merged data.
> 
> A failed md writeback of the lower image is also missed:
> md_writeback_error is set only by qcow2_flush_deferred_activity(), so
> the next resume is not refused.
> 
> Flush the deferred activity of the lower image before writing its
> header, as qcow2_merge_backward_complete() does.
> 
> 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/20261028150645.3222391-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 v9 07/10] dm-qcow2: fix md page cleanup on failed reftable relocation

[no-reply]

> relocate_refcount_table() inserts an md page for every page of the new
> reftable tail and of the new refblocks, starting at
> pos + old_clus * clu_size, but its error path erases only one page per
> cluster starting at end. The remaining pages stay in qcow2->md_pages.
> Since a failed relocation leaves the header and reftable_max_file_size
> unchanged, the next relocation picks the same pos, gets -EEXIST from
> md_page_try_insert() and fails with -EIO. So a single -ENOMEM or header
> write error makes every later relocation fail until the target is
> reloaded.
> 
> Erase every page the allocation loop inserted.
> 
> Failures after the in-memory header is switched to the new reftable
> also jumped there. At that point the new pages back the live reftable
> and may be dirty and linked in slow_wb_batch_list, so freeing them
> leaves writeback with freed pages. Just return -EIO on these paths, as
> freeing with the fixed range would also free the dirty reftable tail.
> 
> 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/20261028150645.3222391-1-alexander.ivanov@virtuozzo.com?part=7


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

Re: [Devel] [PATCH hci-8.1 v9 06/10] dm-qcow2: keep the dirty bit of an image that needs check

[no-reply]

> The dm-qcow2 module parameter kernel_sets_dirty_bit makes the kernel set
> the dirty bit on resume and clear it on postsuspend. It is off by
> default and Virtuozzo does not set it: ploop manages the dirty bit from
> userspace itself. So this is a hardening patch for the non-default mode.
> 
> 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. The same happens after
> an error that sets wants_check, e.g. md_writeback_error: it is kept only
> in the target, so resume is refused, but after a table reload or a host
> restart the image is reopened with a clean header.
> 
> Clear the bit only if the image was clean before the kernel set it and
> [ ... ]
> clear a bit set by userspace while the device was suspended.
> 
> 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/20261028150645.3222391-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 v9 08/10] dm-qcow2: flag the image for check if reftable relocation fails late

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: relocate_refcount_table() late failure leaves target live on half-converted metadata
- [Low] dm-qcow2: relocate_refcount_table() logs the same message for two different failures
--

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

dm-qcow2: flag the image for check if reftable relocation fails late

This patch makes relocate_refcount_table() set md_writeback_error and log
a message when it fails after the header write that points to the new
reftable. With the flag set, wants_check reports the image and resume is
refused until the image is checked.

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

[ ... ]

> @@ -2351,6 +2351,8 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>  	if (ret) {
>  		/* Restore old R1 */
>  		set_reftable_in_raw_hdr(md0->page, old_pos, old_clus);
> +		QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +		qcow2->tgt->md_writeback_error = true;
>  		goto err_free_r2_pages;
>  	}

[Severity: Low]
This isn't a bug, but the message logged here after a failed
qcow2_rw_page_sync() of the header is identical to the one logged at
err_hdr_written.

Can an administrator tell these two failures apart from the log?

In the header write failure case the cached header is restored and a clean
failure is possible, while at err_hdr_written the header has already been
written and the in-memory state is partly converted. Would distinct
messages make the two cases distinguishable?

Setting md_writeback_error here is conservative, since a short write may
still have reached disk, so that choice looks acceptable.

[ ... ]

> @@ -2405,12 +2407,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))
> -			return -EIO;
> +			goto err_hdr_written;
>  		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>  	}
>  
>  	return 1;
>  
> +err_hdr_written:
> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +	qcow2->tgt->md_writeback_error = true;
> +	return -EIO;
>  err_free_r2_pages:

[Severity: Medium]
Does the target stay live on half-converted metadata after this path
returns -EIO?

By the time err_hdr_written is reached, hdr.refcount_table_offset and
hdr.refcount_table_clusters already point at the new reftable. Only some of
the R1 pages may have been renumbered, the new R2 may be only partly
connected, and the used/unused marking may be incomplete.

The only containment here is the md_writeback_error flag, and it is
consulted only in a few places:

dm-qcow2-target.c:qcow2_preresume()
    if (qcow2_wants_check(tgt)) {
        QC_ERR(ti, "image check and target reload are required");
        return -EIO;

Besides preresume, the flag is read only in the status dump, the
dirty-bit decision and the suspend-time truncate logic. dm-qcow2-map.c
only sets it and never reads it.

The allocation path does not check it either:

allocate_cluster()
    if (unlikely(pos == -ENOENT)) {
        ret = relocate_refcount_table(qcow2, &qio);
        if (ret <= 0)
            return ret;
        goto again;
    }

Can a later allocation that finds no free cluster re-enter
relocate_refcount_table() against the already modified hdr and cached
parameters? Can the target keep servicing writes and cause further
refcount inconsistency until suspend?

Would it make sense to make the target read-only, or to have
allocate_cluster() and the relocation entry check md_writeback_error? The
/* TODO: switch to RO */ comment is still present in the err_free_r2_pages
path of this function.

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

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

Re: [Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-qcow2: revert_l_entries_update() skips spin_lock() on the WARN continue path
--

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

dm-qcow2: flag the image for check after a reverted L1/L2 writeback

This patch adds an MD_LX_REVERTED page status that is set when an L1/L2
writeback fails and the page is reverted. The flag is cleared on the next
successful writeback. On md writeback flush, a page that is still marked
sets a new lx_writeback_error flag. That flag is reported by get_errors and
included in qcow2_wants_check().

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

[ ... ]

> @@ -1362,6 +1362,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  		if (cleared)
>  			continue; /* no cluster was allocated */
>  
> +		wbd->md->status |= MD_LX_REVERTED;
>  		spin_unlock(&qcow2->md_pages_lock);
>  		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>  

[Severity: Medium]
Can the lock be left unheld on the WARN path in this loop?

The loop body in revert_l_entries_update() continues like this after the
quoted lines:

	ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
	if (WARN_ON_ONCE(ret <= 0))
		continue;

	mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
	spin_lock(&qcow2->md_pages_lock);

The lock is dropped just above, but the continue skips the closing
spin_lock(). The next iteration then reads wbd->md->page, calls
set_u64_to_be_page(), and executes the new store to wbd->md->status
without md_pages_lock. It may also reach spin_unlock() a second time on a
lock that is not held.

The caller does not expect this:

complete_wbd() {
	spin_lock_irqsave(&qcow2->md_pages_lock, flags);
	revert_l_entries_update(qcow2, wbd);
	clear_writeback_status(qcow2, wbd->md, wbd->ret,
			       &wait_list, &end_list);
	spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
	...
}

If the lock is not re-acquired, clear_writeback_status() and
spin_unlock_irqrestore() run on a lock that is no longer owned. That can
corrupt the spinlock and IRQ state, and it races with other users of
md_pages_lock.

qcow2_handle_r1r2_maps() can return a value <= 0 in several cases. It
returns -ENOENT when the R1 entry is zero, and -EIO on an invalid R1 entry
or when the refcount sanity check fails (entry > 1 with no snapshots). It
returns 0 when an R1/R2 page is not cached.

The comment says R1/R2 "should be cached", but I could not show that none
of these returns can happen. For example, a failed md read or a failed
relocate_refcount_table() can erase a cached page.

The unlock/relock imbalance is not new, but this patch adds an
unlocked-path store to md->status in the same loop. The later patch in the
series, "dm-qcow2: truncate unused tail of image on suspend", does not
change this handling.

Could the WARN path re-take md_pages_lock before continuing, or could the
loop be restructured so the lock state is the same on every path?

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

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

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

[no-reply]

> 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, clamped to file_preallocated_area_start since
> the last cluster may be partial. Refcount ranges with no allocated
> refblock are skipped at once. The scan walks R1 entries normal I/O may
> never touch, so it validates them itself instead of letting
> calc_r2_page_id() WARN on a corrupted one. Metadata pages are taken from
> md cache or read synchronously. This is called on suspended device after
> [ ... ]
> 
> 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/20261028150645.3222391-1-alexander.ivanov@virtuozzo.com?part=10


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

Re: [Devel] [PATCH hci-8.1 v9 08/10] dm-qcow2: flag the image for check if reftable relocation fails late

[Alexander Ivanov]

>> @@ -2351,6 +2351,8 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
>>   	if (ret) {
>>   		/* Restore old R1 */
>>   		set_reftable_in_raw_hdr(md0->page, old_pos, old_clus);
>> +		QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
>> +		qcow2->tgt->md_writeback_error = true;
>>   		goto err_free_r2_pages;
>>   	}
> [Severity: Low]
> This isn't a bug, but the message logged here after a failed
> qcow2_rw_page_sync() of the header is identical to the one logged at
> err_hdr_written.
>
> Can an administrator tell these two failures apart from the log?
>
> In the header write failure case the cached header is restored and a clean
> failure is possible, while at err_hdr_written the header has already been
> written and the in-memory state is partly converted. Would distinct
> messages make the two cases distinguishable?
>
> Setting md_writeback_error here is conservative, since a short write may
> still have reached disk, so that choice looks acceptable.
The two cases are not really different: a failed header write may still have
reached disk, so both need the same check, and every late failure is already
preceded by a WARN splat that tells them apart in the log.
>> @@ -2405,12 +2407,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))
>> -			return -EIO;
>> +			goto err_hdr_written;
>>   		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>>   	}
>>   
>>   	return 1;
>>   
>> +err_hdr_written:
>> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
>> +	qcow2->tgt->md_writeback_error = true;
>> +	return -EIO;
>>   err_free_r2_pages:
> [Severity: Medium]
> Does the target stay live on half-converted metadata after this path
> returns -EIO?
>
> By the time err_hdr_written is reached, hdr.refcount_table_offset and
> hdr.refcount_table_clusters already point at the new reftable. Only some of
> the R1 pages may have been renumbered, the new R2 may be only partly
> connected, and the used/unused marking may be incomplete.
>
> The only containment here is the md_writeback_error flag, and it is
> consulted only in a few places:
>
> dm-qcow2-target.c:qcow2_preresume()
>      if (qcow2_wants_check(tgt)) {
>          QC_ERR(ti, "image check and target reload are required");
>          return -EIO;
>
> Besides preresume, the flag is read only in the status dump, the
> dirty-bit decision and the suspend-time truncate logic. dm-qcow2-map.c
> only sets it and never reads it.
>
> The allocation path does not check it either:
>
> allocate_cluster()
>      if (unlikely(pos == -ENOENT)) {
>          ret = relocate_refcount_table(qcow2, &qio);
>          if (ret <= 0)
>              return ret;
>          goto again;
>      }
>
> Can a later allocation that finds no free cluster re-enter
> relocate_refcount_table() against the already modified hdr and cached
> parameters? Can the target keep servicing writes and cause further
> refcount inconsistency until suspend?
>
> Would it make sense to make the target read-only, or to have
> allocate_cluster() and the relocation entry check md_writeback_error? The
> /* TODO: switch to RO */ comment is still present in the err_free_r2_pages
> path of this function.
This patch only adds reporting to error paths that already returned -EIO
and left the target running, so it is not a regression; containing the 
target
after such a failure, by making it read-only or blocking further relocation,
is a separate change.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[Alexander Ivanov]

>> @@ -1362,6 +1362,7 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   		if (cleared)
>>   			continue; /* no cluster was allocated */
>>   
>> +		wbd->md->status |= MD_LX_REVERTED;
>>   		spin_unlock(&qcow2->md_pages_lock);
>>   		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>>   
> [Severity: Medium]
> Can the lock be left unheld on the WARN path in this loop?
>
> The loop body in revert_l_entries_update() continues like this after the
> quoted lines:
>
> 	ret = qcow2_handle_r1r2_maps(qcow2, pos, NULL, &r1, &r2, false);
> 	if (WARN_ON_ONCE(ret <= 0))
> 		continue;
>
> 	mark_cluster_unused(qcow2, r2.md, r2.index_in_page, pos);
> 	spin_lock(&qcow2->md_pages_lock);
>
> The lock is dropped just above, but the continue skips the closing
> spin_lock(). The next iteration then reads wbd->md->page, calls
> set_u64_to_be_page(), and executes the new store to wbd->md->status
> without md_pages_lock. It may also reach spin_unlock() a second time on a
> lock that is not held.
>
> The caller does not expect this:
>
> complete_wbd() {
> 	spin_lock_irqsave(&qcow2->md_pages_lock, flags);
> 	revert_l_entries_update(qcow2, wbd);
> 	clear_writeback_status(qcow2, wbd->md, wbd->ret,
> 			       &wait_list, &end_list);
> 	spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
> 	...
> }
>
> If the lock is not re-acquired, clear_writeback_status() and
> spin_unlock_irqrestore() run on a lock that is no longer owned. That can
> corrupt the spinlock and IRQ state, and it races with other users of
> md_pages_lock.
>
> qcow2_handle_r1r2_maps() can return a value <= 0 in several cases. It
> returns -ENOENT when the R1 entry is zero, and -EIO on an invalid R1 entry
> or when the refcount sanity check fails (entry > 1 with no snapshots). It
> returns 0 when an R1/R2 page is not cached.
>
> The comment says R1/R2 "should be cached", but I could not show that none
> of these returns can happen. For example, a failed md read or a failed
> relocate_refcount_table() can erase a cached page.
>
> The unlock/relock imbalance is not new, but this patch adds an
> unlocked-path store to md->status in the same loop. The later patch in the
> series, "dm-qcow2: truncate unused tail of image on suspend", does not
> change this handling.
>
> Could the WARN path re-take md_pages_lock before continuing, or could the
> loop be restructured so the lock state is the same on every path?
The lock imbalance on the WARN path has been there since the driver was
introduced, and this patch doesn't make it worse: the new MD_LX_REVERTED
store sits before spin_unlock(), so like the existing 
set_u64_to_be_page() it
only runs unlocked after that path has already been taken, which is why the
fix belongs in a separate patch.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v9 00/10] dm-qcow2: truncate unused tail of image on suspend

[Andrey Zhadchenko]

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

On 10/28/26 16:06, Alexander Ivanov wrote:
> 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-9 fix existing cases where the refcounts or the
> dirty bit on disk don't reflect real cluster usage, or where such an
> image is not flagged for check, which the scan relies on. Patch 10 does
> the actual truncation.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> 
> v9:
> - Patch 5: new patch.
> - Patch 6: also keep the dirty bit while wants_check is set, as
>    md_writeback_error is kept only in the target and is lost on table
>    reload or host restart. Rename kernel_dirty_bit to
>    kernel_dirty_bit_set. Retitled accordingly.
> - Patch 7: new patch.
> - Patches 8, 9: split from patch 10: setting md_writeback_error on a
>    late reftable relocation failure and the MD_LX_REVERTED /
>    lx_writeback_error tracking are separate fixes now.
> - Patch 10: rename qcow2_truncate_preallocations() to
>    qcow2_truncate_unused_tail() and update the dm-ploop comment that
>    refers to it. Move the rationale from code comments to the commit
>    message.
> 
> v8:
> - Patch 3: instead of extending the file in place_r2(), make
>    find_unused_cluster() return the position for the new refblock and
>    let allocate_cluster() prepare it the same way as a data cluster
>    (punch, extend, watermark update moved into prepare_unused_cluster())
>    before calling place_r2(). Retitled accordingly.
> 
> 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 (10):
>    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 for a new refblock in allocate_cluster()
>    dm-qcow2: complete wbd before ending the data qio
>    dm-qcow2: flush lower metadata before clearing its dirty bit on merge
>      failure
>    dm-qcow2: keep the dirty bit of an image that needs check
>    dm-qcow2: fix md page cleanup on failed reftable relocation
>    dm-qcow2: flag the image for check if reftable relocation fails late
>    dm-qcow2: flag the image for check after a reverted L1/L2 writeback
>    dm-qcow2: truncate unused tail of image on suspend
> 
>   drivers/md/dm-ploop-target.c |   2 +-
>   drivers/md/dm-qcow2-cmd.c    |   5 +-
>   drivers/md/dm-qcow2-map.c    | 180 +++++++++++++++++++++++++----------
>   drivers/md/dm-qcow2-target.c | 119 +++++++++++++++++------
>   drivers/md/dm-qcow2.h        |  10 +-
>   5 files changed, 234 insertions(+), 82 deletions(-)
> 

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

Re: [Devel] [PATCH hci-8.1 v9 01/10] dm-qcow2: add qcow2_md_page_read_sync() helper

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> + * 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.

- Require "no concurrent lookup of this page" in the comment.
- Set *md = NULL on both error paths.

These are my v5 remarks, v9 has neither.

A qio that looks the page up before MD_UPTODATE is set waits on
md->wait_list. This helper never wakes it, and on a read error it frees
the page with the qio still queued. So a read in flight is not the only
thing the caller must exclude.

On a read error, and on -EEXIST from qcow2_alloc_and_insert_md_page(),
*md points to freed memory. No caller uses it today, but the next one
will not expect that.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 04/10] dm-qcow2: complete wbd before ending the data qio

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> 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.

- Say what happens to that R2 page instead of "never written back".

The page is dirty and sits on slow_wb_batch_list. Preresume frees all
md pages in qcow2_free_md_pages_all(), which does not take them off
that list, so the slow writeback timer can later walk a freed page. If
the timer comes first, it writes the page after postsuspend, behind the
flush. Both are worse than a lost update, and the message should say
which one you are fixing.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 02/10] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages

[Konstantin Khorenko]

No changes needed in this patch.

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

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 05/10] dm-qcow2: flush lower metadata before clearing its dirty bit on merge failure

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> leaves the lower image marked clean on disk with L2 tables and refblocks
> that lag behind the merged data.
>
> A failed md writeback of the lower image is also missed:
> md_writeback_error is set only by qcow2_flush_deferred_activity(), so
> the next resume is not refused.

- Drop the second paragraph.
- Say refblocks only, not "L2 tables and refblocks".
- Shorten the subject, it is 77 columns.

The next postsuspend flushes the lower image as well:
qcow2_flush_deferred_activity_all() walks tgt->top and every ->lower
and sets md_writeback_error on a failed write. So that resume is
refused already. What the patch adds is the order: the flush now comes
before the header write that clears the dirty bit.

Merge writes wait for their L2 writeback before they complete, so L2
does not lag. Only the refblocks are in the delayed writeback.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 08/10] dm-qcow2: flag the image for check if reftable relocation fails late

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> +err_hdr_written:
> +	QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> +	qcow2->tgt->md_writeback_error = true;
> +	return -EIO;

- Make the two messages differ and print the error code.

You answered that a WARN before every late failure tells the two
apart. The header write failure has no WARN, and the late paths use
WARN_ON_ONCE. So from the second failure on, both cases print the same
single line, and the error of the header write is not printed at all.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 09/10] dm-qcow2: flag the image for check after a reverted L1/L2 writeback

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
>  		if (cleared)
>  			continue; /* no cluster was allocated */
>
> +		wbd->md->status |= MD_LX_REVERTED;

- Leak the clusters on a failed L1/L2 writeback instead of freeing
  them.
- If you keep freeing them, set MD_LX_REVERTED in the pe_page branch
  above as well.

The pe_page branch restores the old entry and continues before this
line. It is taken for a COW over a compressed, snapshot-shared or
preallocated cluster. The new cluster is still freed: the qio waiting
on the page ends with the error, and handle_cleanup_mask() frees it
through FREE_ALLOCATED_CLU. So L2 on disk may point to a free cluster,
lx_writeback_error stays clear, resume is allowed, and patch 10 can
cut that cluster off.

Freeing also lets the allocator give the cluster to another offset
before suspend, while L2 on disk still maps the old offset to it. Two
guest offsets then share one cluster, and the check that wants_check
asks for cannot undo that. A leaked cluster has none of this: the
check just frees it.

Leaking also needs no resume refusal. ploop does not look at
wants_check, so with this patch a transient L2 write error leaves a
live check or a snapshot with the device suspended.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

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

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> inconsistent. Userspace checks the image before opening it RW, which
> find_unused_cluster() already relies on when it reuses clusters with
> zero refcount.

- Drop the find_unused_cluster() argument, and say plainly that in
  the default mode the scan is safe only because userspace checks the
  image before an RW open.
- Say that v2 images are covered only by that check too.
- Split the paragraphs longer than four sentences.

The allocator reuses a wrongly free cluster only on an allocating
write, and it takes the lowest free cluster first, so it reaches the
tail last. The scan reads the tail on every RW suspend. After a crash
the stale refcounts are exactly at the tail, because L2 goes out with
the request and refblocks wait for the slow writeback. So the two do
not carry the same risk.

A v2 image has no dirty bit, so the opened_dirty guard never applies
to it.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 v9 00/10] dm-qcow2: truncate unused tail of image on suspend

[Konstantin Khorenko]

I won't apply this version, please send v10 with the changes below.

On 10/28/26 16:06, Alexander Ivanov wrote:
> 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-9 fix existing cases where the refcounts or the
> dirty bit on disk don't reflect real cluster usage, or where such an
> image is not flagged for check, which the scan relies on. Patch 10 does
> the actual truncation.

- Add a Fixes tag for 222b6f1291c1c ("dm-qcow2: Introduce driver to
  create block devices over QCOW2 files") to patches 3, 4, 5, 7, 8
  and 9.
- Put a blank line before Signed-off-by in patches 4, 5 and 6.
- Describe patch 7 on its own here.
- Say whether the ENOSPC retry of md writeback is planned.

Patches 3-9 fix bugs that came with the driver itself. Other dm-qcow2
fixes in the tree carry the tag, these should too.

Patch 7 is not about refcounts or the dirty bit. It fixes a relocation
that keeps failing after one error, and pages freed while they are
still on the writeback list.

My v5 cover reply asked for three fixes. Patch 3 does place_r2(), and
patch 9 handles the failed L1/L2 write (see my reply there). The ENOSPC
retry of md writeback is not done: a refblock write that fails on
ENOSPC still blocks resume until a reload, and the "FIXME: call
md_make_dirty() and try once again?" is still there.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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