[PATCH] md: don't flush md_misc_wq from md_alloc()

[PATCH] md: don't flush md_misc_wq from md_alloc()

[Li Youhong]

From: Li Youhong <liyouhong@kylinos.cn>

md_alloc() is called from md_probe(), while blk_probe_dev() still holds
major_names_lock. The flush of md_misc_wq in md_alloc() is only meant to
wait for the previous mddev_delayed_delete() to finish.

md_misc_wq also runs sync_work (md_start_sync), and md_start_sync takes
reconfig_mutex. On the other path, md_ioctl() already holds
reconfig_mutex when md_import_device() opens a bdev and takes
major_names_lock.

Flushing md_misc_wq under major_names_lock therefore creates a lockdep
cycle:

  major_names_lock -> md_misc_wq -> reconfig_mutex -> major_names_lock

Move del_work to a dedicated workqueue and flush only that queue from
md_alloc(). Leave sync_work on md_misc_wq.

Reported-by: syzbot+68e1f51046d68329940f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=68e1f51046d68329940f
Fixes: e804ac780e2f ("md: fix and update workqueue usage")
Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
---
 drivers/md/md.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 680b34a63cb3..87a851466892 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -86,13 +86,16 @@ static const struct kobj_type md_ktype;
 static DECLARE_WAIT_QUEUE_HEAD(resync_wait);
 
 /*
- * This workqueue is used for sync_work to register new sync_thread, and for
- * del_work to remove rdev, and for event_work that is only set by dm-raid.
+ * md_misc_wq runs sync_work (new sync_thread) and event_work (dm-raid).
+ * sync_work grabs reconfig_mutex, so this queue must not be flushed while
+ * holding reconfig_mutex, nor while holding major_names_lock (md_probe /
+ * md_alloc is called from blk_probe_dev with that lock held).
  *
- * Noted that sync_work will grab reconfig_mutex, hence never flush this
- * workqueue whith reconfig_mutex grabbed.
+ * md_del_wq runs only del_work (mddev_delayed_delete). md_alloc() flushes
+ * this queue to wait for a previous instance of the same device to go away.
  */
 static struct workqueue_struct *md_misc_wq;
+static struct workqueue_struct *md_del_wq;
 
 static int remove_and_add_spares(struct mddev *mddev,
 				 struct md_rdev *this);
@@ -651,7 +654,7 @@ static void __mddev_put(struct mddev *mddev)
 	 * Call queue_work inside the spinlock so that flush_workqueue() after
 	 * mddev_find will succeed in waiting for the work to be done.
 	 */
-	queue_work(md_misc_wq, &mddev->del_work);
+	queue_work(md_del_wq, &mddev->del_work);
 }
 
 static void mddev_put_locked(struct mddev *mddev)
@@ -6329,9 +6332,12 @@ struct mddev *md_alloc(dev_t dev, char *name)
 
 	/*
 	 * Wait for any previous instance of this device to be completely
-	 * removed (mddev_delayed_delete).
+	 * removed (mddev_delayed_delete). Do not flush md_misc_wq: it also
+	 * runs sync_work, which takes reconfig_mutex and can take
+	 * major_names_lock via md_import_device(). md_alloc() is called from
+	 * md_probe() while blk_probe_dev() already holds major_names_lock.
 	 */
-	flush_workqueue(md_misc_wq);
+	flush_workqueue(md_del_wq);
 
 	mutex_lock(&disks_mutex);
 	mddev = mddev_alloc(dev);
@@ -10765,6 +10771,10 @@ static int __init md_init(void)
 	if (!md_misc_wq)
 		goto err_misc_wq;
 
+	md_del_wq = alloc_workqueue("md_del", WQ_PERCPU, 0);
+	if (!md_del_wq)
+		goto err_del_wq;
+
 	ret = __register_blkdev(MD_MAJOR, "md", md_probe);
 	if (ret < 0)
 		goto err_md;
