Linux RAID subsystem development
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Abd-Alrhman Masalkhi" <abd.masalkhi@gmail.com>
Cc: yukuai@fygo.io, linux-raid@vger.kernel.org
Subject: Re: [PATCH] md/raid1: honor REQ_NOWAIT when waiting for behind writes
Date: Thu, 11 Jun 2026 08:49:53 +0000	[thread overview]
Message-ID: <20260611084953.E078E1F00898@smtp.kernel.org> (raw)
In-Reply-To: <20260611083514.754922-1-abd.masalkhi@gmail.com>

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 <abd.masalkhi@gmail.com>

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/20260611083514.754922-1-abd.masalkhi@gmail.com?part=1

  reply	other threads:[~2026-06-11  8:49 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-11  8:35 [PATCH] md/raid1: honor REQ_NOWAIT when waiting for behind writes Abd-Alrhman Masalkhi
2026-06-11  8:49 ` sashiko-bot [this message]
2026-06-11 10:49   ` Abd-Alrhman Masalkhi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260611084953.E078E1F00898@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=abd.masalkhi@gmail.com \
    --cc=linux-raid@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=yukuai@fygo.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox