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

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

[Li Youhong]

From: Li Youhong <liyouhong@kylinos.cn>

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 <liyouhong@kylinos.cn>
---
 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);
+
+	if (!queued)
 		raid5_release_stripe(sh);
 }
 
-- 
2.25.1

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

[yu kuai]

Hi,

在 2026/9/2 17:53, Li Youhong 写道:
> From: Li Youhong <liyouhong@kylinos.cn>
>
> 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 <liyouhong@kylinos.cn>
> ---
>   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?

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

-- 
Thanks,
Kuai

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

[李佑鸿]

Hi Kuai,












At 2026-09-05 11:17:58, "yu kuai" <yukuai@fygo.io> wrote:
>Hi,
>
>在 2026/9/2 17:53, Li Youhong 写道:
>> From: Li Youhong <liyouhong@kylinos.cn>
>>
>> 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 <liyouhong@kylinos.cn>
>> ---
>>   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.


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


Thanks,
Li Youhong>
>> +
>> +	if (!queued)
>>   		raid5_release_stripe(sh);
>>   }
>>   
>
>-- 
>Thanks,
>Kuai

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

[yu kuai]

Hi,

在 2026/9/7 11:24, 李佑鸿 写道:
>
>
>
> Hi Kuai,
>
>
>
>
>
>
>
>
>
>
>
>
> At 2026-09-05 11:17:58, "yu kuai" <yukuai@fygo.io> wrote:
>> Hi,
>>
>> 在 2026/9/2 17:53, Li Youhong 写道:
>>> From: Li Youhong <liyouhong@kylinos.cn>
>>>
>>> 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 <liyouhong@kylinos.cn>
>>> ---
>>>    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>
>>> +
>>> +	if (!queued)
>>>    		raid5_release_stripe(sh);
>>>    }
>>>    
>> -- 
>> Thanks,
>> Kuai

-- 
Thanks,
Kuai