[PATCHSET] dm: update workqueue usages

[PATCHSET] dm: update workqueue usages

[Tejun Heo]

Hello,

This patchset updates workqueue usages in dm and contains the
following six patches.

 0001-dm-stripe-drop-kstriped.patch
 0002-dm-snap-kill-unused-dm_snapshot-queued_bios_work-and.patch
 0003-dm-snap-convert-to-alloc-_ordered-_workqueue.patch
 0004-dm-don-t-use-flush_scheduled_work.patch
 0005-dm-use-non-reentrant-workqueues-if-equivalent.patch
 0006-dm-snap-persistent-make-metadata_wq-multithreaded.patch

0001-0002 drop unnecessary workqueues.  0003-0004 conver to the new
APIs.  0005-0006 relax ordering requirement of some workqueues.

All patches in this series shouldn't introduce any noticeable
difference in work execution ordering.  I stayed away from more
difficult ones like...

* Can the workqueues converted by 0005 be multithreaded instead of
  non-reentrant?

* Does dm-mpath::kmpath_handlerd need WQ_RESCUER?  I suspect not but
  not sure.  If not, it can be dropped and system_wq or kmultipathd
  can be used instead.  Another curious thing here is that while
  activate_path work is queued to kmpath_handlerd, deactivate_path is
  queued to kmultipathd.  Is this intentional?  If so, it might be a
  good idea to comment why it's done that way as it's a bit confusing.

* I'm pretty sure workqueues in dm-crypt would benefit from using
  multithreaded workqueue but people already seem to be working on it
  so I left it alone.

This patchset is on top of 2.6.37-rc5 (6313e3c2).  If it needs to be
against a different tree, please let me know.  The patches are also
available in the following git tree.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git update-dm

and contains the following changes.  Thanks.

 drivers/md/dm-crypt.c           |    4 ++--
 drivers/md/dm-delay.c           |    2 +-
 drivers/md/dm-kcopyd.c          |    3 ++-
 drivers/md/dm-mpath.c           |    7 ++++---
 drivers/md/dm-raid1.c           |    5 +++--
 drivers/md/dm-snap-persistent.c |    4 ++--
 drivers/md/dm-snap.c            |   38 --------------------------------------
 drivers/md/dm-stripe.c          |   25 ++++++-------------------
 drivers/md/dm.c                 |    3 ++-
 9 files changed, 22 insertions(+), 69 deletions(-)

--
tejun

[PATCH 1/6] dm-stripe: drop kstriped

[Tejun Heo]

kstriped only serves sc->kstriped_ws which runs dm_table_event().
This doesn't need to be executed from an ordered workqueue w/ rescuer.
Drop kstriped and use the system_wq instead.  While at it, rename
kstriped_ws to trigger_event so that it's consistent with other dm
modules.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-stripe.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index f0371b4..f756ac1 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -39,23 +39,20 @@ struct stripe_c {
 	struct dm_target *ti;
 
 	/* Work struct used for triggering events*/
-	struct work_struct kstriped_ws;
+	struct work_struct trigger_event;
 
 	struct stripe stripe[0];
 };
 
-static struct workqueue_struct *kstriped;
-
 /*
  * An event is triggered whenever a drive
  * drops out of a stripe volume.
  */
 static void trigger_event(struct work_struct *work)
 {
-	struct stripe_c *sc = container_of(work, struct stripe_c, kstriped_ws);
-
+	struct stripe_c *sc = container_of(work, struct stripe_c,
+					   trigger_event);
 	dm_table_event(sc->ti->table);
-
 }
 
 static inline struct stripe_c *alloc_context(unsigned int stripes)
@@ -160,7 +157,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		return -ENOMEM;
 	}
 
-	INIT_WORK(&sc->kstriped_ws, trigger_event);
+	INIT_WORK(&sc->trigger_event, trigger_event);
 
 	/* Set pointer to dm target; used in trigger_event */
 	sc->ti = ti;
@@ -211,7 +208,7 @@ static void stripe_dtr(struct dm_target *ti)
 	for (i = 0; i < sc->stripes; i++)
 		dm_put_device(ti, sc->stripe[i].dev);
 
