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

[Devel] [PATCH hci-8.1 v10 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-8 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 9
retries an R1/R2 writeback that failed with ENOSPC, so the next resume
is refused only if the space is still missing at suspend. Patch 10
does the actual truncation.

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

v10:
- Rebase onto 22.19.
- Drop patch 7 "fix md page cleanup on failed reftable relocation",
  merged as beada5e3721a ("dm-qcow2: fix a failed refcount table
  relocation leaking md pages").
- Add Fixes tag to patches 3, 4, 5, 7 and 8.
- Patch 1: set *md to NULL on error, document that a lookup of the page
  while it is read is never woken.
- Patch 4: describe what happens to the R2 page dirtied after the flush.
- Patch 5: drop the claims about L2 tables and a missed md writeback
  error from the commit message, retitle.
- Patches 4-6: add a blank line before Signed-off-by.
- Patch 7 (was 8): log the error code and tell the two failures apart.
- Patch 8 (was 9): keep the refcounts of clusters allocated for a
  failed L1/L2 writeback, leaking them, instead of flagging the image
  with MD_LX_REVERTED / lx_writeback_error. Retitled accordingly.
- Patch 9: new patch.
- Patch 10: drop lx_writeback_error handling, mention v2
  images in the commit message.

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 md before clearing dirty bit on merge failure
  dm-qcow2: keep the dirty bit of an image that needs check
  dm-qcow2: flag the image for check if reftable relocation fails late
  dm-qcow2: leak clusters allocated for a failed L1/L2 writeback
  dm-qcow2: retry R1/R2 writeback that failed with ENOSPC
  dm-qcow2: truncate unused tail of image on suspend

 drivers/md/dm-ploop-target.c |   2 +-
 drivers/md/dm-qcow2-cmd.c    |   1 +
 drivers/md/dm-qcow2-map.c    | 244 +++++++++++++++++++++--------------
 drivers/md/dm-qcow2-target.c | 112 ++++++++++++----
 drivers/md/dm-qcow2.h        |   5 +
 5 files changed, 241 insertions(+), 123 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v10 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 | 48 ++++++++++++++++++++++++++++++++----
 drivers/md/dm-qcow2.h        |  2 ++
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 7c572bd66e1c..0cbbd8e4f493 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -418,6 +418,48 @@ 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. Sets *md to NULL on error.
+ *
+ * The caller must ensure nobody else looks this page up until this
+ * returns (e.g., the device is suspended). A lookup before MD_UPTODATE
+ * is set queues its qio on md->wait_list, which is never woken here,
+ * and the page is freed with the qio still queued on read error.
+ * A cached page that is not MD_UPTODATE yet makes the insert fail.
+ */
+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)
+		goto err;
+
+	ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);
+	if (ret) {
+		spin_lock_irq(&qcow2->md_pages_lock);
+		qcow2_md_page_erase(qcow2, *md);
+		spin_unlock_irq(&qcow2->md_pages_lock);
+		qcow2_free_md_page(*md);
+		goto err;
+	}
+
+	spin_lock_irq(&qcow2->md_pages_lock);
+	(*md)->status |= MD_UPTODATE;
+	spin_unlock_irq(&qcow2->md_pages_lock);
+	return 0;
+err:
+	*md = NULL;
+	return ret;
+}
+
 static void inflight_ref_exit0(struct percpu_ref *ref)
 {
 	struct qcow2_target *tgt = container_of(ref, struct qcow2_target,
@@ -851,13 +893,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 ad5286b16883..5abc7f5ad239 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -388,6 +388,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 v10 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 0cbbd8e4f493..bb322254cea9 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;
 		}
@@ -840,19 +840,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;
@@ -886,7 +886,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;
@@ -918,14 +918,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 */
 			}
 		}
@@ -947,11 +947,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;
 		}
 	}
@@ -986,7 +986,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;
 }
 
@@ -1067,7 +1067,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;
 	}
@@ -1153,7 +1153,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)
@@ -1162,7 +1162,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;
 	}
 
@@ -1183,7 +1183,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 v10 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
Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

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 34d9d9810f60..e8a41954c4c6 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2083,21 +2083,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);
@@ -2113,7 +2104,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,
@@ -2135,11 +2126,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)
@@ -2151,15 +2144,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);
@@ -2436,6 +2429,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
@@ -2445,13 +2469,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)
@@ -2461,28 +2484,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 v10 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 an R2 page after the flush.

That page sits on slow_wb_batch_list with the slow writeback timer
armed. If the timer fires first, the page is written while the device
is suspended, after postsuspend has cleared the dirty bit. If
preresume comes first, qcow2_free_md_pages_all() frees the page
without taking it off the list, and the timer later walks freed
memory.

Complete the wbd first, so the R2 update is in place before the
inflight ref goes away and the suspend flush writes it back. 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
Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

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 e8a41954c4c6..7866c791325b 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2892,13 +2892,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 v10 05/10] dm-qcow2: flush lower md before clearing 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 refblocks that lag
behind the merged data.

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
Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

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 45c517ce0ef8..3bccce090f01 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -330,6 +330,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 v10 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 bb322254cea9..6824d95193ae 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -666,6 +666,7 @@ int qcow2_set_image_file_features(struct qcow2 *qcow2, bool dirty)
 	u64 dirty_mask = cpu_to_be64(INCOMPATIBLE_FEATURES_DIRTY_BIT);
 	struct QCowHeader *raw_hdr;
 	struct md_page *md;
+	int ret;
 
 	if (qcow2->hdr.version ==  2)
 		return 0;
@@ -682,12 +683,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);
 
@@ -911,6 +915,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 5abc7f5ad239..2a6203c86e3a 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -259,6 +259,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 v10 07/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
Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 7866c791325b..cd46a99f81a7 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2351,6 +2351,10 @@ 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 write header with relocated refcount table: %d",
+		       ret);
+		qcow2->tgt->md_writeback_error = true;
 		goto err_free_r2_pages;
 	}
 
@@ -2372,24 +2376,25 @@ 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_eio */
+			break;
 	}
 	spin_unlock_irq(&qcow2->md_pages_lock);
