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 <[email protected]> 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/[email protected]?part=9 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