@@ -10783,6 +10793,8 @@ static int __init md_init(void)
 err_mdp:
 	unregister_blkdev(MD_MAJOR, "md");
 err_md:
+	destroy_workqueue(md_del_wq);
+err_del_wq:
 	destroy_workqueue(md_misc_wq);
 err_misc_wq:
 	md_llbitmap_exit();
@@ -11093,6 +11105,7 @@ static __exit void md_exit(void)
 	spin_unlock(&all_mddevs_lock);
 
 	destroy_workqueue(md_misc_wq);
+	destroy_workqueue(md_del_wq);
 	md_bitmap_exit();
 }
 
-- 
2.43.0

Re: [PATCH] md: don't flush md_misc_wq from md_alloc()

[yu kuai]

Hi,

在 2026/9/15 16:33, Li Youhong 写道:
> From: Li Youhong <liyouhong@kylinos.cn>
>
> md_alloc() is called from md_probe(), while blk_probe_dev() still holds
> major_names_lock. The flush of md_misc_wq in md_alloc() is only meant to
> wait for the previous mddev_delayed_delete() to finish.
>
> md_misc_wq also runs sync_work (md_start_sync), and md_start_sync takes
> reconfig_mutex. On the other path, md_ioctl() already holds
> reconfig_mutex when md_import_device() opens a bdev and takes
> major_names_lock.
>
> Flushing md_misc_wq under major_names_lock therefore creates a lockdep
> cycle:
>
>    major_names_lock -> md_misc_wq -> reconfig_mutex -> major_names_lock
>
> Move del_work to a dedicated workqueue and flush only that queue from
> md_alloc(). Leave sync_work on md_misc_wq.
>
> Reported-by: syzbot+68e1f51046d68329940f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=68e1f51046d68329940f
> Fixes: e804ac780e2f ("md: fix and update workqueue usage")
> Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
> ---
>   drivers/md/md.c | 28 +++++++++++++++++++++-------
>   1 file changed, 21 insertions(+), 7 deletions(-)

Patch itself look correct, however, this is not the solution I'd like.
Since legacy_async_del_gendisk is introduced a long time now, I think it's
time to remove the related legacy async code, so there is no need to keep
del_work, and then the flush_workqueue() from md_alloc() is not needed anymore.

>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3..87a851466892 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -86,13 +86,16 @@ static const struct kobj_type md_ktype;
>   static DECLARE_WAIT_QUEUE_HEAD(resync_wait);
>   
>   /*
> - * This workqueue is used for sync_work to register new sync_thread, and for
> - * del_work to remove rdev, and for event_work that is only set by dm-raid.
> + * md_misc_wq runs sync_work (new sync_thread) and event_work (dm-raid).
> + * sync_work grabs reconfig_mutex, so this queue must not be flushed while
> + * holding reconfig_mutex, nor while holding major_names_lock (md_probe /
> + * md_alloc is called from blk_probe_dev with that lock held).
>    *
> - * Noted that sync_work will grab reconfig_mutex, hence never flush this
> - * workqueue whith reconfig_mutex grabbed.
> + * md_del_wq runs only del_work (mddev_delayed_delete). md_alloc() flushes
> + * this queue to wait for a previous instance of the same device to go away.
>    */
>   static struct workqueue_struct *md_misc_wq;
> +static struct workqueue_struct *md_del_wq;
>   
>   static int remove_and_add_spares(struct mddev *mddev,
>   				 struct md_rdev *this);
> @@ -651,7 +654,7 @@ static void __mddev_put(struct mddev *mddev)
>   	 * Call queue_work inside the spinlock so that flush_workqueue() after
>   	 * mddev_find will succeed in waiting for the work to be done.
>   	 */
> -	queue_work(md_misc_wq, &mddev->del_work);
> +	queue_work(md_del_wq, &mddev->del_work);
>   }
>   
>   static void mddev_put_locked(struct mddev *mddev)
> @@ -6329,9 +6332,12 @@ struct mddev *md_alloc(dev_t dev, char *name)
>   
>   	/*
>   	 * Wait for any previous instance of this device to be completely
> -	 * removed (mddev_delayed_delete).
> +	 * removed (mddev_delayed_delete). Do not flush md_misc_wq: it also
> +	 * runs sync_work, which takes reconfig_mutex and can take
> +	 * major_names_lock via md_import_device(). md_alloc() is called from
> +	 * md_probe() while blk_probe_dev() already holds major_names_lock.
>   	 */
> -	flush_workqueue(md_misc_wq);
> +	flush_workqueue(md_del_wq);
>   
>   	mutex_lock(&disks_mutex);
>   	mddev = mddev_alloc(dev);
> @@ -10765,6 +10771,10 @@ static int __init md_init(void)
>   	if (!md_misc_wq)
>   		goto err_misc_wq;
>   
> +	md_del_wq = alloc_workqueue("md_del", WQ_PERCPU, 0);
> +	if (!md_del_wq)
> +		goto err_del_wq;
> +
>   	ret = __register_blkdev(MD_MAJOR, "md", md_probe);
>   	if (ret < 0)
>   		goto err_md;
> @@ -10783,6 +10793,8 @@ static int __init md_init(void)
>   err_mdp:
>   	unregister_blkdev(MD_MAJOR, "md");
>   err_md:
> +	destroy_workqueue(md_del_wq);
> +err_del_wq:
>   	destroy_workqueue(md_misc_wq);
>   err_misc_wq:
>   	md_llbitmap_exit();
> @@ -11093,6 +11105,7 @@ static __exit void md_exit(void)
>   	spin_unlock(&all_mddevs_lock);
>   
>   	destroy_workqueue(md_misc_wq);
> +	destroy_workqueue(md_del_wq);
>   	md_bitmap_exit();
>   }
>   

-- 
Thanks,
Kuai

Re:Re: [PATCH] md: don't flush md_misc_wq from md_alloc()

[李佑鸿]

At 2026-09-17 15:41:38, "yu kuai" <yukuai@fygo.io> wrote:
>Hi,
>
>在 2026/9/15 16:33, Li Youhong 写道:
>> From: Li Youhong <liyouhong@kylinos.cn>
>>
>> md_alloc() is called from md_probe(), while blk_probe_dev() still holds
>> major_names_lock. The flush of md_misc_wq in md_alloc() is only meant to
>> wait for the previous mddev_delayed_delete() to finish.
>>
>> md_misc_wq also runs sync_work (md_start_sync), and md_start_sync takes
>> reconfig_mutex. On the other path, md_ioctl() already holds
>> reconfig_mutex when md_import_device() opens a bdev and takes
>> major_names_lock.
>>
>> Flushing md_misc_wq under major_names_lock therefore creates a lockdep
>> cycle:
>>
>>    major_names_lock -> md_misc_wq -> reconfig_mutex -> major_names_lock
>>
>> Move del_work to a dedicated workqueue and flush only that queue from
>> md_alloc(). Leave sync_work on md_misc_wq.
>>
>> Reported-by: syzbot+68e1f51046d68329940f@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=68e1f51046d68329940f
>> Fixes: e804ac780e2f ("md: fix and update workqueue usage")
>> Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
>> ---
>>   drivers/md/md.c | 28 +++++++++++++++++++++-------
>>   1 file changed, 21 insertions(+), 7 deletions(-)
>
>Patch itself look correct, however, this is not the solution I'd like.
>Since legacy_async_del_gendisk is introduced a long time now, I think it's
>time to remove the related legacy async code, so there is no need to keep
>del_work, and then the flush_workqueue() from md_alloc() is not needed anymore.

>


Agreed. I'll drop the extra workqueue and post a v2 that removes