-	if (i != nr_pages)
-		goto err_eio;
+	if (i != nr_pages) {
+		ret = -ENOENT;
+		goto err_hdr_written;
+	}
 
 	/* Connect new R2 to new R1: one for each group new R1 and R2 span */
 	group_size = (u64)qcow2->refblock_entries * clu_size;
 	for (i = pos, r2_pos = end; i < r2_end; i += group_size, r2_pos += clu_size) {
 		if (WARN_ON_ONCE(r2_pos >= r2_end))
-			goto err_eio;
-		if (calc_refcounters_map(qcow2, i, &r1, &r2) < 0) {
-			WARN_ON_ONCE(1);
-			goto err_eio;
-		}
+			goto err_hdr_written;
+		ret = calc_refcounters_map(qcow2, i, &r1, &r2);
+		if (WARN_ON_ONCE(ret < 0))
+			goto err_hdr_written;
 		ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
 		if (WARN_ON_ONCE(ret <= 0))
-			goto err_eio;
+			goto err_hdr_written;
 		spin_lock_irq(&qcow2->md_pages_lock);
 		set_u64_to_be_page(md->page, r1.index_in_page, r2_pos);
 		qcow2_md_make_dirty(qcow2, md, true);
@@ -2400,7 +2405,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_eio;
+			goto err_hdr_written;
 		mark_cluster_used(qcow2, r2.md, r2.index_in_page);
 	}
 
@@ -2408,12 +2413,18 @@ 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_eio;
+			goto err_hdr_written;
 		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
 	}
 
 	return 1;
 
+err_hdr_written:
+	/* 0: md page is not cached */
+	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
+	       ret ?: -ENOENT);
+	qcow2->tgt->md_writeback_error = true;
+	return -EIO;
 err_free_r2_pages:
 	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {
 		ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
@@ -2424,7 +2435,6 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
 		spin_unlock_irq(&qcow2->md_pages_lock);
 		qcow2_free_md_page(md);
 	}
-err_eio:
 	/* TODO: switch to RO */
 	return -EIO;
 }
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v10 08/10] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[Alexander Ivanov]

A failed L1/L2 writeback reverts the page in memory and frees each
cluster allocated for it: revert_l_entries_update() drops the refcount
of a newly allocated cluster, and a COW qio waiting on the page frees
its cluster through FREE_ALLOCATED_CLU when it ends with the error. The
write may still have partially reached the disk, so L1/L2 on disk may
point to such a cluster. Until the page is rewritten, the allocator may
give the cluster to another offset, and two guest offsets then share
it. If the page is not rewritten before suspend, the image is left with
a reference to a free cluster.

Keep the refcount of these clusters instead. If L1/L2 on disk points to
such a cluster, the image stays consistent; otherwise the cluster is
leaked, which check repairs by freeing it. So the image needs no check
and resume needs no refusal.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c | 48 +++++++--------------------------------
 1 file changed, 8 insertions(+), 40 deletions(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index cd46a99f81a7..4197f7704e18 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1331,50 +1331,15 @@ static void do_md_page_read_complete(int ret, struct qcow2 *qcow2,
 /* Be careful with dirty_or_writeback()/etc! Check races. */
 static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
 {
-	struct qcow2_map_item r1, r2;
-	struct page *pe_page;
-	bool skip_odd;
-	u64 pos, old;
-	bool cleared;
-	int i, ret;
-
-	skip_odd = qcow2->ext_l2 && wbd->lx_level == L2_LEVEL;
+	struct page *pe_page = wbd->pe_page;
+	u64 old;
+	int i;
 
 	lockdep_assert_held(&qcow2->md_pages_lock);
 	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
-		pos = get_u64_from_be_page(wbd->md->page, i);
-
-		cleared = !(pos & ~(u64)L2_READS_ALL_ZEROES);
-
 		/* Here we restore prealloced and compressed clu mappings */
-		pe_page = wbd->pe_page;
-		if (pe_page) { /* Only L2 has this. */
-			old = get_u64_from_be_page(pe_page, i);
-			if (old != 0) {
-				set_u64_to_be_page(wbd->md->page, i, old);
-				continue; /* Avoid mark_cluster_unused() */
-			}
-		}
-
-		set_u64_to_be_page(wbd->md->page, i, 0);
-		if (skip_odd && (i & 1))
-			continue; /* pos contains ext_l2 part of L2 entry */
-		if (cleared)
-			continue; /* no cluster was allocated */
-
-		spin_unlock(&qcow2->md_pages_lock);
-		pos &= ~LX_REFCOUNT_EXACTLY_ONE;
-
-		/*
-		 * R1/R2 should be cached, since we was able
-		 * to submit cluster allocation.
-		 */
-		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);
+		old = pe_page ? get_u64_from_be_page(pe_page, i) : 0;
+		set_u64_to_be_page(wbd->md->page, i, old);
 	}
 }
 
@@ -1408,6 +1373,7 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
 		LIST_HEAD(wait_list);
 		LIST_HEAD(end_list);
 		unsigned long flags;
+		struct qio *qio;
 
 		spin_lock_irqsave(&qcow2->md_pages_lock, flags);
 		revert_l_entries_update(qcow2, wbd);
@@ -1415,6 +1381,8 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
 				       &wait_list, &end_list);
 		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
 
+		list_for_each_entry(qio, &end_list, link)
+			qio->ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
 		qcow2_dispatch_qios(qcow2, NULL, &wait_list);
 		end_qios(&end_list, errno_to_blk_status(wbd->ret));
 	}
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v10 09/10] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

[Alexander Ivanov]

A failed R1/R2 writeback sets MD_WRITEBACK_ERROR and drops MD_DIRTY.
Nothing writes the page again unless its refcounts change. On suspend
the md writeback flush finds the error and sets md_writeback_error, so
resume is refused until the image is checked and the target reloaded,
even if the failure was a transient ENOSPC of the underlying fs and
the space has been freed since.

Redirty the page on -ENOSPC, so the slow writeback timer retries it,
as data writes delayed on ENOSPC are retried. The suspend flush fires
the timer, which gives the page one more try. A page that still fails
then is not redirtied, as wants_suspend is set, so nothing is left
pending on a suspended device and md_writeback_error is set as before.