-	flush_workqueue(kstriped);
+	flush_scheduled_work();
 	kfree(sc);
 }
 
@@ -367,7 +364,7 @@ static int stripe_end_io(struct dm_target *ti, struct bio *bio,
 			atomic_inc(&(sc->stripe[i].error_count));
 			if (atomic_read(&(sc->stripe[i].error_count)) <
 			    DM_IO_ERROR_THRESHOLD)
-				queue_work(kstriped, &sc->kstriped_ws);
+				schedule_work(&sc->trigger_event);
 		}
 
 	return error;
@@ -422,20 +419,10 @@ int __init dm_stripe_init(void)
 		return r;
 	}
 
-	kstriped = create_singlethread_workqueue("kstriped");
-	if (!kstriped) {
-		DMERR("failed to create workqueue kstriped");
-		dm_unregister_target(&stripe_target);
-		return -ENOMEM;
-	}
-
 	return r;
 }
 
 void dm_stripe_exit(void)
 {
 	dm_unregister_target(&stripe_target);
-	destroy_workqueue(kstriped);
-
-	return;
 }
-- 
1.7.1

[PATCH 2/6] dm-snap: kill unused dm_snapshot->queued_bios_work and ksnapd

[Tejun Heo]

dm_snapshot->queued_bios_work isn't used anywhere.  Remove
->queued_bios[_work], the work function and ksnapd.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-snap.c |   38 --------------------------------------
 1 files changed, 0 insertions(+), 38 deletions(-)

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index 53cf79d..0f47698 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -19,7 +19,6 @@
 #include <linux/vmalloc.h>
 #include <linux/log2.h>
 #include <linux/dm-kcopyd.h>
-#include <linux/workqueue.h>
 
 #include "dm-exception-store.h"
 
@@ -106,10 +105,6 @@ struct dm_snapshot {
 
 	struct dm_kcopyd_client *kcopyd_client;
 
-	/* Queue of snapshot writes for ksnapd to flush */
-	struct bio_list queued_bios;
-	struct work_struct queued_bios_work;
-
 	/* Wait for events based on state_bits */
 	unsigned long state_bits;
 
@@ -160,9 +155,6 @@ struct dm_dev *dm_snap_cow(struct dm_snapshot *s)
 }
 EXPORT_SYMBOL(dm_snap_cow);
 
-static struct workqueue_struct *ksnapd;
-static void flush_queued_bios(struct work_struct *work);
-
 static sector_t chunk_to_sector(struct dm_exception_store *store,
 				chunk_t chunk)
 {
@@ -1153,9 +1145,6 @@ static int snapshot_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	spin_lock_init(&s->tracked_chunk_lock);
 
-	bio_list_init(&s->queued_bios);
-	INIT_WORK(&s->queued_bios_work, flush_queued_bios);
-
 	ti->private = s;
 	ti->num_flush_requests = num_flush_requests;
 
@@ -1279,8 +1268,6 @@ static void snapshot_dtr(struct dm_target *ti)
 	struct dm_snapshot *s = ti->private;
 	struct dm_snapshot *snap_src = NULL, *snap_dest = NULL;
 
-	flush_workqueue(ksnapd);
-
 	down_read(&_origins_lock);
 	/* Check whether exception handover must be cancelled */
 	(void) __find_snapshots_sharing_cow(s, &snap_src, &snap_dest, NULL);
@@ -1342,20 +1329,6 @@ static void flush_bios(struct bio *bio)
 	}
 }
 
