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

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?

>  
> -	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?

>  		return;
> +	}
>  
>  	if (!raid5_discard_limits(mddev, bi))
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1