legacy_async_del_gendisk, del_work, and the flush from md_alloc().


Thanks,
Li Youhong

>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 680b34a63cb3..87a851466892 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -86,13 +86,16 @@ static const struct kobj_type md_ktype;
>>   static DECLARE_WAIT_QUEUE_HEAD(resync_wait);
>>   
>>   /*
>> - * This workqueue is used for sync_work to register new sync_thread, and for
>> - * del_work to remove rdev, and for event_work that is only set by dm-raid.
>> + * md_misc_wq runs sync_work (new sync_thread) and event_work (dm-raid).
>> + * sync_work grabs reconfig_mutex, so this queue must not be flushed while
>> + * holding reconfig_mutex, nor while holding major_names_lock (md_probe /
>> + * md_alloc is called from blk_probe_dev with that lock held).
>>    *
>> - * Noted that sync_work will grab reconfig_mutex, hence never flush this
>> - * workqueue whith reconfig_mutex grabbed.
>> + * md_del_wq runs only del_work (mddev_delayed_delete). md_alloc() flushes
>> + * this queue to wait for a previous instance of the same device to go away.
>>    */
>>   static struct workqueue_struct *md_misc_wq;
>> +static struct workqueue_struct *md_del_wq;
>>   
>>   static int remove_and_add_spares(struct mddev *mddev,
>>   				 struct md_rdev *this);
>> @@ -651,7 +654,7 @@ static void __mddev_put(struct mddev *mddev)
>>   	 * Call queue_work inside the spinlock so that flush_workqueue() after
>>   	 * mddev_find will succeed in waiting for the work to be done.
>>   	 */
>> -	queue_work(md_misc_wq, &mddev->del_work);
>> +	queue_work(md_del_wq, &mddev->del_work);
>>   }
>>   
>>   static void mddev_put_locked(struct mddev *mddev)
>> @@ -6329,9 +6332,12 @@ struct mddev *md_alloc(dev_t dev, char *name)
>>   
>>   	/*
>>   	 * Wait for any previous instance of this device to be completely
>> -	 * removed (mddev_delayed_delete).
>> +	 * removed (mddev_delayed_delete). Do not flush md_misc_wq: it also
>> +	 * runs sync_work, which takes reconfig_mutex and can take
>> +	 * major_names_lock via md_import_device(). md_alloc() is called from
>> +	 * md_probe() while blk_probe_dev() already holds major_names_lock.
>>   	 */
>> -	flush_workqueue(md_misc_wq);
>> +	flush_workqueue(md_del_wq);
>>   
>>   	mutex_lock(&disks_mutex);
>>   	mddev = mddev_alloc(dev);
>> @@ -10765,6 +10771,10 @@ static int __init md_init(void)
>>   	if (!md_misc_wq)
>>   		goto err_misc_wq;
>>   
>> +	md_del_wq = alloc_workqueue("md_del", WQ_PERCPU, 0);
>> +	if (!md_del_wq)
>> +		goto err_del_wq;
>> +
>>   	ret = __register_blkdev(MD_MAJOR, "md", md_probe);
>>   	if (ret < 0)
>>   		goto err_md;
>> @@ -10783,6 +10793,8 @@ static int __init md_init(void)
>>   err_mdp:
>>   	unregister_blkdev(MD_MAJOR, "md");
>>   err_md:
>> +	destroy_workqueue(md_del_wq);
>> +err_del_wq:
>>   	destroy_workqueue(md_misc_wq);
>>   err_misc_wq:
>>   	md_llbitmap_exit();
>> @@ -11093,6 +11105,7 @@ static __exit void md_exit(void)
>>   	spin_unlock(&all_mddevs_lock);
>>   
>>   	destroy_workqueue(md_misc_wq);
>> +	destroy_workqueue(md_del_wq);
>>   	md_bitmap_exit();
>>   }
>>   
>
>-- 
>Thanks,
>Kuai