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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

On 11/9/26 15:16, no-reply@sashiko.virtuozzo.com wrote:
> 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@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?
>
Fixed the commit message.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel