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

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-qcow2: flush gives failed R1/R2 page two retries, not one
--

commit 2997fd4391d8fcb3d55f988df71bac728d0346e1
Author: Alexander Ivanov <alexander.ivanov at virtuozzo.com>

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

This patch redirties an R1/R2 metadata page whose writeback failed with
-ENOSPC, so the slow writeback timer retries it. It adds a flag,
md_wb_no_retry, that qcow2_flush_deferred_activity() sets on its second
pass to stop further redirtying. 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-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c

[ ... ]

> @@ -173,7 +173,9 @@ void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2
>  	struct md_page *md;
>  	int i;
>  
> +	/* Retry R1/R2 writeback failed with ENOSPC once, then give up */
>  	for (i = 0; i < 2; i++) {
> +		WRITE_ONCE(qcow2->md_wb_no_retry, i == 1);
>  		if (timer_delete_sync_try(&qcow2->slow_wb_timer) != -1)
>  			qcow2_slow_wb_timer_fn(&qcow2->slow_wb_timer);

[Severity: Low]
This isn't a functional bug, but the comment and the commit message don't
match what the loop does.

Consider a page that failed with -ENOSPC at runtime and was redirtied onto
slow_wb_batch_list:

  qcow2_flush_deferred_activity()
    pass i == 0: md_wb_no_retry is false, the timer is fired and the page
    is rewritten
      clear_writeback_status()
        if (ret == -ENOSPC && !READ_ONCE(qcow2->md_wb_no_retry))
                qcow2_md_make_dirty(qcow2, md, true);
    pass i == 1: md_wb_no_retry is true, the timer is fired again and the
    page is rewritten a second time

If the write in pass 0 fails with -ENOSPC again, the page is redirtied. So
a page that had already failed gets two flush-time attempts. Only the
failure in pass 1 is final.

The code comment says "Retry R1/R2 writeback failed with ENOSPC once, then
give up". The commit message says the flush "gives the page one more try"
and that "A page that still fails then is not redirtied".

Could the comment and the commit message be reworded to say that the flush
makes up to two attempts, or could the loop be changed so that only one
attempt is made?

[ ... ]

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