complete_metadata_writeback() turned every failed write into -EIO.
Pass the errno of the write through and keep -EIO for a short write.
A data request depending on a failed L1/L2 write then ends with
BLK_STS_NOSPC and is delayed and resubmitted like any other write
failing with ENOSPC, as already happens when fsync fails with it.

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

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 4197f7704e18..27ae22495f37 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1357,6 +1357,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
 		 * so here we care about R1R2 only.
 		 */
 		md->status |= MD_WRITEBACK_ERROR;
+		if (ret == -ENOSPC && !READ_ONCE(qcow2->tgt->wants_suspend))
+			qcow2_md_make_dirty(qcow2, md, true);
 	}
 	if (md->wbd) {
 		if (likely(ret == 0))
@@ -3864,9 +3866,9 @@ static int complete_metadata_writeback(struct qcow2 *qcow2)
 		md = qio->ext->md;
 		qvec = qio->data;
 		ret = qio->ret;
-		if (unlikely(ret != PAGE_SIZE))
+		if (unlikely(ret >= 0 && ret != PAGE_SIZE))
 			ret = -EIO;
-		else
+		else if (ret == PAGE_SIZE)
 			ret = fsync_ret;
 
 		do_md_page_write_complete(ret, qcow2, md);
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 6824d95193ae..1b7a75239291 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -190,7 +190,6 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
 	for (node = rb_first(&qcow2->md_pages);
 	     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;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v10 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 a failed R1/R2 writeback, which leaves refcounts
on disk stale.

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.

In the default mode userspace sets the dirty bit on every RW open, so
the bit doesn't mean the image is inconsistent. There the scan is safe
only because userspace checks the image before an RW open. A v2 image
has no dirty bit, so the opened_dirty guard never applies to it, and
it is covered only by that check too.

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 | 21 +++++++++---
 drivers/md/dm-qcow2.h        |  1 +
 4 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index d8279c79ef56..b869c92bbe72 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 27ae22495f37..d96532b3753e 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 */
@@ -2169,6 +2173,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 1b7a75239291..7587e8f7ac0b 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -1062,21 +1062,34 @@ 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 (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;
 	}
@@ -1157,7 +1170,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 2a6203c86e3a..b0ee70bd5497 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 v10 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/20261109115856.1362416-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 v10 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
> Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")
> 
> 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/20261109115856.1362416-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 v10 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/20261109115856.1362416-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 v10 05/10] dm-qcow2: flush lower md before clearing 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 refblocks that lag
> behind the merged data.
> 
> 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
> Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")
> 
> 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/20261109115856.1362416-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 v10 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 an R2 page after the flush.
> 
> That page sits on slow_wb_batch_list with the slow writeback timer
> armed. If the timer fires first, the page is written while the device
> is suspended, after postsuspend has cleared the dirty bit. If
> preresume comes first, qcow2_free_md_pages_all() frees the page
> without taking it off the list, and the timer later walks freed
> memory.
> 
> Complete the wbd first, so the R2 update is in place before the
> inflight ref goes away and the suspend flush writes it back. 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
> Fixes: 222b6f1291c1c ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")
> 
> 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/20261109115856.1362416-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 v10 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
> [ ... ]
> 
> 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/20261109115856.1362416-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 v10 08/10] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: clusters of a failed L1/L2 writeback leak at runtime and are not logged
- [Low] dm-qcow2: stale comment in qcow2_flush_deferred_activity() after revert change
--

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

dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

This patch stops revert_l_entries_update() from dropping the refcount of
clusters allocated for a failed L1/L2 writeback. It also clears
FREE_ALLOCATED_CLU on COW qios that end with the error, so those clusters
keep their refcount too. The commit message says a later check repairs any
clusters that end up leaked.

> 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

[ ... ]

>  static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>  {
> +	struct page *pe_page = wbd->pe_page;
> +	u64 old;
> +	int i;
>  
>  	lockdep_assert_held(&qcow2->md_pages_lock);
>  	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>  		/* Here we restore prealloced and compressed clu mappings */
> +		old = pe_page ? get_u64_from_be_page(pe_page, i) : 0;
> +		set_u64_to_be_page(wbd->md->page, i, old);
>  	}
>  }

[Severity: Medium]
Does this leave the newly allocated clusters permanently allocated for the
lifetime of the running instance?

The data cluster from LU_WANTS_ALLOC and the new L2-table cluster from an
L1 update are allocated in prepare_l_entry_update(), and
mark_cluster_used() sets their refcount to 1.

If the md page writeback then fails, complete_wbd() calls
revert_l_entries_update(), which now only restores the in-memory entry
from pe_page, or to 0. Nothing calls mark_cluster_unused() for these
clusters any more, and no other path frees them.

The guest can retry the failed write, and that retry allocates a new
cluster because the old one is still marked used. With a persistently
failing backing device, would each retry consume one more cluster until
the image or the refcount table is exhausted?

Nothing counts or reclaims these clusters, and no QC_ERR message is
logged, so the operator is not told that a check is needed. Would it make
sense to log when a cluster is intentionally leaked here?

The comment above complete_wbd() that says "clusters allocations
reverted" also looks stale now.

[ ... ]

> @@ -1415,6 +1381,8 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
>  				       &wait_list, &end_list);
>  		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
>  
> +		list_for_each_entry(qio, &end_list, link)
> +			qio->ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
>  		qcow2_dispatch_qios(qcow2, NULL, &wait_list);
>  		end_qios(&end_list, errno_to_blk_status(wbd->ret));

Clearing FREE_ALLOCATED_CLU here makes handle_cleanup_mask() skip its
mark_cluster_unused() call for COW qios on the failed end_list.

That removes the only remaining path that would free the COW-allocated
cluster. Is this loop always safe, given that qio->ext is dereferenced
for every qio on end_list? Can any qio on dependent_list have a NULL ext?

The commit message also says:

  "Keep the refcount of these clusters instead. If L1/L2 on disk points to
  such a cluster, the image stays consistent; otherwise the cluster is
  leaked, which check repairs by freeing it. So the image needs no check
  and resume needs no refusal."

Is this accurate? The message itself says a leaked cluster is repaired by
check, so the statement that the image needs no check appears to
contradict it. Could the message be reworded to say that the image stays
consistent but may need a check to reclaim leaked clusters?

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c

[ ... ]

