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

["李佑鸿 " <dayou5941@163.com>]

At 2026-09-17 15:52:40, "李佑鸿 " <dayou5941@163.com> wrote:
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>At 2026-09-12 22:04:12, "yu kuai" <yukuai@fygo.io> wrote:
>>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.
>>
>
>A dedicated llist would avoid two list_adds on the same sh->lru from plug and do_release_stripe(). 
>Last-ref enqueue still does not drop sh->count; the decrement happens later in __release_stripe(). 
>So at count == 1 the same stripe can sit on both the plug list and released_stripes. What follows is 
>that one undropped last-ref being released twice by __release_stripe(), not a separate bug.
>
>raid5_unplug() and release_stripe_list() both take device_lock, so they do not run __release_stripe() in parallel. 
>They run one after the other. The only difference is whether another get happens between the two drains.
>
>1. No get in between
>The first __release_stripe() does 1→0 and puts the stripe on inactive.
>The second does 0→-1. dec_and_test is false, so it does not enter do_release_stripe() 
>and there is no second list_add(lru). Count is already -1, so a later get_free_stripe() → init_stripe() hits the BUG.
>
>2. A get in between
>The first __release_stripe() does 1→0.
>The stripe is gotten again and count is 1; that is the new I/O's reference.
>The second __release_stripe() drops that new 1 to 0.
>
>The second drain is not necessarily unplug; release_stripe_list() running second is the same. 
>At drain time count == 1 does not tell you whether this is the parked last-ref or a ref after reuse.
>
>Enqueue to released_stripes happens only at count == 1. When count > 1, raid5_release_stripe() only does add_unless and returns; 
>it does not enqueue. If the stripe is already on released_stripes, a get raises count above 1, and then plug runs, both lists can be occupied. 
>That is two references and two drains each dropping one; only the drop to 0 does list_add(lru). Cases 1 and 2 are only about parking twice at count == 1.
>
>So an llist only fixes the lru collision. It does not fix last-ref ownership. I do not see a lockless way to fix that.
>
>Thanks,
>Li Youhong

>

Sorry for the noise on the last-ref / dual-list part.

I said a stripe at count == 1 could sit on both the plug list and 
released_stripes, so two later __release_stripe() calls would drop 
the same undropped last-ref. I do not have a code path that shows 
raid5_release_stripe() unpaired with get, so I should not have treated 
that as a reason to reject llist.

A dedicated llist still looks right: it stops plug and do_release_stripe() 
from list_add()ing the same sh->lru. I'll post a v2 that way.

Thanks, 
Li Youhong>