-static void flush_queued_bios(struct work_struct *work)
-{
-	struct dm_snapshot *s =
-		container_of(work, struct dm_snapshot, queued_bios_work);
-	struct bio *queued_bios;
-	unsigned long flags;
-
-	spin_lock_irqsave(&s->pe_lock, flags);
-	queued_bios = bio_list_get(&s->queued_bios);
-	spin_unlock_irqrestore(&s->pe_lock, flags);
-
-	flush_bios(queued_bios);
-}
-
 static int do_origin(struct dm_dev *origin, struct bio *bio);
 
 /*
@@ -2291,17 +2264,8 @@ static int __init dm_snapshot_init(void)
 		goto bad_tracked_chunk_cache;
 	}
 
-	ksnapd = create_singlethread_workqueue("ksnapd");
-	if (!ksnapd) {
-		DMERR("Failed to create ksnapd workqueue.");
-		r = -ENOMEM;
-		goto bad_pending_pool;
-	}
-
 	return 0;
 
-bad_pending_pool:
-	kmem_cache_destroy(tracked_chunk_cache);
 bad_tracked_chunk_cache:
 	kmem_cache_destroy(pending_cache);
 bad_pending_cache:
@@ -2322,8 +2286,6 @@ bad_register_snapshot_target:
 
 static void __exit dm_snapshot_exit(void)
 {
-	destroy_workqueue(ksnapd);
-
 	dm_unregister_target(&snapshot_target);
 	dm_unregister_target(&origin_target);
 	dm_unregister_target(&merge_target);
-- 
1.7.1

[PATCH 3/6] dm-snap: convert to alloc[_ordered]_workqueue()

[Tejun Heo]

Convert all create[_singlethread]_work() users to the new
alloc[_ordered]_workqueue().  This conversion is mechanical and
doesn't introduce any behavior change.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-crypt.c           |    4 ++--
 drivers/md/dm-delay.c           |    2 +-
 drivers/md/dm-kcopyd.c          |    2 +-
 drivers/md/dm-mpath.c           |    5 +++--
 drivers/md/dm-raid1.c           |    2 +-
 drivers/md/dm-snap-persistent.c |    2 +-
 drivers/md/dm.c                 |    2 +-
 7 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index d5b0e4c..0ff5ae2 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1252,13 +1252,13 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	cc->start = tmpll;
 
 	ret = -ENOMEM;
-	cc->io_queue = create_singlethread_workqueue("kcryptd_io");
+	cc->io_queue = alloc_ordered_workqueue("kcryptd_io", WQ_MEM_RECLAIM);
 	if (!cc->io_queue) {
 		ti->error = "Couldn't create kcryptd io queue";
 		goto bad;
 	}
 
-	cc->crypt_queue = create_singlethread_workqueue("kcryptd");
+	cc->crypt_queue = alloc_ordered_workqueue("kcryptd", WQ_MEM_RECLAIM);
 	if (!cc->crypt_queue) {
 		ti->error = "Couldn't create kcryptd queue";
 		goto bad;
diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index baa1191..f18375d 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -352,7 +352,7 @@ static int __init dm_delay_init(void)
 {
 	int r = -ENOMEM;
 
-	kdelayd_wq = create_workqueue("kdelayd");
+	kdelayd_wq = alloc_workqueue("kdelayd", WQ_MEM_RECLAIM, 0);
 	if (!kdelayd_wq) {
 		DMERR("Couldn't start kdelayd");
 		goto bad_queue;
diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
index d8587ba..6d641e8 100644
--- a/drivers/md/dm-kcopyd.c
+++ b/drivers/md/dm-kcopyd.c
@@ -624,7 +624,7 @@ int dm_kcopyd_client_create(unsigned int nr_pages,
 		goto bad_slab;
 
 	INIT_WORK(&kc->kcopyd_work, do_work);
-	kc->kcopyd_wq = create_singlethread_workqueue("kcopyd");
+	kc->kcopyd_wq = alloc_ordered_workqueue("kcopyd", WQ_MEM_RECLAIM);
 	if (!kc->kcopyd_wq)
 		goto bad_workqueue;
 
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 487ecda..5765eb7 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1687,7 +1687,7 @@ static int __init dm_multipath_init(void)
 		return -EINVAL;
 	}
 
-	kmultipathd = create_workqueue("kmpathd");
+	kmultipathd = alloc_workqueue("kmpathd", WQ_MEM_RECLAIM, 0);
 	if (!kmultipathd) {
 		DMERR("failed to create workqueue kmpathd");
 		dm_unregister_target(&multipath_target);
@@ -1701,7 +1701,8 @@ static int __init dm_multipath_init(void)
 	 * old workqueue would also create a bottleneck in the
 	 * path of the storage hardware device activation.
 	 */
