[Devel] [PATCH vz10 23/24] dm-qcow2: fail waiters instead of retrying forever after metadata I/O error

[Andrey Zhadchenko]

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 <khorenko at virtuozzo.com>
> ---
>   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,