Re: [PATCH] md/raid5: serialize plug list add with device_lock

Hi,

在 2026/9/7 11:24, 李佑鸿 写道:

Hi Kuai,












At 2026-09-05 11:17:58, "yu kuai" [off-list ref] wrote:

quoted

Hi,

在 2026/9/2 17:53, Li Youhong 写道:

quoted

From: Li Youhong <redacted>

raid5_unplug() can spin forever under conf->device_lock when
raid5_plug_cb.list still points at a stripe whose sh->lru has
already been reinitialized (self-looped). That disables IRQs on
the holder CPU and causes multi-CPU hard lockups on waiters of
the same lock.

This happens because release_stripe_plug() sets
STRIPE_ON_UNPLUG_LIST and list_add_tail(sh->lru) without
device_lock, while do_release_stripe() may concurrently move the
same lru onto handle/inactive when the last reference drops.

Note: a 2020 proposal tried extra refs / checking
STRIPE_ON_UNPLUG_LIST in do_release_stripe() without serializing
the plug enqueue:
https://lore.kernel.org/linux-raid/20200108163023.9301-1-guoqing.jiang@cloud.ionos.com/
That still leaves a TOCTOU window where the bit is clear during
the check and set afterwards, allowing two list_add on the same
lru. It was not merged.

Serialize the bit update and list_add with device_lock. If
do_release_stripe() still sees STRIPE_ON_UNPLUG_LIST, restore the
reference and let raid5_unplug() own the final release.

Observed on production 9-disk NVMe RAID5 under MySQL AIO
(io_submit -> blk_finish_plug -> raid5_unplug).

Fixes: 8811b5968f62 ("raid5: make_request use batch stripe release")
Cc: stable@vger.kernel.org
Signed-off-by: Li Youhong <redacted>
---
   drivers/md/raid5.c | 33 ++++++++++++++++++++++++++++++---
   1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index b91545ce090d..c9197b63b1c4 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -229,6 +229,17 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh,
   	int i;
   	int injournal = 0;	/* number of date pages with R5_InJournal */
   
+	/*
+	 * Stripe is owned by release_stripe_plug()'s cb->list. A concurrent
+	 * last-ref release can reach here after the stripe was queued for
+	 * unplug (lru may already be non-empty). Do not re-add lru elsewhere;
+	 * restore the reference and let raid5_unplug() finish the release.
+	 */
+	if (test_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)) {
+		atomic_inc(&sh->count);
+		return;
+	}
+
   	BUG_ON(!list_empty(&sh->lru));
   	BUG_ON(atomic_read(&conf->active_stripes)==0);
   

@@ -5760,6 +5771,9 @@ static void release_stripe_plug(struct mddev *mddev,
   		raid5_unplug, mddev,
   		sizeof(struct raid5_plug_cb));
   	struct raid5_plug_cb *cb;
+	struct r5conf *conf = mddev->private;
+	unsigned long flags;
+	bool queued = false;
   
   	if (!blk_cb) {
   		raid5_release_stripe(sh);

@@ -5775,9 +5789,22 @@ static void release_stripe_plug(struct mddev *mddev,
   			INIT_LIST_HEAD(cb->temp_inactive_list + i);
   	}
   
-	if (!test_and_set_bit(STRIPE_ON_UNPLUG_LIST, &sh->state))
-		list_add_tail(&sh->lru, &cb->list);
-	else
+	/*
+	 * Serialize with do_release_stripe() on device_lock so sh->lru cannot
+	 * be added to handle/inactive and cb->list at the same time.
+	 */
+	spin_lock_irqsave(&conf->device_lock, flags);
+	if (!test_and_set_bit(STRIPE_ON_UNPLUG_LIST, &sh->state)) {
+		if (unlikely(!list_empty(&sh->lru))) {
+			clear_bit(STRIPE_ON_UNPLUG_LIST, &sh->state);
+		} else {
+			list_add_tail(&sh->lru, &cb->list);
+			queued = true;
+		}
+	}
+	spin_unlock_irqrestore(&conf->device_lock, flags);

Do you run some performance test on this patch? I believe this is from IO hot path,
a spinlock is not acceptable. I know there is already some spinlock for raid5, but
I'd like not to introduce new lock contention.

Can this problem be fixed by a new llist in stripe_head?

I agree that adding device_lock on this path can bring some
overhead; I will follow up with performance numbers.


Regarding the alternative: did you mean adding a dedicated
llist_node in stripe_head for the plug/unplug path (instead of
reusing sh->lru), so that release_stripe_plug() and
do_release_stripe() no longer share the same list node?


If we still queue the plug list via sh->lru, switching that list
to an llist would not remove the race, because do_release_stripe()
can still list_add the same lru onto handle/inactive concurrently.
A separate node would avoid that, but unplug would then need to
move stripes from the plug llist onto the existing lru-based
release path, which is a larger change.

I mean a new list for unplug in sh like unplug_llist. And I think it's
fine to use the llist directly for unplug, there is no need to move
sh from unplug llist to lru list first.

Please let me know if that matches what you had in mind.


Thanks,
Li Youhong>

quoted

quoted

+
+	if (!queued)
   		raid5_release_stripe(sh);
   }
   

-- 
Thanks,
Kuai

-- 
Thanks,
Kuai