Re: [PATCH] md: create new workqueue for object destruction

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 4473 bytes --]

On Wed, Oct 18 2017, Artur Paszkiewicz wrote:

> On 10/18/2017 09:29 AM, NeilBrown wrote:
>> On Tue, Oct 17 2017, Shaohua Li wrote:
>> 
>>> On Tue, Oct 17, 2017 at 04:04:52PM +1100, Neil Brown wrote:
>>>>
>>>> lockdep currently complains about a potential deadlock
>>>> with sysfs access taking reconfig_mutex, and that
>>>> waiting for a work queue to complete.
>>>>
>>>> The cause is inappropriate overloading of work-items
>>>> on work-queues.
>>>>
>>>> We currently have two work-queues: md_wq and md_misc_wq.
>>>> They service 5 different tasks:
>>>>
>>>>   mddev->flush_work                       md_wq
>>>>   mddev->event_work (for dm-raid)         md_misc_wq
>>>>   mddev->del_work (mddev_delayed_delete)  md_misc_wq
>>>>   mddev->del_work (md_start_sync)         md_misc_wq
>>>>   rdev->del_work                          md_misc_wq
>>>>
>>>> We need to call flush_workqueue() for md_start_sync and ->event_work
>>>> while holding reconfig_mutex, but mustn't hold it when
>>>> flushing mddev_delayed_delete or rdev->del_work.
>>>>
>>>> md_wq is a bit special as it has WQ_MEM_RECLAIM so it is
>>>> best to leave that alone.
>>>>
>>>> So create a new workqueue, md_del_wq, and a new work_struct,
>>>> mddev->sync_work, so we can keep two classes of work separate.
>>>>
>>>> md_del_wq and ->del_work are used only for destroying rdev
>>>> and mddev.
>>>> md_misc_wq is used for event_work and sync_work.
>>>>
>>>> Also document the purpose of each flush_workqueue() call.
>>>>
>>>> This removes the lockdep warning.
>>>
>>> I had the exactly same patch queued internally,
>> 
>> Cool :-)
>> 
>>>                                                   but the mdadm test suite still
>>> shows lockdep warnning. I haven't time to check further.
>>>
>> 
>> The only other lockdep I've seen later was some ext4 thing, though I
>> haven't tried the full test suite.  I might have a look tomorrow.
>
> I'm also seeing a lockdep warning with or without this patch,
> reproducible with:
>

Thanks!
Looks like using one workqueue for mddev->del_work and rdev->del_work
causes problems.
Can you try with this addition please?

Thanks,
NeilBrown

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1dfc9879368..b3192943de7d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -92,6 +92,7 @@ EXPORT_SYMBOL(md_cluster_mod);
 static DECLARE_WAIT_QUEUE_HEAD(resync_wait);
 static struct workqueue_struct *md_wq;
 static struct workqueue_struct *md_del_wq;
+static struct workqueue_struct *rdev_del_wq;
 static struct workqueue_struct *md_misc_wq;
 
 static int remove_and_add_spares(struct mddev *mddev,
@@ -2265,7 +2266,7 @@ static void unbind_rdev_from_array(struct md_rdev *rdev)
 	synchronize_rcu();
 	INIT_WORK(&rdev->del_work, md_delayed_delete);
 	kobject_get(&rdev->kobj);
-	queue_work(md_del_wq, &rdev->del_work);
+	queue_work(rdev_del_wq, &rdev->del_work);
 }
 
 /*
@@ -4307,7 +4308,7 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len)
 		return -EOVERFLOW;
 
 	/* Ensure old devices are fully deleted (rdev->del_work) */
-	flush_workqueue(md_del_wq);
+	flush_workqueue(rdev_del_wq);
 
 	err = mddev_lock(mddev);
 	if (err)
@@ -7140,7 +7141,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
 
 	if (cmd == ADD_NEW_DISK)
 		/* need to ensure md_delayed_delete() has completed (rdev->del_work) */
-		flush_workqueue(md_del_wq);
+		flush_workqueue(rdev_del_wq);
 
 	if (cmd == HOT_REMOVE_DISK)
 		/* need to ensure recovery thread has run */
@@ -9033,8 +9034,9 @@ static int __init md_init(void)
 
 	md_wq = alloc_workqueue("md", WQ_MEM_RECLAIM, 0);
 	md_del_wq = alloc_workqueue("md_del", 0, 0);
+	rdev_del_wq = alloc_workqueue("md_rdev_del", 0, 0);
 	md_misc_wq = alloc_workqueue("md_misc", 0, 0);
-	if (!md_wq || !md_del_wq || !md_misc_wq)
+	if (!md_wq || !md_del_wq || !rdev_del_wq || !md_misc_wq)
 		goto err;
 
 	if ((ret = register_blkdev(MD_MAJOR, "md")) < 0)
@@ -9062,6 +9064,8 @@ static int __init md_init(void)
 		destroy_workqueue(md_wq);
 	if (md_del_wq)
 		destroy_workqueue(md_del_wq);
+	if (rdev_del_wq)
+		destroy_workqueue(rdev_del_wq);
 	if (md_misc_wq)
 		destroy_workqueue(md_misc_wq);
 	return ret;
@@ -9319,6 +9323,7 @@ static __exit void md_exit(void)
 	}
 	destroy_workqueue(md_misc_wq);
 	destroy_workqueue(md_del_wq);
+	destroy_workqueue(rdev_del_wq);
 	destroy_workqueue(md_wq);
 }
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]