* [Devel] [PATCH hci-8.1 v6 1/6] dm-qcow2: add qcow2_md_page_read_sync() helper
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:22 ` no-reply
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 2/6] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages Alexander Ivanov
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
Add a helper, which returns an up to date md page from md cache,
or allocates a new one, inserts it and reads it synchronously.
On read failure the page is erased from cache and freed.
Use it in qcow2_parse_header() instead of open-coded allocation
and read of page 0. It will also be used by a later patch in the series.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
drivers/md/dm-qcow2-target.c | 41 +++++++++++++++++++++++++++++++-----
drivers/md/dm-qcow2.h | 2 ++
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index be09615eb8e2..311a23d63a5c 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -418,6 +418,41 @@ int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index, struct md_pag
}
ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO);
+/*
+ * Returns up to date md page from cache, or allocates
+ * a new one and reads it synchronously.
+ *
+ * The caller must ensure no read of this page is in flight
+ * (e.g., the device is suspended): if the page is cached but
+ * not MD_UPTODATE yet, qcow2_md_page_find_or_postpone() returns
+ * NULL and the following insert fails.
+ */
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id, struct md_page **md)
+{
+ int ret;
+
+ spin_lock_irq(&qcow2->md_pages_lock);
+ *md = qcow2_md_page_find_or_postpone(qcow2, id, NULL);
+ spin_unlock_irq(&qcow2->md_pages_lock);
+ if (*md)
+ return 0;
+
+ ret = qcow2_alloc_and_insert_md_page(qcow2, id, md);
+ if (ret)
+ return ret;
+
+ ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);
+ spin_lock_irq(&qcow2->md_pages_lock);
+ if (ret)
+ qcow2_md_page_erase(qcow2, *md);
+ else
+ (*md)->status |= MD_UPTODATE;
+ spin_unlock_irq(&qcow2->md_pages_lock);
+ if (ret)
+ qcow2_free_md_page(*md);
+ return ret;
+}
+
static void inflight_ref_exit0(struct percpu_ref *ref)
{
struct qcow2_target *tgt = container_of(ref, struct qcow2_target,
@@ -851,13 +886,9 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
}
qcow2->file_preallocated_area_start = qcow2->file_size;
- ret = qcow2_alloc_and_insert_md_page(qcow2, 0, &md);
- if (ret)
- return ret;
- ret = qcow2_rw_page_sync(READ, qcow2, md->id, md->page);
+ ret = qcow2_md_page_read_sync(qcow2, 0, &md);
if (ret)
return ret;
- md->status |= MD_UPTODATE;
raw_hdr = kmap(md->page);
min_len = to_bytes(ti->len);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index d9b8c38e093f..69d3ae6fca8a 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -386,6 +386,8 @@ void do_qcow2_work(struct work_struct *ws);
void do_qcow2_fsync_work(struct work_struct *ws);
int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index,
struct md_page **md);
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id,
+ struct md_page **md);
struct md_page *qcow2_md_page_find_or_postpone(struct qcow2 *qcow2,
u64 id, struct qio **qio);
struct md_page *qcow2_md_page_renumber(struct qcow2 *qcow2, u64 id,
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* [Devel] [PATCH hci-8.1 v6 2/6] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 1/6] dm-qcow2: add qcow2_md_page_read_sync() helper Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:20 ` no-reply
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock Alexander Ivanov
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
QCOW2_FMT already adds the "dm-qcow2: <dev>: " prefix and the trailing
newline, so several messages were printed like:
dm-qcow2: <dev>: dm-qcow2: Can't parse metadata
and some of them were followed by an empty line. Drop the redundant
prefix and newlines.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
drivers/md/dm-qcow2-target.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 311a23d63a5c..b667adb5472a 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -192,7 +192,7 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
md = rb_entry(node, struct md_page, node);
/* FIXME: call md_make_dirty() and try once again? */
if (md->status & MD_WRITEBACK_ERROR) {
- QC_ERR(tgt->ti, "qcow2: Failed to write dirty pages");
+ QC_ERR(tgt->ti, "Failed to write dirty pages");
tgt->md_writeback_error = true;
break;
}
@@ -834,19 +834,19 @@ static int qcow2_parse_extensions(struct dm_target *ti, struct qcow2 *qcow2,
struct Qcow2BitmapHeaderExt bitmaps_ext;
if (ext.len != sizeof(bitmaps_ext)) {
- QC_ERR(ti, "unexpected bitmap extension size\n");
+ QC_ERR(ti, "unexpected bitmap extension size");
return -EINVAL;
}
if (offset + sizeof(bitmaps_ext) > end) {
- QC_ERR(ti, "unexpected end of extension area\n");
+ QC_ERR(ti, "unexpected end of extension area");
return -EINVAL;
}
memcpy(&bitmaps_ext, addr + offset, sizeof(bitmaps_ext));
ret = qcow2_parse_bitmap_ext(ti, qcow2, &bitmaps_ext);
if (ret) {
- QC_ERR(ti, "unable to pase bitmap extension\n");
+ QC_ERR(ti, "unable to parse bitmap extension");
return ret;
}
break;
@@ -879,7 +879,7 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
new_size = PAGE_ALIGN(qcow2->file_size);
ret = qcow2_truncate_safe(file, new_size);
if (ret) {
- QC_ERR(ti, "qcow2: Can't truncate file");
+ QC_ERR(ti, "Can't truncate file");
return ret;
} /* See md_page_read_complete() */
qcow2->file_size = new_size;
@@ -910,14 +910,14 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
ret = -EXDEV;
if (upper) {
if (upper->clu_size != qcow2->clu_size) {
- QC_ERR(ti, "dm-qcow2: clu_size mismatch\n");
+ QC_ERR(ti, "clu_size mismatch");
goto out;
}
if (upper->ext_l2 != qcow2->ext_l2) {
if (is_bottom && upper->ext_l2 && !qcow2->ext_l2) {
- QC_INFO(ti, "dm-qcow2: using bottom delta without ext_l2\n");
+ QC_INFO(ti, "using bottom delta without ext_l2");
} else {
- QC_ERR(ti, "dm-qcow2: upper delta without ext_l2 is unsupported\n");
+ QC_ERR(ti, "upper delta without ext_l2 is unsupported");
goto out; /* This is not supported yet */
}
}
@@ -939,11 +939,11 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
ext_end = PAGE_SIZE;
if (ext_end > PAGE_SIZE) {
- QC_INFO(ti, "extensions are too long, skipping them\n");
+ QC_INFO(ti, "extensions are too long, skipping them");
} else {
ret = qcow2_parse_extensions(ti, qcow2, ext_start, ext_end, raw_hdr);
if (ret) {
- QC_ERR(ti, "failed to parse extensions\n");
+ QC_ERR(ti, "failed to parse extensions");
goto out;
}
}
@@ -976,7 +976,7 @@ static int qcow2_parse_metadata(struct dm_target *ti, struct qcow2_target *tgt)
ret = 0;
out:
if (ret)
- QC_ERR(ti, "dm-qcow2: Can't parse metadata");
+ QC_ERR(ti, "Can't parse metadata");
return ret;
}
@@ -1057,7 +1057,7 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
ret = qcow2_truncate_safe(qcow2->file, end);
if (ret) {
- QC_ERR(ti, "dm-qcow2: Can't truncate preallocations");
+ QC_ERR(ti, "Can't truncate preallocations");
tgt->truncate_error = true;
return;
}
@@ -1143,7 +1143,7 @@ static void qcow2_postsuspend(struct dm_target *ti)
if (dm_table_get_mode(ti->table) & FMODE_WRITE) {
ret = qcow2_set_image_file_features(qcow2, false);
if (ret)
- QC_ERR(ti, "qcow2: Can't set features");
+ QC_ERR(ti, "Can't set features");
}
}
static int qcow2_preresume(struct dm_target *ti)
@@ -1152,7 +1152,7 @@ static int qcow2_preresume(struct dm_target *ti)
int ret = 0;
if (qcow2_wants_check(tgt)) {
- QC_ERR(ti, "qcow2: image check and target reload are required");
+ QC_ERR(ti, "image check and target reload are required");
return -EIO;
}
@@ -1173,7 +1173,7 @@ static int qcow2_preresume(struct dm_target *ti)
if (dm_table_get_mode(ti->table) & FMODE_WRITE) {
ret = qcow2_set_image_file_features(tgt->top, true);
if (ret)
- QC_ERR(ti, "qcow2: Can't set features");
+ QC_ERR(ti, "Can't set features");
}
if (!ret)
qcow2_set_wants_suspend(ti, false);
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* [Devel] [PATCH hci-8.1 v6 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 1/6] dm-qcow2: add qcow2_md_page_read_sync() helper Alexander Ivanov
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 2/6] dm-qcow2: drop duplicate prefix from QC_ERR/QC_INFO messages Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:21 ` no-reply
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 4/6] dm-qcow2: complete wbd before ending the data qio Alexander Ivanov
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
place_r2() links a new refblock into R1, but moves neither file_size
nor file_preallocated_area_start. The refblock stays at or above the
watermark if truncate_prealloc_safe() in allocate_cluster() fails
after place_r2(), or if a parallel mark_cluster_unused() lowers
free_cluster_search_pos before the second pass of
find_unused_cluster(). Truncation of preallocations on suspend then
cuts the file under the refblock: R1 on disk points past EOF, and
the next session gets -EIO on allocations in the range it covers.
Extend the file and raise the watermark to the end of the refblock
before linking it into R1, as allocate_cluster() does for data
clusters.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-qcow2-map.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..810dd22425ae 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -82,6 +82,7 @@ struct qcow2_bvec {
static int qcow2_handle_r1r2_maps(struct qcow2 *qcow2, loff_t pos, struct qio **qio,
struct qcow2_map_item *r1, struct qcow2_map_item *r2, bool compressed);
static int qcow2_punch_hole(struct file *file, loff_t pos, loff_t len);
+static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func);
static void handle_cleanup_mask(struct qio *qio);
static void process_read_qio(struct qcow2 *qcow2, struct qio *qio,
struct qcow2_map *map);
@@ -2088,6 +2089,7 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
struct qcow2_map_item *r2, loff_t r2_pos, struct qio **qio)
{
struct dm_target *ti = qcow2->tgt->ti;
+ loff_t end = r2_pos + qcow2->clu_size;
u64 page_id = r2_pos >> PAGE_SHIFT;
int ret;
@@ -2100,6 +2102,12 @@ static int place_r2(struct qcow2 *qcow2, struct qcow2_map_item *r1,
return ret;
}
+ ret = truncate_prealloc_safe(qcow2, end, __func__);
+ if (ret)
+ return ret;
+ if (end > qcow2->file_preallocated_area_start)
+ qcow2->file_preallocated_area_start = end;
+
ret = qcow2_alloc_and_insert_md_page(qcow2, page_id, &r2->md);
if (ret < 0) {
QC_ERR(ti, "Can't alloc: ret=%d, page_id=%llu", ret, page_id);
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* [Devel] [PATCH hci-8.1 v6 4/6] dm-qcow2: complete wbd before ending the data qio
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
` (2 preceding siblings ...)
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 3/6] dm-qcow2: extend the file in place_r2() before linking the refblock Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:23 ` no-reply
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty Alexander Ivanov
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
data_rw_complete() calls qio_endio() before complete_wbd(). qio_endio()
drops the inflight ref, so qcow2_flush_deferred_activity() may return
on suspend while complete_wbd() for a failed L1/L2 writeback is still
running in AIO completion context. revert_l_entries_update() then
marks clusters unused and dirties R2 after the flush, and the change
is never written back.
Complete the wbd first. The md writeback path already does it in this
order.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-qcow2-map.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 810dd22425ae..ef832813b718 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2884,13 +2884,14 @@ static void data_rw_complete(struct qio *qio)
spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
}
+ /* qio_endio() drops the inflight ref, which suspend waits for */
+ if (finalize_wbd)
+ complete_wbd(qcow2, wbd);
if (call_endio) {
if (bi_status != BLK_STS_OK)
qio->bi_status = bi_status;
qio_endio(qio);
}
- if (finalize_wbd)
- complete_wbd(qcow2, wbd);
}
static void perform_rw_mapped(struct qcow2_map *map, struct qio *qio)
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
` (3 preceding siblings ...)
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 4/6] dm-qcow2: complete wbd before ending the data qio Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:25 ` no-reply
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
With kernel_sets_dirty_bit, postsuspend clears the dirty bit
unconditionally. An image that is dirty at open, e.g. after a crash,
has unrepaired refcounts, which may be below real usage, yet it is
marked clean on disk after the first suspend. A userspace check before
the next open then has no reason to repair it.
Clear the bit only if it isn't set in the parsed header. The kernel
sets the bit in the raw header page only, so hdr.incompatible_features
keeps the state read from disk, which is reread on every preresume.
Clearing the bit of an image opened dirty is left to userspace check.
This only preserves the on-disk marker. The driver still accepts a
dirty image read-write and allocates from its refcounts, so
find_unused_cluster() may hand out clusters that L2 still references
until the image is checked.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-qcow2-target.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index b667adb5472a..eb56c8de65c3 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -676,7 +676,8 @@ 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->hdr.incompatible_features &
+ INCOMPATIBLE_FEATURES_DIRTY_BIT))
raw_hdr->incompatible_features &= ~dirty_mask;
}
kunmap(md->page);
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty Alexander Ivanov
@ 2026-10-22 12:25 ` no-reply
2026-10-22 13:15 ` Alexander Ivanov
0 siblings, 1 reply; 14+ messages in thread
From: no-reply @ 2026-10-22 12:25 UTC (permalink / raw)
To: Alexander Ivanov, andrey.zhadchenko; +Cc: devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] dm-qcow2: hdr.incompatible_features can't tell the driver's own dirty bit from a crash-left one
- [Low] dm-qcow2: stale hdr dirty bit makes the new clear-skip leave a clean lower image dirty
--
commit bfb5fe94dfe964378e82e4145f33599250b6d25a
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
dm-qcow2: keep the dirty bit of an image opened dirty
This patch changes qcow2_set_image_file_features() so that, with
kernel_sets_dirty_bit, the on-disk dirty bit is cleared only if it is not
set in the parsed header (qcow2->hdr.incompatible_features). The aim is to
keep the dirty marker on an image that was already dirty when opened.
Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
[ ... ]
> @@ -676,7 +676,8 @@ 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->hdr.incompatible_features &
> + INCOMPATIBLE_FEATURES_DIRTY_BIT))
> raw_hdr->incompatible_features &= ~dirty_mask;
> }
[Severity: Low]
Can qcow2->hdr.incompatible_features tell the driver's own dirty bit apart
from one left by a crash? The header is re-parsed from disk on every
preresume:
qcow2_preresume()
qcow2_free_md_pages_all(tgt);
...
qcow2_parse_metadata(ti, tgt);
...
qcow2_set_image_file_features(tgt->top, true);
If the kernel's bit is already on disk at parse time, the parsed header
reports it as dirty-at-open, and every later call with dirty=false skips
the clear.
One way this happens is a failed clear in postsuspend:
qcow2_postsuspend()
ret = qcow2_set_image_file_features(qcow2, false);
if (ret)
QC_ERR(ti, "Can't set features");
If the qcow2_rw_page_sync() write fails, the error is only logged and the
bit stays on disk. The next qcow2_preresume() then reads it back as dirty
at open, and the driver never clears it afterwards. A retried preresume
without an intervening postsuspend could do the same.
The new "Skip shrinking to used end of dirty image" branch in
qcow2_truncate_preallocations() from patch 6 tests the same parsed-header
bit. A falsely dirty image would also lose its tail truncation.
The commit message does not mention this behaviour. Could it be described,
or could the driver track separately whether the bit was set at open?
[Severity: Low]
Does the new condition also leave a clean lower image marked dirty after a
cancelled backward merge? In dm-qcow2-cmd.c, qcow2_merge_backward_work()
calls:
ret = qcow2_set_image_file_features(lower, true);
This sets the bit on lower's raw header page and on disk only. lower->hdr
is not updated.
If a cancel arrives while the merge is in the WAIT_COMPLETION state,
qcow2_merge_backward_cancel() does only this:
} else if (tgt->backward_merge.state == BACKWARD_MERGE_WAIT_COMPLETION) {
set_backward_merge_in_process(tgt, *tgt->backward_merge.pqcow2, false);
tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
It never clears lower's on-disk dirty bit. On the next resume,
qcow2_preresume() re-parses every image in the chain, so
lower->hdr.incompatible_features now contains the kernel's own bit.
A later merge_backward start can then go one of two ways:
- If it fails, the clear in qcow2_merge_backward_work() is skipped:
ret2 = qcow2_set_image_file_features(lower, false);
- If it completes, lower becomes the top image. The clear in
qcow2_postsuspend() is skipped for the same reason:
ret = qcow2_set_image_file_features(qcow2, false);
Before this patch the second case cleared the bit. With this patch the
image stays marked dirty on disk even though the kernel set the bit. The
stale on-disk bit after cancel already existed, but it used to be cleared
at the next postsuspend and is now permanent.
Userspace may then run an unnecessary check, or refuse to open the image.
The exposure needs kernel_sets_dirty_bit=1, a cancel in WAIT_COMPLETION, and
a later merge or suspend cycle.
Could the cancel path clear the bit, or could the driver record the open-time
dirty state separately from the re-parsed header?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022121706.1890593-1-alexander.ivanov@virtuozzo.com?part=5
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty
2026-10-22 12:25 ` no-reply
@ 2026-10-22 13:15 ` Alexander Ivanov
0 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 13:15 UTC (permalink / raw)
To: no-reply, andrey.zhadchenko; +Cc: devel
>> @@ -676,7 +676,8 @@ 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->hdr.incompatible_features &
>> + INCOMPATIBLE_FEATURES_DIRTY_BIT))
>> raw_hdr->incompatible_features &= ~dirty_mask;
>> }
> [Severity: Low]
> Can qcow2->hdr.incompatible_features tell the driver's own dirty bit apart
> from one left by a crash? The header is re-parsed from disk on every
> preresume:
>
> qcow2_preresume()
> qcow2_free_md_pages_all(tgt);
> ...
> qcow2_parse_metadata(ti, tgt);
> ...
> qcow2_set_image_file_features(tgt->top, true);
>
> If the kernel's bit is already on disk at parse time, the parsed header
> reports it as dirty-at-open, and every later call with dirty=false skips
> the clear.
>
> One way this happens is a failed clear in postsuspend:
>
> qcow2_postsuspend()
> ret = qcow2_set_image_file_features(qcow2, false);
> if (ret)
> QC_ERR(ti, "Can't set features");
>
> If the qcow2_rw_page_sync() write fails, the error is only logged and the
> bit stays on disk. The next qcow2_preresume() then reads it back as dirty
> at open, and the driver never clears it afterwards. A retried preresume
> without an intervening postsuspend could do the same.
>
> The new "Skip shrinking to used end of dirty image" branch in
> qcow2_truncate_preallocations() from patch 6 tests the same parsed-header
> bit. A falsely dirty image would also lose its tail truncation.
>
> The commit message does not mention this behaviour. Could it be described,
> or could the driver track separately whether the bit was set at open?
If the clear fails, the image stays dirty and gets checked before the next
open. That's safe, so I'll leave it.
> [Severity: Low]
> Does the new condition also leave a clean lower image marked dirty after a
> cancelled backward merge? In dm-qcow2-cmd.c, qcow2_merge_backward_work()
> calls:
>
> ret = qcow2_set_image_file_features(lower, true);
>
> This sets the bit on lower's raw header page and on disk only. lower->hdr
> is not updated.
>
> If a cancel arrives while the merge is in the WAIT_COMPLETION state,
> qcow2_merge_backward_cancel() does only this:
>
> } else if (tgt->backward_merge.state == BACKWARD_MERGE_WAIT_COMPLETION) {
> set_backward_merge_in_process(tgt, *tgt->backward_merge.pqcow2, false);
> tgt->backward_merge.state = BACKWARD_MERGE_STOPPED;
>
> It never clears lower's on-disk dirty bit. On the next resume,
> qcow2_preresume() re-parses every image in the chain, so
> lower->hdr.incompatible_features now contains the kernel's own bit.
>
> A later merge_backward start can then go one of two ways:
>
> - If it fails, the clear in qcow2_merge_backward_work() is skipped:
> ret2 = qcow2_set_image_file_features(lower, false);
>
> - If it completes, lower becomes the top image. The clear in
> qcow2_postsuspend() is skipped for the same reason:
> ret = qcow2_set_image_file_features(qcow2, false);
>
> Before this patch the second case cleared the bit. With this patch the
> image stays marked dirty on disk even though the kernel set the bit. The
> stale on-disk bit after cancel already existed, but it used to be cleared
> at the next postsuspend and is now permanent.
>
> Userspace may then run an unnecessary check, or refuse to open the image.
> The exposure needs kernel_sets_dirty_bit=1, a cancel in WAIT_COMPLETION, and
> a later merge or suspend cycle.
>
> Could the cancel path clear the bit, or could the driver record the open-time
> dirty state separately from the re-parsed header?
Will fix.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend
2026-10-22 12:17 [Devel] [PATCH hci-8.1 v6 0/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
` (4 preceding siblings ...)
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 5/6] dm-qcow2: keep the dirty bit of an image opened dirty Alexander Ivanov
@ 2026-10-22 12:17 ` Alexander Ivanov
2026-10-22 12:29 ` no-reply
5 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2026-10-22 12:17 UTC (permalink / raw)
To: andrey.zhadchenko; +Cc: devel
Discard punches holes in place of freed clusters, so they hold no
physical extents, but it doesn't change the file size. On postsuspend
we truncate the image file only down to file_preallocated_area_start,
i.e. we drop preallocations only. So if clusters at the end of the
file are freed (refcount dropped to zero), the file size stays the
same and the image never shrinks.
Add qcow2_calc_used_end(), which scans refcount blocks backward from
file_preallocated_area_start and returns the end of the last cluster
with non-zero refcount. Refcount ranges with no allocated refblock
are skipped at once. Metadata pages are taken from md cache or read
synchronously. This is called on suspended device after md writeback,
so R1 and R2 can't change under us. A failed relocation of the
reftable leaves file_preallocated_area_start beyond the space covered
by the reftable, so the scan starts at most from its end. If the
relocation fails after the new header is written, refcounts no longer
match the header, so set md_writeback_error there.
Truncate the file to the used end. Fall back to the old behaviour on
scan error and after any failed md writeback. A failed R1/R2 write
leaves refcounts on disk stale. A failed L1/L2 write reverts the
refcount of a newly allocated cluster to zero, while L1/L2 on disk may
still point to it. Mark such an L1/L2 page with MD_LX_REVERTED and
clear the mark on its next successful writeback, which rewrites the
whole page, e.g. when a request delayed on ENOSPC is resubmitted. On
md writeback flush a still marked page sets a new lx_writeback_error
flag, which is reported in get_errors and logged when the scan is
skipped. The flag is lost on target reload, after which the scan would
trust the reverted refcount, so include it in wants_check like
md_writeback_error: resume is refused until the image is checked and
the target reloaded.
Refcounts may also be below real usage after a crash, since both QEMU
with lazy_refcounts and this driver write L2 before refcounts. The scan
would then cut off clusters L2 still points to, even in a session
without guest writes. With kernel_sets_dirty_bit such an image has the
dirty bit set at open, and the disabled dirty bit check lets it be
opened RW, so skip the scan for it. Otherwise userspace sets the dirty
bit on every RW open, so the bit doesn't mean the image is
inconsistent. Userspace checks the image before opening it RW, which
find_unused_cluster() already relies on when it reuses clusters with
zero refcount.
Close and reload to RO go through postsuspend too. On remove,
__dm_destroy() calls the presuspend and postsuspend hooks if the live
table isn't suspended yet. It neither stops the queue nor waits for
I/O, but the device is idle there, because dm_lock_for_deletion()
refuses to remove an open device. On table swap, do_resume() suspends
the old (RW) table before dm_swap_table(). So the file is shrunk in
these cases as well.
https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
drivers/md/dm-qcow2-cmd.c | 4 +-
drivers/md/dm-qcow2-map.c | 96 +++++++++++++++++++++++++++++++++---
drivers/md/dm-qcow2-target.c | 40 ++++++++++++---
drivers/md/dm-qcow2.h | 6 ++-
4 files changed, 129 insertions(+), 17 deletions(-)
diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index 2865940cff30..1520733c962c 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -20,9 +20,9 @@ static int qcow2_get_errors(struct qcow2_target *tgt, char *result,
int ret;
ret = DMEMIT("wants_check=%d\nmd_writeback_error=%d\ntruncate_error=%d\n"
- "merge_backward_error=%d\n",
+ "merge_backward_error=%d\nlx_writeback_error=%d\n",
wants_check, tgt->md_writeback_error, tgt->truncate_error,
- tgt->backward_merge.error);
+ tgt->backward_merge.error, tgt->lx_writeback_error);
return ret ? 1 : 0;
}
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index ef832813b718..860095a59f4f 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1091,6 +1091,11 @@ static int calc_refcounters_map(struct qcow2 *qcow2, loff_t pos,
return -EIO;
}
+static bool r1_entry_is_valid(struct qcow2 *qcow2, u64 entry)
+{
+ return !(entry & R1_RESERVED_ZERO_MASK) && CLU_OFF(qcow2, entry) == 0;
+}
+
static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
struct qcow2_map_item *r2)
{
@@ -1098,8 +1103,7 @@ static int calc_r2_page_id(struct qcow2 *qcow2, struct qcow2_map_item *r1,
u32 bits = qcow2->refblock_bits;
loff_t pos;
- if (WARN_ON_ONCE((entry & R1_RESERVED_ZERO_MASK) ||
- (CLU_OFF(qcow2, entry) != 0)))
+ if (WARN_ON_ONCE(!r1_entry_is_valid(qcow2, entry)))
return -EIO;
/* The corresponding refcount block has not yet been allocated */
@@ -1363,6 +1367,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
if (cleared)
continue; /* no cluster was allocated */
+ /* L1/L2 on disk may point to the cluster */
+ wbd->md->status |= MD_LX_REVERTED;
spin_unlock(&qcow2->md_pages_lock);
pos &= ~LX_REFCOUNT_EXACTLY_ONE;
@@ -1386,6 +1392,8 @@ static void clear_writeback_status(struct qcow2 *qcow2, struct md_page *md,
lockdep_assert_held(&qcow2->md_pages_lock);
md->status &= ~(MD_WRITEBACK|MD_WRITEBACK_ERROR);
+ if (!ret)
+ md->status &= ~MD_LX_REVERTED;
list_splice_init(&md->wait_list, wait_list);
if (ret && !md->wbd) {
/*
@@ -2216,6 +2224,74 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
return do_ftruncate(file, new_len, 0);
}
+/*
+ * Returns the end of the last cluster, which is used according
+ * to refcount table, clamped to file_preallocated_area_start since
+ * the last cluster may be partial. Clusters freed by discard may
+ * leave the tail of file unused. Everything at or above
+ * file_preallocated_area_start is unused by construction, so the scan
+ * starts below it. Failed relocate_refcount_table() leaves it beyond
+ * reftable_max_file_size, so clamp the start to the reftable coverage.
+ * This must be called on suspended device after md writeback, so
+ * nobody changes R1 and R2.
+ */
+loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
+{
+ u32 clu_size = qcow2->clu_size;
+ struct qcow2_map_item r1, r2;
+ struct md_page *r1_md = NULL;
+ loff_t pos;
+ s32 i;
+ int ret;
+
+ pos = min(qcow2->file_preallocated_area_start,
+ qcow2->reftable_max_file_size);
+ pos = round_down(pos - 1, clu_size);
+ while (pos >= 0) {
+ if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
+ return -EIO;
+
+ if (!r1_md || r1_md->id != r1.page_id) {
+ ret = qcow2_md_page_read_sync(qcow2, r1.page_id, &r1_md);
+ if (ret)
+ return ret;
+ }
+ r1.md = r1_md;
+
+ /*
+ * The scan walks R1 entries normal I/O may never touch,
+ * so don't let calc_r2_page_id() WARN on a corrupted one.
+ */
+ if (!r1_entry_is_valid(qcow2, get_u64_from_be_page(r1_md->page,
+ r1.index_in_page)))
+ return -EIO;
+
+ ret = calc_r2_page_id(qcow2, &r1, &r2);
+ if (ret == -ENOENT) {
+ /* No R2 cluster, so all clusters it covers are unused */
+ pos = (loff_t)r1.index * qcow2->refblock_entries * clu_size;
+ pos -= clu_size;
+ continue;
+ }
+
+ if (ret < 0)
+ return ret;
+
+ ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md);
+ if (ret)
+ return ret;
+
+ for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
+ /* Last cluster is partial if size isn't cluster-aligned */
+ if (get_r2_entry(qcow2, r2.md, i))
+ return min_t(loff_t, pos + clu_size,
+ qcow2->file_preallocated_area_start);
+ }
+ }
+
+ return -EIO;
+}
+
static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func)
{
loff_t prealloc_len, max_prealloc_len = qcow2->reftable_max_file_size;
@@ -2387,21 +2463,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
if (!WARN_ON_ONCE(!md))
qcow2_md_make_dirty(qcow2, md, true);
if (!md)
- break; /* goto err_free_r2_pages */
+ break; /* goto err_hdr_written */
}
spin_unlock_irq(&qcow2->md_pages_lock);
if (i != nr_pages)
- goto err_free_r2_pages;
+ goto err_hdr_written;
/* Connect new R2 to new R1 */
for (i = end; i < r2_end; i += clu_size) {
if (calc_refcounters_map(qcow2, i, &r1, &r2) < 0) {
WARN_ON_ONCE(1);
- goto err_free_r2_pages;
+ goto err_hdr_written;
}
ret = handle_md_page(qcow2, r1.page_id, NULL, &md);
if (WARN_ON_ONCE(ret <= 0))
- goto err_free_r2_pages;
+ goto err_hdr_written;
spin_lock_irq(&qcow2->md_pages_lock);
set_u64_to_be_page(md->page, r1.index_in_page, i);
qcow2_md_make_dirty(qcow2, md, true);
@@ -2412,7 +2488,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
for (i = pos; i < r2_end; i += clu_size) {
ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
if (WARN_ON_ONCE(ret <= 0))
- goto err_free_r2_pages;
+ goto err_hdr_written;
mark_cluster_used(qcow2, r2.md, r2.index_in_page);
}
@@ -2420,12 +2496,16 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
for (i = old_pos; i < old_end; i += clu_size) {
ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
if (WARN_ON_ONCE(ret <= 0))
- goto err_free_r2_pages;
+ goto err_hdr_written;
mark_cluster_unused(qcow2, r2.md, r2.index_in_page, i);
}
return 1;
+err_hdr_written:
+ /* Header on disk points to new R1, refcounts may be inconsistent */
+ QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
+ qcow2->tgt->md_writeback_error = true;
err_free_r2_pages:
for (i = end; i < r2_end; i += clu_size) {
ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index eb56c8de65c3..832cdbc8de33 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)
{
struct rb_node *node;
+ bool wb_error = false;
struct md_page *md;
int i;
@@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
node; node = rb_next(node)) {
md = rb_entry(node, struct md_page, node);
/* FIXME: call md_make_dirty() and try once again? */
- if (md->status & MD_WRITEBACK_ERROR) {
- QC_ERR(tgt->ti, "Failed to write dirty pages");
- tgt->md_writeback_error = true;
- break;
- }
+ if (md->status & MD_WRITEBACK_ERROR)
+ wb_error = true;
+ if (md->status & MD_LX_REVERTED)
+ tgt->lx_writeback_error = true;
}
spin_unlock_irq(&qcow2->md_pages_lock);
+
+ if (wb_error) {
+ QC_ERR(tgt->ti, "Failed to write dirty pages");
+ tgt->md_writeback_error = true;
+ }
}
static void qcow2_flush_deferred_activity_all(struct qcow2_target *tgt)
@@ -1048,11 +1053,34 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
{
struct qcow2_target *tgt = to_qcow2_target(ti);
struct qcow2 *qcow2 = top_qcow2_protected(ti);
- loff_t end = qcow2->file_preallocated_area_start;
+ loff_t used_end, end = qcow2->file_preallocated_area_start;
int ret;
if (!(dm_table_get_mode(ti->table) & FMODE_WRITE))
return;
+
+ /*
+ * Refcounts don't match L1/L2 on disk after failed md writeback.
+ * They may also be below real usage in an image opened dirty,
+ * e.g. after a crash. If userspace sets the dirty bit, it is set
+ * on every RW open and userspace checks the image before that.
+ */
+ if (tgt->md_writeback_error) {
+ QC_INFO(ti, "Skip shrinking to used end after md writeback error");
+ } else if (tgt->lx_writeback_error) {
+ QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
+ } else if (kernel_sets_dirty_bit &&
+ (qcow2->hdr.incompatible_features &
+ INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
+ QC_INFO(ti, "Skip shrinking to used end of dirty image");
+ } else {
+ used_end = qcow2_calc_used_end(qcow2);
+ if (used_end < 0)
+ QC_ERR(ti, "Can't find used end: %pe", ERR_PTR(used_end));
+ else
+ end = used_end;
+ }
+
if (end == qcow2->file_size)
return;
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 69d3ae6fca8a..95d0ec09c169 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -134,6 +134,7 @@ struct md_page {
#define MD_DIRTY (1U << 1) /* Page contains changes and wants writeback */
#define MD_WRITEBACK (1U << 2) /* Writeback was submitted */
#define MD_WRITEBACK_ERROR (1U << 3) /* Last writeback failed with error */
+#define MD_LX_REVERTED (1U << 4) /* L1/L2 on disk may point to freed cluster */
unsigned int status;
struct page *page;
struct list_head wait_list;
@@ -200,6 +201,7 @@ struct qcow2_target {
bool service_operations_allowed;
bool wants_suspend;
bool md_writeback_error;
+ bool lx_writeback_error;
bool truncate_error;
bool event_enospc;
@@ -411,6 +413,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);
@@ -459,7 +462,8 @@ static inline bool fake_service_qio(struct qio *qio)
static inline bool qcow2_wants_check(struct qcow2_target *tgt)
{
- return !!(tgt->md_writeback_error|tgt->truncate_error);
+ return !!(tgt->md_writeback_error|tgt->lx_writeback_error|
+ tgt->truncate_error);
}
static inline bool qcow2_file_is_writable(struct qcow2 *qcow2)
--
2.53.0
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend
2026-10-22 12:17 ` [Devel] [PATCH hci-8.1 v6 6/6] dm-qcow2: truncate unused tail of image on suspend Alexander Ivanov
@ 2026-10-22 12:29 ` no-reply
0 siblings, 0 replies; 14+ messages in thread
From: no-reply @ 2026-10-22 12:29 UTC (permalink / raw)
To: andrey.zhadchenko, Alexander Ivanov; +Cc: devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-qcow2: shrink-to-used-end trusts stale refcounts of crash-dirty or v2 images
- [Medium] dm-qcow2: sticky lx_writeback_error set from a discarded image blocks resume
- [Medium] dm-qcow2: revert_l_entries_update() skips spin_lock() on WARN_ON_ONCE continue path
- [Medium] dm-qcow2: relocate_refcount_table() frees dirty R2 md pages still linked on slow_wb_batch_list
--
commit 782650161bf80812bb686df6d342136798d62726
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 from file_preallocated_area_start and finds the end of the last
cluster with a non-zero refcount. qcow2_truncate_preallocations() then
truncates the image file to that end. The scan is skipped after md
writeback errors, after a reverted L1/L2 writeback (new MD_LX_REVERTED
and lx_writeback_error), and for dirty images when kernel_sets_dirty_bit
is set.
Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171
> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c
[ ... ]
> @@ -1363,6 +1367,8 @@ static void revert_l_entries_update(struct qcow2 *qcow2, struct wb_desc *wbd)
> if (cleared)
> continue; /* no cluster was allocated */
>
> + /* L1/L2 on disk may point to the cluster */
> + wbd->md->status |= MD_LX_REVERTED;
> spin_unlock(&qcow2->md_pages_lock);
> pos &= ~LX_REFCOUNT_EXACTLY_ONE;
>
> /*
> * 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);
[Severity: Medium]
Can the continue taken on WARN_ON_ONCE(ret <= 0) leave this loop running
without md_pages_lock?
The lock is dropped just above with spin_unlock(), and it is only retaken
by the spin_lock() at the bottom of the loop body, which the continue
skips. qcow2_handle_r1r2_maps() can return a value <= 0 in several cases:
qcow2_handle_r1r2_maps()
__handle_r1r2_maps()
calc_refcounters_map() fails with -EIO
calc_r2_page_id() returns -EIO or -ENOENT
__handle_md_page() returns 0 when the page is not cached and
qio is NULL
"refblock=%llu, while no snapshots" returns -EIO
After the continue, the next iteration calls get_u64_from_be_page() and
set_u64_to_be_page() on wbd->md->page without the lock. The caller
complete_wbd() then does:
spin_lock_irqsave(&qcow2->md_pages_lock, flags);
revert_l_entries_update(qcow2, wbd);
clear_writeback_status(qcow2, wbd->md, wbd->ret, &wait_list, &end_list);
spin_unlock_irqrestore(&qcow2->md_pages_lock, flags);
So clear_writeback_status() also runs unlocked, and the final unlock is
unbalanced. The comment says R1/R2 should be cached, but nothing enforces
that. This patch adds the MD_LX_REVERTED assignment in this same
unlock window and relies on this error path. Should the lock be retaken
before the continue?
> @@ -2387,21 +2463,21 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
[ ... ]
> @@ -2412,7 +2488,7 @@ static int relocate_refcount_table(struct qcow2 *qcow2, struct qio **qio)
> for (i = pos; i < r2_end; i += clu_size) {
> ret = qcow2_handle_r1r2_maps(qcow2, i, NULL, &r1, &r2, false);
> if (WARN_ON_ONCE(ret <= 0))
> - goto err_free_r2_pages;
> + goto err_hdr_written;
> mark_cluster_used(qcow2, r2.md, r2.index_in_page);
> }
[ ... ]
> +err_hdr_written:
> + /* Header on disk points to new R1, refcounts may be inconsistent */
> + QC_ERR(qcow2->tgt->ti, "failed to relocate refcount table");
> + qcow2->tgt->md_writeback_error = true;
> err_free_r2_pages:
> for (i = end; i < r2_end; i += clu_size) {
> ret = handle_md_page(qcow2, i >> PAGE_SHIFT, NULL, &md);
> if (ret <= 0)
> break;
> 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);
> }
[Severity: Medium]
Can this cleanup free R2 md pages that are still linked on
slow_wb_batch_list?
Earlier in the function, mark_cluster_used() calls
qcow2_md_make_dirty(qcow2, r2_md, true). That sets MD_DIRTY, does
list_add_tail(&md->wb_link, &qcow2->slow_wb_batch_list), and may arm
slow_wb_timer. The R2 pages for the clusters in [end, r2_end) are the
ones the cleanup loop frees.
If a later WARN_ON_ONCE(ret <= 0) check fails, the code reaches
err_hdr_written and falls into err_free_r2_pages. There,
qcow2_md_page_erase() only does rb_erase(&md->node, &qcow2->md_pages).
qcow2_free_md_page() then frees the page. Neither one does
list_del(&md->wb_link) or clears MD_DIRTY.
The freed md_page stays on slow_wb_batch_list. The timer callback or the
worker can later splice that list and read md->status and md->wbd of the
freed md. Could the dirty pages be unlinked from the writeback list
before they are freed here? This patch routes more failure paths into
this shared cleanup.
> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -170,6 +170,7 @@ static void qcow2_free_md_pages_tree(struct rb_root *root)
> void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2)
[ ... ]
> @@ -191,13 +192,17 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
> node; node = rb_next(node)) {
> md = rb_entry(node, struct md_page, node);
> /* FIXME: call md_make_dirty() and try once again? */
> - if (md->status & MD_WRITEBACK_ERROR) {
> - QC_ERR(tgt->ti, "Failed to write dirty pages");
> - tgt->md_writeback_error = true;
> - break;
> - }
> + if (md->status & MD_WRITEBACK_ERROR)
> + wb_error = true;
> + if (md->status & MD_LX_REVERTED)
> + tgt->lx_writeback_error = true;
> }
[Severity: Medium]
Can this set the sticky tgt->lx_writeback_error from an image that is
about to be destroyed?
qcow2_flush_deferred_activity() is also called from a non-suspend path:
qcow2_merge_backward_complete()
qcow2_flush_deferred_activity(tgt, qcow2); /* Delayed md pages */
qcow2->lower = NULL;
...
qcow2_destroy(qcow2);
If a page of the image being removed from the chain is still marked
MD_LX_REVERTED, the target-wide flag is set. The image and its md pages
are then freed, but the flag stays set. Nothing clears it.
qcow2_wants_check() now includes the flag, so qcow2_preresume() fails
with:
if (qcow2_wants_check(tgt)) {
QC_ERR(ti, "image check and target reload are required");
return -EIO;
}
The surviving image is then refused resume, and
qcow2_truncate_preallocations() skips the shrink, even though the stale
page no longer exists. The existing md_writeback_error has the same
imprecision on this path, but is that intended for the new flag as well?
> @@ -1048,11 +1053,34 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
[ ... ]
> + if (tgt->md_writeback_error) {
> + QC_INFO(ti, "Skip shrinking to used end after md writeback error");
> + } else if (tgt->lx_writeback_error) {
> + QC_INFO(ti, "Skip shrinking to used end after reverted L1/L2 writeback");
> + } else if (kernel_sets_dirty_bit &&
> + (qcow2->hdr.incompatible_features &
> + INCOMPATIBLE_FEATURES_DIRTY_BIT)) {
> + 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;
> + }
[Severity: Medium]
Is this guard enough to protect against truncating below clusters that
L1/L2 still reference after a crash?
The only protection against stale refcounts is:
kernel_sets_dirty_bit && (qcow2->hdr.incompatible_features &
INCOMPATIBLE_FEATURES_DIRTY_BIT)
kernel_sets_dirty_bit is a static bool that defaults to false, so this
branch is inactive by default. In qcow2_check_convert_hdr(), v2 images
return early:
if (hdr->version == 2)
return 0;
hdr->incompatible_features = be64_to_cpu(raw_hdr->incompatible_features);
That return comes before incompatible_features is read, so the field
stays 0 and the guard can never fire for v2 images. The open-time
dirty-bit check in qcow2_check_convert_hdr() is commented out:
// if (!is_ro && kernel_sets_dirty_bit !=
// !(hdr->incompatible_features & INCOMPATIBLE_FEATURES_DIRTY_BIT))
// return kernel_sets_dirty_bit ? -EUCLEAN : -ENOLCK;
So the kernel does not enforce that the image was checked before an RW
open.
If refcounts are below real usage after a crash, qcow2_calc_used_end()
returns an end below clusters that L2 still points to. It is computed
purely from on-disk R2 entries. qcow2_truncate_safe() then cuts those
clusters off, even on a plain suspend, remove or table swap with no guest
writes in the session.
The commit message says find_unused_cluster() already relies on the same
userspace contract. That path is only reached once a guest write needs a
new cluster, while this truncation runs on every RW suspend, remove and
reload.
Should the shrink be skipped or enforced in-kernel for v2 images and for
the default kernel_sets_dirty_bit setup?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022121706.1890593-1-alexander.ivanov@virtuozzo.com?part=6
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
^ permalink raw reply [flat|nested] 14+ messages in thread