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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=68e1f51046d68329940f
> Fixes: e804ac780e2f ("md: fix and update workqueue usage")
> Signed-off-by: Li Youhong <liyouhong@xxxxxxxxxx>
> ---
>   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