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

["yu kuai" <yukuai@fygo.io>]

Hi,

在 2026/9/22 14:09, Li Youhong 写道:
> From: Li Youhong <liyouhong@kylinos.cn>
>
> 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@fygo.io>
> Cc: stable@vger.kernel.org
> Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
> ---
> 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@163.com/
>
> ---
>   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.

> +	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