Re: [PATCH] md/raid5: complete discard bios while reshape is active

At 2026-07-12 00:27:27, [email protected] wrote:
>Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
>New issues:
>- [High] Completing blocking bios with BLK_STS_AGAIN violates block layer semantics and causes unexpected errors in synchronous paths.
>
>Pre-existing issues:
>- [High] make_discard_request() leaks the bio and causes an uninterruptible hang if REQ_NOWAIT is set.
>--
>
>--- Patch [1]: [PATCH] md/raid5: complete discard bios while reshape is active ---
>commit f117d558ba13364f82645f866dea16210cadce51
>Author: Genjian Zhang <[email protected]>
>
>md/raid5: complete discard bios while reshape is active
>
>This patch modifies the discard request handling to complete bios with
>BLK_STS_AGAIN when an mdraid reshape is in progress. This prevents callers
>from hanging in uninterruptible sleep waiting for a completion that never
>arrives.
>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5722,9 +5722,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
>>  	if (WARN_ON_ONCE(bi->bi_opf & REQ_NOWAIT))
>>  		return;
>
>[Severity: High]
>This isn't a bug introduced by this patch, but does this early return leak
>the bio and cause an uninterruptible hang if REQ_NOWAIT is set?
>
>If a caller submits a discard bio with REQ_NOWAIT, the driver encounters this
>warning and returns immediately without calling bio_endio(). Because the
>caller assumes the bio was successfully handled, could this lead to a
>resource leak and task hang?

Agreed — the early return without bio_endio() is a pre-existing issue in the same function. This patch intentionally focuses on the reshape case that we reproduced, where blkdiscard hangs in bio_await() even after reshape has finished. I will fix the REQ_NOWAIT path in v2 using bio_wouldblock_error(), consistent with raid5_make_request().

>
>>  
>> -	if (mddev->reshape_position != MaxSector)
>> +	if (mddev->reshape_position != MaxSector) {
>>  		/* Skip discard while reshape is happening */
>> +		bio_endio_status(bi, BLK_STS_AGAIN);
>
>[Severity: High]
>Does completing a blocking bio with BLK_STS_AGAIN violate block layer
>semantics?
>
>Since the REQ_NOWAIT bios are filtered out just above this check, this path
>is only reached by synchronous, blocking discard bios. Completing them with
>BLK_STS_AGAIN will propagate an unexpected -EAGAIN error to synchronous IO
>paths and userspace applications.
>
>Block APIs typically handle BLK_STS_NOTSUPP gracefully, but passing -EAGAIN
>back through synchronous paths might cause filesystem transaction aborts or
>userspace failures. Should this return BLK_STS_NOTSUPP instead?
>

I did not find a status that fits perfectly.

In my view, BLK_STS_NOTSUPP does not fit well here either: discard is supported on the array, just not while reshape is in progress. In addition, blk_ioctl_discard() treats -EOPNOTSUPP as success (err = 0), so returning BLK_STS_NOTSUPP would report success to userspace even though no discard was performed.

BLK_STS_DEV_RESOURCE (-EBUSY) may also be worth considering.

Although the details differ from raid10, I used BLK_STS_AGAIN for the same case ( discard during reshape）, consistent with raid10_handle_discard() returning -EAGAIN. The main goal remains completing the bio with a retryable error instead of hanging callers.

>>  		return;
>> +	}
>>  
>>  	if (!raid5_discard_limits(mddev, bi))
>>  		return;
>
>-- 
>Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Suggestions are welcome.

Thanks,
Genjian