> 	/*
> 	 * We need second iteration, since revert_l_entries_update()
> 	 * may start timer again after failed wb.
> 	 */
> 	for (i = 0; i < 2; i++) {

[Severity: Low]
This isn't a bug, but is this comment in qcow2_flush_deferred_activity()
now stale?

It says revert_l_entries_update() may start the slow-writeback timer again
after a failed writeback. That relied on revert_l_entries_update() calling
mark_cluster_unused(), which dirtied R2 pages and re-armed slow_wb_timer.
This patch removes that call, so revert_l_entries_update() only rewrites
L1/L2 entries in the page.

Timer restarts can still come from other end_qios()/qio_endio() cleanup
paths. Could the comment be rewritten to describe the current reason for
the second iteration?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-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 v10 07/10] dm-qcow2: flag the image for check if reftable relocation fails late

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dm-qcow2: relocate_refcount_table() late failure leaves half-updated reftable live
- [Low] dm-qcow2: relocate_refcount_table() logs stale ret at err_hdr_written
- [Low] dm-qcow2: relocate_refcount_table() doc comment still promises safe rollback on header write failure
--

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

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

This sets md_writeback_error and logs a message when
relocate_refcount_table() fails after the header write has been issued.
With the flag set, wants_check reports the image and resume is refused
until it is checked. The old err_eio label is replaced by a new
err_hdr_written label for the failures that happen after the header write.

> 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,10 @@ 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 write header with relocated refcount table: %d",
> +		       ret);
> +		qcow2->tgt->md_writeback_error = true;
>  		goto err_free_r2_pages;
>  	}

[Severity: Low]
This isn't a bug, but the block comment above relocate_refcount_table()
is now out of date:

 * Then, we try to write new hdr on disk. In case of failure, we
 * restore old hdr in memory and do safe rollback. In case of success,

A failure of the header write now also sets md_writeback_error, so the
image is reported by wants_check and resume is refused. Could the comment
be updated to say so?

The comment at the top of the function also says "We act in the way to
provide safe rollback throughout whole function", which is no longer
accurate.

[ ... ]

> @@ -2372,24 +2376,25 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)

[ ... ]

>  	/* Connect new R2 to new R1: one for each group new R1 and R2 span */
>  	group_size = (u64)qcow2->refblock_entries * clu_size;
>  	for (i = pos, r2_pos = end; i < r2_end; i += group_size, r2_pos += clu_size) {
>  		if (WARN_ON_ONCE(r2_pos >= r2_end))
> -			goto err_eio;
> +			goto err_hdr_written;

[Severity: Low]
Is ret assigned on this path before the jump? In
relocate_refcount_table(), the WARN_ON_ONCE(r2_pos >= r2_end) branch goes
to err_hdr_written without setting ret.

At that point ret holds either 0 (from the header write or from
calc_refcounters_map()) or the positive value left by handle_md_page() in
an earlier iteration.

The label then logs:

	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
	       ret ?: -ENOENT);

The ?: operator only replaces 0, so a stale positive ret is printed as if
it were an error code, for example "failed to fill relocated refcount
table: 1". Could this branch set ret to a real errno, as the other
converted err_eio sites do?

[ ... ]

> @@ -2408,12 +2413,18 @@ 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_eio;
> +			goto err_hdr_written;
>  		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>  	}
>  
>  	return 1;
>  
> +err_hdr_written:
> +	/* 0: md page is not cached */
> +	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
> +	       ret ?: -ENOENT);
> +	qcow2->tgt->md_writeback_error = true;
> +	return -EIO;

[Severity: Medium]
Does this path leave a half-updated reftable live while the target keeps
running?

When control reaches err_hdr_written, qcow2->hdr.refcount_table_offset and
refcount_table_clusters already point at the new reftable. Only some of the
R1 pages may have been renumbered. The new R2 clusters may not be linked
into R1. The new R1 and R2 clusters may not be marked used, and the old R1
clusters may or may not be marked unused. Nothing rolls this back.

The caller is allocate_cluster():

allocate_cluster()
  find_unused_cluster() returns -ENOENT
    relocate_refcount_table()
      -> err_hdr_written, returns -EIO
  if (ret <= 0) return ret;

The -EIO only fails the current qio. As far as I can see, md_writeback_error
is read only by the status output, qcow2_truncate_unused_tail() and
wants_check(). No allocator or map path reads it. Later qios can therefore
keep calling find_unused_cluster() and mark_cluster_used() against the
inconsistent refcounts.

Could those qios hand out clusters that belong to the new reftable or R2
area, or call relocate_refcount_table() again on the corrupt state and
overwrite metadata or data?

The commit message only claims that the image is flagged and that resume is
refused, and that claim holds. But the "/* TODO: switch to RO */" comment
now remains only on the err_free_r2_pages path. Should the late-failure
path also stop further allocations, for example by switching the target to
read-only?

>  err_free_r2_pages:
>  	for (i = new_md_start; i < new_md_end; i += PAGE_SIZE) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-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 v10 09/10] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dm-qcow2: ENOSPC redirty re-arms slow_wb_timer before qcow2_destroy()
--

Subject: Re: [PATCH] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

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

dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

This patch redirties an R1/R2 metadata page when its writeback fails with
-ENOSPC, so the slow writeback timer retries it. It also passes the errno
of a failed metadata write through complete_metadata_writeback() instead of
converting it to -EIO.

> 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

[ ... ]

> @@ -1357,6 +1357,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
>  		 * so here we care about R1R2 only.
>  		 */
>  		md->status |= MD_WRITEBACK_ERROR;
> +		if (ret == -ENOSPC && !READ_ONCE(qcow2->tgt->wants_suspend))
> +			qcow2_md_make_dirty(qcow2, md, true);

[Severity: High]
Can this redirty re-arm slow_wb_timer on a path that frees the qcow2
right afterwards?

The redirty is gated only on wants_suspend. Only qcow2_presuspend() sets
wants_suspend, but qcow2_flush_deferred_activity() is also called from
paths where it is false.

One example is qcow2_merge_backward_complete():

    qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
    qcow2->lower = NULL;
    ...
    qcow2_destroy(qcow2);

