Re: [PATCH] md/raid1: honor REQ_NOWAIT when waiting for behind writes

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Leaking pre-allocated `r1_bio` when `REQ_NOWAIT` reads fail `wait_read_barrier()` during retry.
- [High] Permanent barrier leak if a REQ_NOWAIT write encounters a blocked device.
- [Medium] REQ_NOWAIT semantics violation: memory allocations use GFP_NOIO unconditionally.
--

commit 7fed7389f4fe20e79eece974eaf3b8d317519e27
Author: Abd-Alrhman Masalkhi <[email protected]>

md/raid1: honor REQ_NOWAIT when waiting for behind writes

This commit ensures that raid1 consistently honors REQ_NOWAIT reads
across paths that may otherwise wait for behind writes. It modifies
bitmap_wait_behind_writes() to return early instead of sleeping if
the nowait flag is set.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 5b9368bd9e700..d03da046a5920 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -1360,8 +1361,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>  	 * Still need barrier for READ in case that whole
>  	 * array is frozen.
>  	 */
> -	if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> -				bio->bi_opf & REQ_NOWAIT)) {
> +	if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
>  		bio_wouldblock_error(bio);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this early return leak the 
pre-allocated r1_bio when wait_read_barrier() fails during a retry?

When a REQ_NOWAIT read fails its initial attempt, it is queued to the 
retry_list and handled by the raid1d thread via handle_read_error().

That function passes the existing, pre-allocated r1_bio back to 
raid1_read_request():

drivers/md/raid1.c:handle_read_error() {
    ...
    raid1_read_request(mddev, bio, r1_bio->sectors, r1_bio);
    allow_barrier(conf, sector);
    ...
}

If the array is concurrently frozen, wait_read_barrier() returns false 
since REQ_NOWAIT is still set on the master bio. The code handles this 
by returning immediately, bypassing the err_handle cleanup path and 
failing to call raid_end_bio_io().

Since handle_read_error() assumes ownership was taken and only releases 
its old barrier, does this result in a permanent memory leak of the 
r1_bio from the r1bio_pool mempool?

[Severity: Medium]
This is another pre-existing issue, but does the allocation path for 
REQ_NOWAIT bios violate block layer non-blocking semantics by 
unconditionally using GFP_NOIO?

Even when REQ_NOWAIT is requested, alloc_r1bio() uses GFP_NOIO 
unconditionally:

drivers/md/raid1.c:alloc_r1bio() {
    ...
    r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
    ...
}

Similarly, bio_alloc_clone() in raid1_read_request() unconditionally 
uses gfp flags that include __GFP_DIRECT_RECLAIM.

Since mempool-backed allocations will sleep under memory pressure 
instead of failing immediately when these flags are used, will this 
cause unexpected high latencies and potential priority inversion in 
asynchronous contexts?

[Severity: High]
This isn't a bug introduced by this patch, but does 
raid1_write_request() permanently leak the barrier if a REQ_NOWAIT 
write encounters a blocked device?

In raid1_write_request(), the code successfully acquires a barrier by 
calling wait_barrier(), which internally increments conf->nr_pending.

Immediately after, it calls wait_blocked_rdev():

drivers/md/raid1.c:raid1_write_request() {
    ...
    if (!wait_blocked_rdev(mddev, bio)) {
        bio_wouldblock_error(bio);
        return;
    }
    ...
}

If the device is blocked and the request is REQ_NOWAIT, 
wait_blocked_rdev() returns false and the function returns early. 
However, it fails to call allow_barrier() before returning to drop 
the previously acquired barrier increment.

Because freeze_array() waits for nr_pending to reach 0, could this 
permanently prevent the array from ever freezing again, deadlocking 
resyncs, reconfiguration, and array teardown?

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