* [PATCH v2] md/raid5: use dedicated llist for stripe plug
@ 2026-09-22 6:09 Li Youhong
2026-10-09 3:12 ` yu kuai
0 siblings, 1 reply; 3+ messages in thread
From: Li Youhong @ 2026-09-22 6:09 UTC (permalink / raw)
To: song, yukuai
Cc: magiclinan, xiao, linux-raid, linux-kernel, Li Youhong, stable
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);
+ 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 */
--
2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] md/raid5: use dedicated llist for stripe plug
2026-09-22 6:09 [PATCH v2] md/raid5: use dedicated llist for stripe plug Li Youhong
@ 2026-10-09 3:12 ` yu kuai
2026-10-09 7:24 ` 李佑鸿
0 siblings, 1 reply; 3+ messages in thread
From: yu kuai @ 2026-10-09 3:12 UTC (permalink / raw)
To: Li Youhong, song, yu kuai
Cc: magiclinan, xiao, linux-raid, linux-kernel, Li Youhong, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re:Re: [PATCH v2] md/raid5: use dedicated llist for stripe plug
2026-10-09 3:12 ` yu kuai
@ 2026-10-09 7:24 ` 李佑鸿
0 siblings, 0 replies; 3+ messages in thread
From: 李佑鸿 @ 2026-10-09 7:24 UTC (permalink / raw)
To: yukuai
Cc: song, magiclinan, xiao, linux-raid, linux-kernel, Li Youhong, stable
At 2026-10-09 11:12:33, "yu kuai" <yukuai@fygo.io> wrote:
>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.
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-09 7:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-22 6:09 [PATCH v2] md/raid5: use dedicated llist for stripe plug Li Youhong
2026-10-09 3:12 ` yu kuai
2026-10-09 7:24 ` 李佑鸿
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®