qcow2_flush_deferred_activity() runs only two iterations. If the
underlying fs is still full, the sequence is:

    iteration 0: R1/R2 write fails with -ENOSPC, page is redirtied
    iteration 1: timer handler fires, the retry fails with -ENOSPC again
    clear_writeback_status()
      qcow2_md_make_dirty(qcow2, md, true)
        list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list)
        mod_timer(&qcow2->slow_wb_timer, jiffies + WB_TIMEOUT_JI)

After the last iteration nothing drains the page or the timer.

qcow2_destroy() then only does:

    WARN_ON(!list_empty(&qcow2->paused_qios) ||
            !list_empty(&qcow2->wb_batch_list) ||
            !list_empty(&qcow2->slow_wb_batch_list) ||
            timer_pending(&qcow2->slow_wb_timer));
    qcow2_free_md_pages_tree(&qcow2->md_pages);
    ...
    kfree(qcow2);

It does not call timer_shutdown_sync() or timer_delete_sync(). Could
qcow2_slow_wb_timer_fn() then run on the freed struct qcow2 when the timer
fires, up to WB_TIMEOUT_JI later? It does timer_container_of(),
list_splice_init() and queue_work() on that memory. The WARN_ON would also
trigger.

The merge-error path in qcow2_merge_backward_work() calls
qcow2_flush_deferred_activity(tgt, lower) with wants_suspend false in the
same way. There the ENOSPC page is left redirtied and the timer re-armed
on an image that is still live but no longer being merged.

qcow2_tgt_destroy() also flushes and then calls qcow2_destroy(). It looks
protected only if presuspend ran first.

Before this patch, the completion handler never re-armed the timer after a
failed R1/R2 write, so this exposure is new.

The commit message says "A page that still fails then is not redirtied, as
wants_suspend is set, so nothing is left pending on a suspended device".
Is that true outside the suspend flush? As described above, the claim
holds only when wants_suspend is set.

Should the redirty be limited to cases where the flush cannot leave the
timer armed, or should the flush and destroy paths cancel the timer and
drain slow_wb_batch_list before the qcow2 is freed?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-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 v10 10/10] dm-qcow2: truncate unused tail of image on suspend

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dm-qcow2: used-end shrink trusts refcounts without any in-kernel crash guard
- [Medium] dm-qcow2: unvalidated on-disk refcounts steer destructive truncation in qcow2_calc_used_end()
- [Medium] dm-qcow2: GFP_KERNEL md page allocs in postsuspend scan outside NOIO scope
--

commit e35086f1c84ad69710c30b217b7a28fd6da3991c
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.
On postsuspend, qcow2_truncate_unused_tail() now truncates the image file to
that used end instead of only dropping the preallocated area. The scan is
skipped after an md writeback error, and for a dirty image when
kernel_sets_dirty_bit is set.

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> index 27ae22495f37..d96532b3753e 100644
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

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

[ ... ]

> +		if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
> +							r1.index_in_page)))
> +			return -EIO;

[Severity: Medium]
r1_entry_is_valid() only checks the reserved bits and the cluster alignment
of the R1 entry. Is the refblock offset checked anywhere against the file
size or against overlap with other metadata?

The open path does not check it either. qcow2_parse_header() and
qcow2_check_convert_hdr() do not validate R1 entries or refblock offsets.
The only reftable check there is:

    if (qcow2->reftable_max_file_size < qcow2->file_size)
            goto out;

If an R1 entry is zero or points at the wrong block, can the scan skip or
miss clusters that L2 still maps? The file would then be truncated below
live guest data.

A refblock beyond EOF fails the read, and the scan returns -EIO and falls
back to the old behaviour.

> +
> +		ret = calc_r2_page_id(qcow2, &r1, &r2);

[ ... ]

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

[Severity: Medium]
The used end comes only from the on-disk refcounts. There is no cross-check
against L1/L2 offsets. If a cluster is mapped by L2 but has refcount 0, for
example after a crash, doesn't this loop cut it off?

> +	}
> +
> +	return -EIO;
> +}

[ ... ]

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> index 1b7a75239291..7587e8f7ac0b 100644
> --- 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 (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);

[Severity: Medium]
The only in-kernel guard before this truncate is the opened_dirty check. It
applies only when kernel_sets_dirty_bit is set, which defaults to false. A
v2 image has no dirty bit, because qcow2_set_image_file_features() returns
early for version 2, so the guard can never apply to it.

The commit message says "the scan is safe only because userspace checks the
image before an RW open". Is that precondition enforced anywhere in the
kernel?

The dirty-bit consistency 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;

qcow2_wants_check() reflects only md_writeback_error and truncate_error from
the current session.

Suppose a crash leaves a refcount at 0 for a cluster that L2 already
references, and the image is opened RW without a userspace check. Will
qcow2_truncate_safe() then permanently cut off mapped clusters?

The new end is stored into file_preallocated_area_start and file_size, so
the loss cannot be recovered. The old code only dropped the preallocated
area, so it could not remove mapped data.

Could the kernel refuse the scan for images whose consistency it cannot
verify, or could the commit message explain how the userspace check is
enforced?

[ ... ]

> @@ -1157,7 +1170,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);

[Severity: Medium]
qcow2_truncate_unused_tail() now reaches qcow2_calc_used_end(), which loops
over qcow2_md_page_read_sync(). That calls qcow2_alloc_and_insert_md_page(),
which uses kmalloc(GFP_KERNEL) and alloc_page(GFP_KERNEL). The backing-file
read can also allocate memory.

The call chain is:

dm_suspend()->dm_table_postsuspend_targets()->qcow2_postsuspend()
  ->qcow2_truncate_unused_tail()->qcow2_calc_used_end()
    ->qcow2_md_page_read_sync()->qcow2_alloc_and_insert_md_page()

dm_suspend() does not wrap the postsuspend hooks in memalloc_noio_save().
Can direct reclaim triggered by these allocations wait on I/O to this same
suspended dm device, for example if swap is on it? That I/O could not
complete until resume, and resume is waiting for postsuspend.

The old postsuspend path allocated no md pages, so this exposure is new. I
did not find an in-tree user that swaps onto a qcow2 device, so this may be
theoretical.

Would wrapping qcow2_calc_used_end() or qcow2_postsuspend() in
memalloc_noio_save()/memalloc_noio_restore(), or using GFP_NOIO in
qcow2_alloc_and_insert_md_page(), be appropriate?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109115856.1362416-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 v10 07/10] dm-qcow2: flag the image for check if reftable relocation fails late

[Alexander Ivanov]

>> @@ -2351,6 +2351,10 @@ 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 write header with relocated refcount table: %d",
>> +		       ret);
>> +		qcow2->tgt->md_writeback_error = true;
>>   		goto err_free_r2_pages;
>>   	}
> [Severity: Low]
> This isn't a bug, but the block comment above relocate_refcount_table()
> is now out of date:
>
>   * Then, we try to write new hdr on disk. In case of failure, we
>   * restore old hdr in memory and do safe rollback. In case of success,
>
> A failure of the header write now also sets md_writeback_error, so the
> image is reported by wants_check and resume is refused. Could the comment
> be updated to say so?
>
> The comment at the top of the function also says "We act in the way to
> provide safe rollback throughout whole function", which is no longer
> accurate.
Fixed.
>> @@ -2372,24 +2376,25 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
> [ ... ]
>
>>   	/* Connect new R2 to new R1: one for each group new R1 and R2 span */
>>   	group_size = (u64)qcow2->refblock_entries * clu_size;
>>   	for (i = pos, r2_pos = end; i < r2_end; i += group_size, r2_pos += clu_size) {
>>   		if (WARN_ON_ONCE(r2_pos >= r2_end))
>> -			goto err_eio;
>> +			goto err_hdr_written;
> [Severity: Low]
> Is ret assigned on this path before the jump? In
> relocate_refcount_table(), the WARN_ON_ONCE(r2_pos >= r2_end) branch goes
> to err_hdr_written without setting ret.
>
> At that point ret holds either 0 (from the header write or from
> calc_refcounters_map()) or the positive value left by handle_md_page() in
> an earlier iteration.
>
> The label then logs:
>
> 	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
> 	       ret ?: -ENOENT);
>
> The ?: operator only replaces 0, so a stale positive ret is printed as if
> it were an error code, for example "failed to fill relocated refcount
> table: 1". Could this branch set ret to a real errno, as the other
> converted err_eio sites do?
Fixed.
>> @@ -2408,12 +2413,18 @@ 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_eio;
>> +			goto err_hdr_written;
>>   		mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
>>   	}
>>   
>>   	return 1;
>>   
>> +err_hdr_written:
>> +	/* 0: md page is not cached */
>> +	QC_ERR(qcow2->tgt->ti, "failed to fill relocated refcount table: %d",
>> +	       ret ?: -ENOENT);
>> +	qcow2->tgt->md_writeback_error = true;
>> +	return -EIO;
> [Severity: Medium]
> Does this path leave a half-updated reftable live while the target keeps
> running?
>
> When control reaches err_hdr_written, qcow2->hdr.refcount_table_offset and
> refcount_table_clusters already point at the new reftable. Only some of the
> R1 pages may have been renumbered. The new R2 clusters may not be linked
> into R1. The new R1 and R2 clusters may not be marked used, and the old R1
> clusters may or may not be marked unused. Nothing rolls this back.
>
> The caller is allocate_cluster():
>
> allocate_cluster()
>    find_unused_cluster() returns -ENOENT
>      relocate_refcount_table()
>        -> err_hdr_written, returns -EIO
>    if (ret <= 0) return ret;
>
> The -EIO only fails the current qio. As far as I can see, md_writeback_error
> is read only by the status output, qcow2_truncate_unused_tail() and
> wants_check(). No allocator or map path reads it. Later qios can therefore
> keep calling find_unused_cluster() and mark_cluster_used() against the
> inconsistent refcounts.
>
> Could those qios hand out clusters that belong to the new reftable or R2
> area, or call relocate_refcount_table() again on the corrupt state and
> overwrite metadata or data?
>
> The commit message only claims that the image is flagged and that resume is
> refused, and that claim holds. But the "/* TODO: switch to RO */" comment
> now remains only on the err_free_r2_pages path. Should the late-failure
> path also stop further allocations, for example by switching the target to
> read-only?
The late failure path is not a regression. Before this patch the same 
branches
went to err_eio, returned -EIO with no rollback, and left no trace at 
all. The patch
only flags the image and logs the failure, which is what the commit message
claims. These branches are also reachable only through a driver bug: 
every md
page they use is cached before the header write, and each failure is under
WARN_ON_ONCE. Switching the target to read-only would need a separate
"in-memory metadata is inconsistent" state and refusing writes on it, since
md_writeback_error is also set when in-memory state is consistent. That is
out of scope for this fix, so no changes here.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v10 08/10] dm-qcow2: leak clusters allocated for a failed L1/L2 writeback

[Alexander Ivanov]

>>   static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   {
>> +	struct page *pe_page = wbd->pe_page;
>> +	u64 old;
>> +	int i;
>>   
>>   	lockdep_assert_held(&qcow2->md_pages_lock);
>>   	for_each_set_bit(i, wbd->changed_indexes, LX_INDEXES_PER_PAGE) {
>>   		/* Here we restore prealloced and compressed clu mappings */
>> +		old = pe_page ? get_u64_from_be_page(pe_page, i) : 0;
>> +		set_u64_to_be_page(wbd->md->page, i, old);
>>   	}
>>   }
> [Severity: Medium]
> Does this leave the newly allocated clusters permanently allocated for the
> lifetime of the running instance?
>
> The data cluster from LU_WANTS_ALLOC and the new L2-table cluster from an
> L1 update are allocated in prepare_l_entry_update(), and
> mark_cluster_used() sets their refcount to 1.
>
> If the md page writeback then fails, complete_wbd() calls
> revert_l_entries_update(), which now only restores the in-memory entry
> from pe_page, or to 0. Nothing calls mark_cluster_unused() for these
> clusters any more, and no other path frees them.
>
> The guest can retry the failed write, and that retry allocates a new
> cluster because the old one is still marked used. With a persistently
> failing backing device, would each retry consume one more cluster until
> the image or the refcount table is exhausted?
>
> Nothing counts or reclaims these clusters, and no QC_ERR message is
> logged, so the operator is not told that a check is needed. Would it make
> sense to log when a cluster is intentionally leaked here?
>
> The comment above complete_wbd() that says "clusters allocations
> reverted" also looks stale now.
The leak is intentional. The failed write may have partially reached
the disk, so L1/L2 on disk may point to the allocated cluster. Freeing
it lets the allocator hand it to another offset, and two guest offsets
then share it, which corrupts the image. A leaked cluster is harmless,
and check reclaims it.

