Metadata writeback is critical. Really long time ago we discussed what
we should actually do if it fails. There was some talks about suspending
the device to indicate some broken state or something about that.
This is the situation that should be immediately visible in the system.
Silently returning the error is a very bad approach. I don't like
endless retries either way, but at least this will catch some attention.
Also this would be again discussed when we add postponed metadata writeback.
It is not like we have something good already going, but I think this
patch will make the current state even worse.
On 7/6/26 13:00, Konstantin Khorenko wrote:
clear_writeback_status() unconditionally re-dispatched md->wait_list (qios
postponed by delay_if_dirty()/delay_if_writeback()/delay_if_locked()/
delay_if_wpc_readers_locked() while an L1/L2/R1/R2 metadata page was
dirty/under writeback/locked) for retry, regardless of whether the writeback
that just completed succeeded or failed.
On a persistently failing backing file (a real I/O error, e.g. the backing
device going bad or being remapped to an error target), the retried qio
re-marks the same metadata page dirty, its writeback is submitted again,
fails again with the same error, and clear_writeback_status() requeues the
waiters again - an unbounded retry storm. The write() that originally
triggered the cluster allocation stays blocked in submit_bio_wait() forever:
that wait is uninterruptible (TASK_UNINTERRUPTIBLE), so the process cannot
be killed and sits in D state permanently. Live-reproduced: a write to a
qcow2 image backed by a dm-error target hung indefinitely, the kernel
logging "can't sync md: -5" every ~200ms, while `dmsetup remove`/`ploop
umount` and eventually an orderly `reboot` all hang on the stuck task.
A metadata write failure means the page's dirty state was not persisted, so
retrying it as if nothing happened is not safe or productive. Route
md->wait_list to end_list (fail the waiters with the write's error) on
failure, exactly like the analogous md->wbd->dependent_list handling a few
lines below already does. The success path (ret == 0) is unchanged.
Fixes: 59d2cb3de264 ("dm-qcow2: Introduce driver to create block devices over QCOW2
files")
Feature: dm-qcow2: block device over QCOW2 files driver
https://virtuozzo.atlassian.net/browse/VSTOR-137234
Signed-off-by: Konstantin Khorenko <[email protected]>
---
drivers/md/dm-qcow2-map.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index db21efb45e17..ed01902be184 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -1344,7 +1344,22 @@ 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);
- list_splice_init(&md->wait_list, wait_list);
+ /*
+ * Qios postponed on this page (delay_if_dirty()/delay_if_writeback()/
+ * delay_if_locked()/delay_if_wpc_readers_locked()) were waiting for
+ * this writeback to finish before retrying their own metadata
+ * update. If it failed, this page's on-disk state was not
+ * persisted, so retrying is not safe: on a persistently failing
+ * backing file (I/O error) it would immediately redirty and
+ * rewrite the very page, fail again, and requeue itself here again
+ * -- an unbounded retry storm that keeps the originating I/O
+ * (and its uninterruptible submit_bio_wait() caller) blocked
+ * forever. Fail them instead, same as the wbd->dependent_list below.
+ */
+ if (likely(ret == 0))
+ list_splice_init(&md->wait_list, wait_list);
+ else
+ list_splice_init(&md->wait_list, end_list);
if (ret && !md->wbd) {
/*
* L1L2 updates can do safe revert,
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