-	kmpath_handlerd = create_singlethread_workqueue("kmpath_handlerd");
+	kmpath_handlerd = alloc_ordered_workqueue("kmpath_handlerd",
+						  WQ_MEM_RECLAIM);
 	if (!kmpath_handlerd) {
 		DMERR("failed to create workqueue kmpath_handlerd");
 		destroy_workqueue(kmultipathd);
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index 19a59b0..dbd6af8 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1077,7 +1077,7 @@ static int mirror_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->split_io = dm_rh_get_region_size(ms->rh);
 	ti->num_flush_requests = 1;
 
-	ms->kmirrord_wq = create_singlethread_workqueue("kmirrord");
+	ms->kmirrord_wq = alloc_ordered_workqueue("kmirrord", WQ_MEM_RECLAIM);
 	if (!ms->kmirrord_wq) {
 		DMERR("couldn't start kmirrord");
 		r = -ENOMEM;
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index 2129cdb..d3021a6 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -818,7 +818,7 @@ static int persistent_ctr(struct dm_exception_store *store,
 	atomic_set(&ps->pending_count, 0);
 	ps->callbacks = NULL;
 
-	ps->metadata_wq = create_singlethread_workqueue("ksnaphd");
+	ps->metadata_wq = alloc_ordered_workqueue("ksnaphd", WQ_MEM_RECLAIM);
 	if (!ps->metadata_wq) {
 		kfree(ps);
 		DMERR("couldn't start header metadata update thread");
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7cb1352..064b2dc 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1884,7 +1884,7 @@ static struct mapped_device *alloc_dev(int minor)
 	add_disk(md->disk);
 	format_dev_t(md->name, MKDEV(_major, minor));
 
-	md->wq = create_singlethread_workqueue("kdmflush");
+	md->wq = alloc_ordered_workqueue("kdmflush", WQ_MEM_RECLAIM);
 	if (!md->wq)
 		goto bad_thread;
 
-- 
1.7.1

[PATCH 4/6] dm: don't use flush_scheduled_work()

[Tejun Heo]

flush_scheduled_work() is being deprecated.  Flush the used work
directly instead.  In all dm modules, the only work which uses
system_wq is ->trigger_event.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-mpath.c  |    2 +-
 drivers/md/dm-raid1.c  |    2 +-
 drivers/md/dm-stripe.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 5765eb7..33255bd 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -931,7 +931,7 @@ static void flush_multipath_work(struct multipath *m)
 	flush_workqueue(kmpath_handlerd);
 	multipath_wait_for_pg_init_completion(m);
 	flush_workqueue(kmultipathd);
-	flush_scheduled_work();
+	flush_work_sync(&m->trigger_event);
 }
 
 static void multipath_dtr(struct dm_target *ti)
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index dbd6af8..5139091 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1130,7 +1130,7 @@ static void mirror_dtr(struct dm_target *ti)
 
 	del_timer_sync(&ms->timer);
 	flush_workqueue(ms->kmirrord_wq);
-	flush_scheduled_work();
+	flush_work_sync(&ms->trigger_event);
 	dm_kcopyd_client_destroy(ms->kcopyd_client);
 	destroy_workqueue(ms->kmirrord_wq);
 	free_context(ms, ti, ms->nr_mirrors);
diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index f756ac1..cf0693f 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -208,7 +208,7 @@ static void stripe_dtr(struct dm_target *ti)
 	for (i = 0; i < sc->stripes; i++)
 		dm_put_device(ti, sc->stripe[i].dev);
 
-	flush_scheduled_work();
+	flush_work_sync(&sc->trigger_event);
 	kfree(sc);
 }
 
-- 
1.7.1

[PATCH 5/6] dm: use non-reentrant workqueues if equivalent

[Tejun Heo]

kmirrord_wq, kcopyd_work and md->wq are created per dm instance and
serves only single work item from the dm instance, so non-reentrant
workqueues would provide the same ordering guarantees as ordered ones
while allowing CPU affinity and use of the workqueues for other
purposes.  Switch them to non-reentrant workqueues.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-kcopyd.c |    3 ++-
 drivers/md/dm-raid1.c  |    3 ++-
 drivers/md/dm.c        |    3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
index 6d641e8..20c1a11 100644
--- a/drivers/md/dm-kcopyd.c
+++ b/drivers/md/dm-kcopyd.c
@@ -624,7 +624,8 @@ int dm_kcopyd_client_create(unsigned int nr_pages,
 		goto bad_slab;
 
 	INIT_WORK(&kc->kcopyd_work, do_work);
-	kc->kcopyd_wq = alloc_ordered_workqueue("kcopyd", WQ_MEM_RECLAIM);
+	kc->kcopyd_wq = alloc_workqueue("kcopyd",
+					WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
 	if (!kc->kcopyd_wq)
 		goto bad_workqueue;
 
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index 5139091..32bc404 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1077,7 +1077,8 @@ static int mirror_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->split_io = dm_rh_get_region_size(ms->rh);
 	ti->num_flush_requests = 1;
 
-	ms->kmirrord_wq = alloc_ordered_workqueue("kmirrord", WQ_MEM_RECLAIM);
+	ms->kmirrord_wq = alloc_workqueue("kmirrord",
+					  WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
 	if (!ms->kmirrord_wq) {
 		DMERR("couldn't start kmirrord");
 		r = -ENOMEM;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 064b2dc..854409d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1884,7 +1884,8 @@ static struct mapped_device *alloc_dev(int minor)
 	add_disk(md->disk);
 	format_dev_t(md->name, MKDEV(_major, minor));
 
-	md->wq = alloc_ordered_workqueue("kdmflush", WQ_MEM_RECLAIM);
+	md->wq = alloc_workqueue("kdmflush",
+				 WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);
 	if (!md->wq)
 		goto bad_thread;
 
-- 
1.7.1

[PATCH 6/6] dm-snap-persistent: make metadata_wq multithreaded

[Tejun Heo]

metadata_wq serves on-stack work items from chunk_io().  Even if
multiple chunk_io() are simultaneously in progress, each is
independent and queued only once, so multithreaded workqueue can be
safely used.

Switch metadata_wq to multithread and flush the work item instead of
the workqueue in chunk_io().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
---
 drivers/md/dm-snap-persistent.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index d3021a6..95891df 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -256,7 +256,7 @@ static int chunk_io(struct pstore *ps, void *area, chunk_t chunk, int rw,
 	 */
 	INIT_WORK_ONSTACK(&req.work, do_metadata);
 	queue_work(ps->metadata_wq, &req.work);
-	flush_workqueue(ps->metadata_wq);
+	flush_work(&req.work);
 
 	return req.result;
 }
@@ -818,7 +818,7 @@ static int persistent_ctr(struct dm_exception_store *store,
 	atomic_set(&ps->pending_count, 0);
 	ps->callbacks = NULL;
 
-	ps->metadata_wq = alloc_ordered_workqueue("ksnaphd", WQ_MEM_RECLAIM);
+	ps->metadata_wq = alloc_workqueue("ksnaphd", WQ_MEM_RECLAIM, 0);
 	if (!ps->metadata_wq) {
 		kfree(ps);
 		DMERR("couldn't start header metadata update thread");
-- 
1.7.1

Re: [PATCHSET] dm: update workqueue usages

[Milan Broz]

On 12/12/2010 08:10 PM, Tejun Heo wrote:

> * I'm pretty sure workqueues in dm-crypt would benefit from using
>   multithreaded workqueue but people already seem to be working on it
>   so I left it alone.

Yes, FYI it is already here with new queue api:
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-scale-to-multiple-CPUs.patch

(So that one chunk for dm-crypt in patch 3/6 can be dropped.)

Thanks for fixing other parts,
Milan

Re: [PATCHSET] dm: update workqueue usages

[Heinz Mauelshagen]

On Sun, 2010-12-12 at 20:10 +0100, Tejun Heo wrote:
> Hello,

Hi Tejun,

what's the rational behind dropping create_singlethread_workqueue() in
favor of alloc_ordered_workqueue(...WQ_MEM_RECLAIM..) which are
semantically identical?

Heinz

> 
> This patchset updates workqueue usages in dm and contains the
> following six patches.
> 
>  0001-dm-stripe-drop-kstriped.patch
>  0002-dm-snap-kill-unused-dm_snapshot-queued_bios_work-and.patch
>  0003-dm-snap-convert-to-alloc-_ordered-_workqueue.patch
>  0004-dm-don-t-use-flush_scheduled_work.patch
>  0005-dm-use-non-reentrant-workqueues-if-equivalent.patch
>  0006-dm-snap-persistent-make-metadata_wq-multithreaded.patch
> 
> 0001-0002 drop unnecessary workqueues.  0003-0004 conver to the new
> APIs.  0005-0006 relax ordering requirement of some workqueues.
> 
> All patches in this series shouldn't introduce any noticeable
> difference in work execution ordering.  I stayed away from more
> difficult ones like...
> 
> * Can the workqueues converted by 0005 be multithreaded instead of
>   non-reentrant?
> 
> * Does dm-mpath::kmpath_handlerd need WQ_RESCUER?  I suspect not but
>   not sure.  If not, it can be dropped and system_wq or kmultipathd
>   can be used instead.  Another curious thing here is that while
>   activate_path work is queued to kmpath_handlerd, deactivate_path is
>   queued to kmultipathd.  Is this intentional?  If so, it might be a
>   good idea to comment why it's done that way as it's a bit confusing.
> 
> * I'm pretty sure workqueues in dm-crypt would benefit from using
>   multithreaded workqueue but people already seem to be working on it
>   so I left it alone.
> 
> This patchset is on top of 2.6.37-rc5 (6313e3c2).  If it needs to be
> against a different tree, please let me know.  The patches are also
> available in the following git tree.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git update-dm
> 
> and contains the following changes.  Thanks.
> 
>  drivers/md/dm-crypt.c           |    4 ++--
>  drivers/md/dm-delay.c           |    2 +-
>  drivers/md/dm-kcopyd.c          |    3 ++-
>  drivers/md/dm-mpath.c           |    7 ++++---
>  drivers/md/dm-raid1.c           |    5 +++--
>  drivers/md/dm-snap-persistent.c |    4 ++--
>  drivers/md/dm-snap.c            |   38 --------------------------------------
>  drivers/md/dm-stripe.c          |   25 ++++++-------------------
>  drivers/md/dm.c                 |    3 ++-
>  9 files changed, 22 insertions(+), 69 deletions(-)
> 
> --
> tejun
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCHSET] dm: update workqueue usages

[Tejun Heo]

Hello,

On 12/14/2010 01:19 PM, Heinz Mauelshagen wrote:
> what's the rational behind dropping create_singlethread_workqueue() in
> favor of alloc_ordered_workqueue(...WQ_MEM_RECLAIM..) which are
> semantically identical?

create[_singlethread]_workqueue() implies WQ_MEM_RECLAIM which is
usually only necessary in the block IO path, so I'm going through all
usages and either making them use system*_wq or make use of only the
necessary features (often droppping WQ_MEM_RECLAIM), so the use of new
API basically indicates that it went through workqueue usage review
and determined to actually require a separate workqueue with the
described features.

create_*workqueue() interfaces will be deprecated once all in-kernel
users have been converted to encourage new and external code to study
and apply the new workqueue features as necessary.

For dm, this looks rather spruious as basically all workqueues in dm
need WQ_MEM_RECLAIM but as shown with the later patches, there still
are adjustments which can be made with the new API to make the
workqueues behave more efficiently.

Thanks.

-- 
tejun

Re: [PATCHSET] dm: update workqueue usages

[Tejun Heo]

On 12/12/2010 10:23 PM, Milan Broz wrote:
> On 12/12/2010 08:10 PM, Tejun Heo wrote:
> 
>> * I'm pretty sure workqueues in dm-crypt would benefit from using
>>   multithreaded workqueue but people already seem to be working on it
>>   so I left it alone.
> 
> Yes, FYI it is already here with new queue api:
> http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-scale-to-multiple-CPUs.patch

Yeah, that looks much better.

> (So that one chunk for dm-crypt in patch 3/6 can be dropped.)

I'll regenerate the series w/o the chunk.  BTW, against which tree
should it be generated?

Thanks.

-- 
tejun

Re: dm: update workqueue usages

[Mike Snitzer]

On Tue, Dec 14 2010 at 10:49am -0500,
Tejun Heo <tj@kernel.org> wrote:

> On 12/12/2010 10:23 PM, Milan Broz wrote:
> > On 12/12/2010 08:10 PM, Tejun Heo wrote:
> > 
> >> * I'm pretty sure workqueues in dm-crypt would benefit from using
> >>   multithreaded workqueue but people already seem to be working on it
> >>   so I left it alone.
> > 
> > Yes, FYI it is already here with new queue api:
> > http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-crypt-scale-to-multiple-CPUs.patch
> 
> Yeah, that looks much better.
> 
> > (So that one chunk for dm-crypt in patch 3/6 can be dropped.)
> 
> I'll regenerate the series w/o the chunk.  BTW, against which tree
> should it be generated?

I'll be reviewing your workqueue patches today.  I can take care of
rebasing the patches as needed ontop of Alasdair's quilt tree (up to
dm-raid1-support-discard.patch):
http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/series

Once I've folded your patches in I will share the url for resulting
quilt tree.

Thanks,
Mike

Re: dm: update workqueue usages

[Tejun Heo]

Hello,

On 12/14/2010 05:03 PM, Mike Snitzer wrote:
> I'll be reviewing your workqueue patches today.  I can take care of
> rebasing the patches as needed ontop of Alasdair's quilt tree (up to
> dm-raid1-support-discard.patch):
> http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/series
> 
> Once I've folded your patches in I will share the url for resulting
> quilt tree.

Thanks.  I'll hold off for now then.

-- 
tejun

Re: [PATCH 1/6] dm-stripe: drop kstriped

[Mike Snitzer]

On Sun, Dec 12 2010 at  2:10pm -0500,
Tejun Heo <tj@kernel.org> wrote:

> kstriped only serves sc->kstriped_ws which runs dm_table_event().
> This doesn't need to be executed from an ordered workqueue w/ rescuer.
> Drop kstriped and use the system_wq instead.

What might look unnecessary at first glance is a bit more nuanced than
we'd both like...

I'm concerned about switching to using the global system_wq and calling
flush_scheduled_work() when resuming a DM table (calls a DM target's
->dtr() via dm_table_destroy in drivers/md/dm-ioctl.c:do_resume).

This introduces potential for a DM table swap performing a storm of
additional unrelated works.

Definitely needs further review.

Mike

Re: [PATCH 1/6] dm-stripe: drop kstriped

[Mike Snitzer]

On Tue, Dec 14 2010 at  6:59pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Sun, Dec 12 2010 at  2:10pm -0500,
> Tejun Heo <tj@kernel.org> wrote:
> 
> > kstriped only serves sc->kstriped_ws which runs dm_table_event().
> > This doesn't need to be executed from an ordered workqueue w/ rescuer.
> > Drop kstriped and use the system_wq instead.
>
> I'm concerned about switching to using the global system_wq and calling
> flush_scheduled_work() when resuming a DM table (calls a DM target's
> ->dtr() via dm_table_destroy in drivers/md/dm-ioctl.c:do_resume).

Ah, I just got to 4/6 and see you switched flush_scheduled_work() to:
flush_work_sync(&sc->trigger_event);

Thanks,
Mike

Re: dm: update workqueue usages

[Mike Snitzer]

On Tue, Dec 14 2010 at 11:05am -0500,
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On 12/14/2010 05:03 PM, Mike Snitzer wrote:
> > I'll be reviewing your workqueue patches today.  I can take care of
> > rebasing the patches as needed ontop of Alasdair's quilt tree (up to
> > dm-raid1-support-discard.patch):
> > http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/series
> > 
> > Once I've folded your patches in I will share the url for resulting
> > quilt tree.
> 
> Thanks.  I'll hold off for now then.

Hi Tejun,

I've refreshed your patches, tweaked the patch headers and subjects a
bit and added my Signed-off-by.  I've staged them in a quilt tree for
Alasdair to pick up here:
http://people.redhat.com/msnitzer/patches/upstream/for-agk/series.html

(your patches are at the end, 0005-0010).

Thanks,
Mike

Re: dm: update workqueue usages

[Tejun Heo]

Hello,

On 12/20/2010 03:14 PM, Mike Snitzer wrote:
> I've refreshed your patches, tweaked the patch headers and subjects a
> bit and added my Signed-off-by.  I've staged them in a quilt tree for
> Alasdair to pick up here:
> http://people.redhat.com/msnitzer/patches/upstream/for-agk/series.html
> 
> (your patches are at the end, 0005-0010).

Great, looks good to me.

Thank you.

-- 
tejun

Re: [PATCH 1/6] dm-stripe: drop kstriped

[Alasdair G Kergon]

On Tue, Dec 14, 2010 at 08:34:58PM -0500, Mike Snitzer wrote:
> On Tue, Dec 14 2010 at  6:59pm -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> > I'm concerned about switching to using the global system_wq and calling
> > flush_scheduled_work() when resuming a DM table (calls a DM target's
> > ->dtr() via dm_table_destroy in drivers/md/dm-ioctl.c:do_resume).
 
Absolutely.

I'm not taking patch 1 in its current form - it introduces a problem that was
not present before.  Probably part of patch 4 needs moving into patch 1.

Alasdair

Re: [PATCH 6/6] dm-snap-persistent: make metadata_wq multithreaded

[Mikulas Patocka]

On Sun, 12 Dec 2010, Tejun Heo wrote:

> metadata_wq serves on-stack work items from chunk_io().  Even if
> multiple chunk_io() are simultaneously in progress, each is
> independent and queued only once, so multithreaded workqueue can be
> safely used.
> 
> Switch metadata_wq to multithread and flush the work item instead of
> the workqueue in chunk_io().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: dm-devel@redhat.com

Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>
Acked-by: Mikulas Patocka <mpatocka@redhat.com>

> ---
>  drivers/md/dm-snap-persistent.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
> index d3021a6..95891df 100644
> --- a/drivers/md/dm-snap-persistent.c
> +++ b/drivers/md/dm-snap-persistent.c
> @@ -256,7 +256,7 @@ static int chunk_io(struct pstore *ps, void *area, chunk_t chunk, int rw,
>  	 */
>  	INIT_WORK_ONSTACK(&req.work, do_metadata);
>  	queue_work(ps->metadata_wq, &req.work);
> -	flush_workqueue(ps->metadata_wq);
> +	flush_work(&req.work);
>  
>  	return req.result;
>  }
> @@ -818,7 +818,7 @@ static int persistent_ctr(struct dm_exception_store *store,
>  	atomic_set(&ps->pending_count, 0);
>  	ps->callbacks = NULL;
>  
> -	ps->metadata_wq = alloc_ordered_workqueue("ksnaphd", WQ_MEM_RECLAIM);
> +	ps->metadata_wq = alloc_workqueue("ksnaphd", WQ_MEM_RECLAIM, 0);
>  	if (!ps->metadata_wq) {
>  		kfree(ps);
>  		DMERR("couldn't start header metadata update thread");
> -- 
> 1.7.1
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 

Re: [PATCH 1/6] dm-stripe: drop kstriped

[Tejun Heo]

Hello,

On Fri, Jan 07, 2011 at 11:02:31PM +0000, Alasdair G Kergon wrote:
> On Tue, Dec 14, 2010 at 08:34:58PM -0500, Mike Snitzer wrote:
> > On Tue, Dec 14 2010 at  6:59pm -0500,
> > Mike Snitzer <snitzer@redhat.com> wrote:
> > > I'm concerned about switching to using the global system_wq and calling
> > > flush_scheduled_work() when resuming a DM table (calls a DM target's
> > > ->dtr() via dm_table_destroy in drivers/md/dm-ioctl.c:do_resume).
>  
> Absolutely.
> 
> I'm not taking patch 1 in its current form - it introduces a problem
> that was not present before.  Probably part of patch 4 needs moving
> into patch 1.

I can just combine 1 and 4 then or reorder them such that
flush_work_sync() conversion happens first and then the dedicated wq
is dropped.  Which one would you prefer?

Thanks.

-- 
tejun