The retry concern is mostly theoretical. Refcounts reach the disk
before L1/L2 does, so on a persistently failing device the R2
writeback is expected to fail first. That sets MD_WRITEBACK_ERROR and
the image needs check anyway. A leak needs the R2 and data writes to
succeed while the L2 write fails, e.g. a transient ENOSPC on an L2
overwrite. Such a write is retried no more often than the ENOSPC
timeout. The refcount table is not exhausted either: it is relocated
and the file grows.

A message is not added: the image stays consistent, and space is the
only thing lost.

The stale comment above complete_wbd() is fixed, thanks.
>
>> @@ -1415,6 +1381,8 @@ static void complete_wbd(struct qcow2 *qcow2, struct wb_desc *wbd)
>>   				       &wait_list, &end_list);
>>   		spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
>>   
>> +		list_for_each_entry(qio, &end_list, link)
>> +			qio->ext->cleanup_mask &= ~FREE_ALLOCATED_CLU;
>>   		qcow2_dispatch_qios(qcow2, NULL, &wait_list);
>>   		end_qios(&end_list, errno_to_blk_status(wbd->ret));
> Clearing FREE_ALLOCATED_CLU here makes handle_cleanup_mask() skip its
> mark_cluster_unused() call for COW qios on the failed end_list.
>
> That removes the only remaining path that would free the COW-allocated
> cluster. Is this loop always safe, given that qio->ext is dereferenced
> for every qio on end_list? Can any qio on dependent_list have a NULL ext?
>
> The commit message also says:
>
>    "Keep the refcount of these clusters instead. If L1/L2 on disk points to
>    such a cluster, the image stays consistent; otherwise the cluster is
>    leaked, which check repairs by freeing it. So the image needs no check
>    and resume needs no refusal."
>
> Is this accurate? The message itself says a leaked cluster is repaired by
> check, so the statement that the image needs no check appears to
> contradict it. Could the message be reworded to say that the image stays
> consistent but may need a check to reclaim leaked clusters?
A leak needs the data write to succeed while the L2 overwrite fails,
e.g. with a transient ENOSPC. With the disk full, the data write fails
first and the cluster is freed as before. The image stays consistent;
only space is lost, so no message is added.
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
>
> [ ... ]
>
>> 	/*
>> 	 * We need second iteration, since revert_l_entries_update()
>> 	 * may start timer again after failed wb.
>> 	 */
>> 	for (i = 0; i < 2; i++) {
> [Severity: Low]
> This isn't a bug, but is this comment in qcow2_flush_deferred_activity()
> now stale?
>
> It says revert_l_entries_update() may start the slow-writeback timer again
> after a failed writeback. That relied on revert_l_entries_update() calling
> mark_cluster_unused(), which dirtied R2 pages and re-armed slow_wb_timer.
> This patch removes that call, so revert_l_entries_update() only rewrites
> L1/L2 entries in the page.
>
> Timer restarts can still come from other end_qios()/qio_endio() cleanup
> paths. Could the comment be rewritten to describe the current reason for
> the second iteration?
Rewrote the comment.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v10 09/10] dm-qcow2: retry R1/R2 writeback that failed with ENOSPC

[Alexander Ivanov]

>> @@ -1357,6 +1357,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
>>   		 * so here we care about R1R2 only.
>>   		 */
>>   		md->status |= MD_WRITEBACK_ERROR;
>> +		if (ret == -ENOSPC && !READ_ONCE(qcow2->tgt->wants_suspend))
>> +			qcow2_md_make_dirty(qcow2, md, true);
> [Severity: High]
> Can this redirty re-arm slow_wb_timer on a path that frees the qcow2
> right afterwards?
>
> The redirty is gated only on wants_suspend. Only qcow2_presuspend() sets
> wants_suspend, but qcow2_flush_deferred_activity() is also called from
> paths where it is false.
>
> One example is qcow2_merge_backward_complete():
>
>      qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
>      qcow2->lower = NULL;
>      ...
>      qcow2_destroy(qcow2);
>
> qcow2_flush_deferred_activity() runs only two iterations. If the
> underlying fs is still full, the sequence is:
>
>      iteration 0: R1/R2 write fails with -ENOSPC, page is redirtied
>      iteration 1: timer handler fires, the retry fails with -ENOSPC again
>      clear_writeback_status()
>        qcow2_md_make_dirty(qcow2, md, true)
>          list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list)
>          mod_timer(&qcow2->slow_wb_timer, jiffies + WB_TIMEOUT_JI)
>
> After the last iteration nothing drains the page or the timer.
>
> qcow2_destroy() then only does:
>
>      WARN_ON(!list_empty(&qcow2->paused_qios) ||
>              !list_empty(&qcow2->wb_batch_list) ||
>              !list_empty(&qcow2->slow_wb_batch_list) ||
>              timer_pending(&qcow2->slow_wb_timer));
>      qcow2_free_md_pages_tree(&qcow2->md_pages);
>      ...
>      kfree(qcow2);
>
> It does not call timer_shutdown_sync() or timer_delete_sync(). Could
> qcow2_slow_wb_timer_fn() then run on the freed struct qcow2 when the timer
> fires, up to WB_TIMEOUT_JI later? It does timer_container_of(),
> list_splice_init() and queue_work() on that memory. The WARN_ON would also
> trigger.
>
> The merge-error path in qcow2_merge_backward_work() calls
> qcow2_flush_deferred_activity(tgt, lower) with wants_suspend false in the
> same way. There the ENOSPC page is left redirtied and the timer re-armed
> on an image that is still live but no longer being merged.
>
> qcow2_tgt_destroy() also flushes and then calls qcow2_destroy(). It looks
> protected only if presuspend ran first.
>
> Before this patch, the completion handler never re-armed the timer after a
> failed R1/R2 write, so this exposure is new.
>
> The commit message says "A page that still fails then is not redirtied, as
> wants_suspend is set, so nothing is left pending on a suspended device".
> Is that true outside the suspend flush? As described above, the claim
> holds only when wants_suspend is set.
>
> Should the redirty be limited to cases where the flush cannot leave the
> timer armed, or should the flush and destroy paths cancel the timer and
> drain slow_wb_batch_list before the qcow2 is freed?
Fixed in the previous fixes.  The redirty is no longer gated on 
wants_suspend.
qcow2_flush_deferred_activity() sets md_wb_no_retry for its last 
iteration on
every caller (suspend, merge complete/error, tgt destroy). The md wb qio
drops its inflight ref only after do_md_page_write_complete(), so the 
retry's
completion runs while the flag is still set. A page that fails again is 
not redirtied,
and the timer is not re-armed before qcow2_destroy().
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Alexander Ivanov]

