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

[李佑鸿]

At 2026-09-17 15:41:38, "yu kuai" <yukuai@xxxxxxx> wrote:
>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.

>


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