Re:Re: [PATCH v2] md/raid5: use dedicated llist for stripe plug

[李佑鸿]

At 2026-10-09 11:12:33, "yu kuai" <yukuai@xxxxxxx> wrote:
>Hi,
>
>在 2026/9/22 14:09, Li Youhong 写道:
>> From: Li Youhong <liyouhong@xxxxxxxxxx>
>>
>> release_stripe_plug() and do_release_stripe() share sh->lru.
>> release_stripe_plug() sets STRIPE_ON_UNPLUG_LIST and
>> list_add_tail()s sh->lru onto raid5_plug_cb.list without
>> device_lock. do_release_stripe() holds device_lock and, when the
>> last reference drops, list_add()s the same lru onto a handle or
>> inactive list.
>>
>> Two list_add()s on one node corrupt it. sh->lru can be
>> reinitialized into a self-loop while raid5_plug_cb.list still
>> points at that stripe. raid5_unplug() then walks the list under
>> device_lock with IRQs disabled and never finishes. Other CPUs
>> waiting for the same lock hard-lockup.
>>
>> Add a dedicated llist_node, unplug_list, to stripe_head, as
>> release_list is used for released_stripes.
>>
>> Fixes: 8811b5968f62 ("raid5: make_request use batch stripe release")
>> Suggested-by: Yu Kuai <yukuai@xxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Li Youhong <liyouhong@xxxxxxxxxx>
>> ---
>> v2:
>> - Drop taking device_lock in release_stripe_plug(). Add a dedicated
>>    unplug_list.
>> - v1: link: https://lore.kernel.org/linux-raid/20260902095307.358569-1-dayou5941@xxxxxxx/
>>
>> ---
>>   drivers/md/raid5.c | 52 ++++++++++++++++++++++++++--------------------------
>>   drivers/md/raid5.h |  1 +
>>   2 files changed, 27 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index b91545ce090d..9dabbf9743d7 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5710,7 +5710,7 @@ static struct stripe_head *__get_priority_stripe(struct r5conf *conf, int group)
>>   
>>   struct raid5_plug_cb {
>>   	struct blk_plug_cb	cb;
>> -	struct list_head	list;
>> +	struct llist_head	unplug_list;
>>   	struct list_head	temp_inactive_list[NR_STRIPE_HASH_LOCKS];
>>   };
>>   
>> @@ -5718,34 +5718,33 @@ static void raid5_unplug(struct blk_plug_cb *blk_cb, bool from_schedule)
>>   {
>>   	struct raid5_plug_cb *cb = container_of(
>>   		blk_cb, struct raid5_plug_cb, cb);
>> -	struct stripe_head *sh;
>> +	struct stripe_head *sh, *tmp;
>>   	struct mddev *mddev = cb->cb.data;
>>   	struct r5conf *conf = mddev->private;
>> +	struct llist_node *head;
>>   	int cnt = 0;
>>   	int hash;
>>   
>> -	if (cb->list.next && !list_empty(&cb->list)) {
>> -		spin_lock_irq(&conf->device_lock);
>> -		while (!list_empty(&cb->list)) {
>> -			sh = list_first_entry(&cb->list, struct stripe_head, lru);
>> -			list_del_init(&sh->lru);
>> -			/*
>> -			 * avoid race release_stripe_plug() sees
>> -			 * STRIPE_ON_UNPLUG_LIST clear but the stripe
>> -			 * is still in our list
>> -			 */
>> -			smp_mb__before_atomic();
>> -			clear_bit(STRIPE_ON_UNPLUG_LIST, &sh->state);
>> -			/*
>> -			 * STRIPE_ON_RELEASE_LIST could be set here. In that
>> -			 * case, the count is always > 1 here
>> -			 */
>> -			hash = sh->hash_lock_index;
>> -			__release_stripe(conf, sh, &cb->temp_inactive_list[hash]);
>> -			cnt++;
>> -		}
>> -		spin_unlock_irq(&conf->device_lock);
>> +	head = llist_del_all(&cb->unplug_list);
>
>Is it possible that registered plug from task A can be flushed concurrent by unplug from
>another task, and later unplug_list is empty for task A's unplug. If so, a NULL check for

>head is needed here.


Hi Kuai,


A plug belongs to one task, in current->plug. blk_check_plugged()
searches that plug's cb_list by callback and data (the mddev), so each
task only flushes its own cbs. flush_plug_callbacks() also drops the cb
from cb_list before raid5_unplug(), and raid5_unplug() frees it. So
another task should not be able to flush this unplug_list.


An empty unplug_list can still happen, just not from that race.
blk_check_plugged() puts the new cb on cb_list before the stripe is
added. If STRIPE_ON_UNPLUG_LIST is already set, test_and_set_bit() fails
and we call raid5_release_stripe() instead of llist_add(). The stripe is
already queued on some other task's unplug_list, and this cb stays
registered with nothing on it. llist_del_all() then returns NULL.
llist_reverse_order(NULL) returns NULL, and llist_for_each_entry_safe()
does not walk a NULL node, so I think an extra NULL check is not
necessary.


Thanks,
Li Youhong

>
>> +	head = llist_reverse_order(head);
>> +	spin_lock_irq(&conf->device_lock);
>> +	llist_for_each_entry_safe(sh, tmp, head, unplug_list) {
>> +		/*
>> +		 * avoid race release_stripe_plug() sees
>> +		 * STRIPE_ON_UNPLUG_LIST clear but the stripe
>> +		 * is still in our list
>> +		 */
>> +		smp_mb__before_atomic();
>> +		clear_bit(STRIPE_ON_UNPLUG_LIST, &sh->state);
>> +		/*
>> +		 * STRIPE_ON_RELEASE_LIST could be set here. In that
>> +		 * case, the count is always > 1 here
>> +		 */
>> +		hash = sh->hash_lock_index;
>> +		__release_stripe(conf, sh, &cb->temp_inactive_list[hash]);
>> +		cnt++;
>>   	}
>> +	spin_unlock_irq(&conf->device_lock);
>>   	release_inactive_stripe_list(conf, cb->temp_inactive_list,
>>   				     NR_STRIPE_HASH_LOCKS);
>>   	if (!mddev_is_dm(mddev))
>> @@ -5768,15 +5767,16 @@ static void release_stripe_plug(struct mddev *mddev,
>>   
>>   	cb = container_of(blk_cb, struct raid5_plug_cb, cb);
>>   
>> -	if (cb->list.next == NULL) {
>> +	if (!cb->temp_inactive_list[0].next) {
>>   		int i;
>> -		INIT_LIST_HEAD(&cb->list);
>> +
>> +		init_llist_head(&cb->unplug_list);
>>   		for (i = 0; i < NR_STRIPE_HASH_LOCKS; i++)
>>   			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);
>> +		llist_add(&sh->unplug_list, &cb->unplug_list);
>>   	else
>>   		raid5_release_stripe(sh);
>>   }
>> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
>> index cb5feae04db2..e314f17eb949 100644
>> --- a/drivers/md/raid5.h
>> +++ b/drivers/md/raid5.h
>> @@ -201,6 +201,7 @@ struct stripe_head {
>>   	struct hlist_node	hash;
>>   	struct list_head	lru;	      /* inactive_list or handle_list */
>>   	struct llist_node	release_list;
>> +	struct llist_node	unplug_list;
>>   	struct r5conf		*raid_conf;
>>   	short			generation;	/* increments with every
>>   						 * reshape */
>
>-- 
>Thanks,
>Kuai