>> +/*
>> + * 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)
>> +{
> [ ... ]
>
>> +		if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
>> +							r1.index_in_page)))
>> +			return -EIO;
> [Severity: Medium]
> r1_entry_is_valid() only checks the reserved bits and the cluster alignment
> of the R1 entry. Is the refblock offset checked anywhere against the file
> size or against overlap with other metadata?
>
> The open path does not check it either. qcow2_parse_header() and
> qcow2_check_convert_hdr() do not validate R1 entries or refblock offsets.
> The only reftable check there is:
>
>      if (qcow2->reftable_max_file_size < qcow2->file_size)
>              goto out;
>
> If an R1 entry is zero or points at the wrong block, can the scan skip or
> miss clusters that L2 still maps? The file would then be truncated below
> live guest data.
>
> A refblock beyond EOF fails the read, and the scan returns -EIO and falls
> back to the old behaviour.
The scan trusts refcounts, as the rest of the driver does: normal I/O does
not check R1 entries against other metadata either. Checking R1 offsets
would not help much. A zeroed R1 entry looks the same as a range with
no refblock, and a valid R1 entry can still point at a refblock full of 
zeros.
Only a walk over all L2 tables would catch these. Refcounts that disagree
with L2 are left to the image check before an RW open, the same check
that covers refcounts left stale by a crash. I added this to the commit 
message.
>
>> +
>> +		ret = calc_r2_page_id(qcow2, &r1, &r2);
> [ ... ]
>
>> +		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);
>> +		}
> [Severity: Medium]
> The used end comes only from the on-disk refcounts. There is no cross-check
> against L1/L2 offsets. If a cluster is mapped by L2 but has refcount 0, for
> example after a crash, doesn't this loop cut it off?
The driver already trusts on-disk refcounts: find_unused_cluster() hands out
any refcount-0 cluster, so an image whose refcounts are below real usage is
corrupted by the first allocating write regardless of this patch. The 
only new
exposure is a session with no guest writes; that case is covered by the
opened_dirty guard with kernel_sets_dirty_bit, and by the userspace check
before RW open otherwise, as the commit message describes. Cross-checking
against L2 would mean walking every L2 table on each suspend and still
would not replace an image check.
>
>> +	}
>> +
>> +	return -EIO;
>> +}
> [ ... ]
>
>> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
>> index 1b7a75239291..7587e8f7ac0b 100644
>> --- 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 (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);
> [Severity: Medium]
> The only in-kernel guard before this truncate is the opened_dirty check. It
> applies only when kernel_sets_dirty_bit is set, which defaults to false. A
> v2 image has no dirty bit, because qcow2_set_image_file_features() returns
> early for version 2, so the guard can never apply to it.
>
> The commit message says "the scan is safe only because userspace checks the
> image before an RW open". Is that precondition enforced anywhere in the
> kernel?
>
> The dirty-bit consistency 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;
>
> qcow2_wants_check() reflects only md_writeback_error and truncate_error from
> the current session.
>
> Suppose a crash leaves a refcount at 0 for a cluster that L2 already
> references, and the image is opened RW without a userspace check. Will
> qcow2_truncate_safe() then permanently cut off mapped clusters?
>
> The new end is stored into file_preallocated_area_start and file_size, so
> the loss cannot be recovered. The old code only dropped the preallocated
> area, so it could not remove mapped data.
>
> Could the kernel refuse the scan for images whose consistency it cannot
> verify, or could the commit message explain how the userspace check is
> enforced?
>
The precondition is not enforced in the kernel, and the kernel cannot verify
it cheaply: in the default mode userspace sets the dirty bit on every RW
open, so v2 and v3 are equally indistinguishable, and the only reliable
check is a walk over all L2 tables on every suspend. It is not a new
precondition either. find_unused_cluster() reuses any cluster with zero
refcount and prepare_unused_cluster() punches a hole in it, so the scenario
you describe already destroys mapped data on the next allocation. The
scan widens the damage of a violated invariant, it doesn't introduce one.
>> @@ -1157,7 +1170,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);
> [Severity: Medium]
> qcow2_truncate_unused_tail() now reaches qcow2_calc_used_end(), which loops
> over qcow2_md_page_read_sync(). That calls qcow2_alloc_and_insert_md_page(),
> which uses kmalloc(GFP_KERNEL) and alloc_page(GFP_KERNEL). The backing-file
> read can also allocate memory.
>
> The call chain is:
>
> dm_suspend()->dm_table_postsuspend_targets()->qcow2_postsuspend()
>    ->qcow2_truncate_unused_tail()->qcow2_calc_used_end()
>      ->qcow2_md_page_read_sync()->qcow2_alloc_and_insert_md_page()
>
> dm_suspend() does not wrap the postsuspend hooks in memalloc_noio_save().
> Can direct reclaim triggered by these allocations wait on I/O to this same
> suspended dm device, for example if swap is on it? That I/O could not
> complete until resume, and resume is waiting for postsuspend.
>
> The old postsuspend path allocated no md pages, so this exposure is new. I
> did not find an in-tree user that swaps onto a qcow2 device, so this may be
> theoretical.
>
> Would wrapping qcow2_calc_used_end() or qcow2_postsuspend() in
> memalloc_noio_save()/memalloc_noio_restore(), or using GFP_NOIO in
> qcow2_alloc_and_insert_md_page(), be appropriate?
Not new: qcow2_preresume() already allocates md pages with GFP_KERNEL
via qcow2_parse_metadata() on the same suspended device, and postsuspend
already did fs I/O (truncate, header write).
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel