system_nrt_wq, system suspend, and the freezer

system_nrt_wq, system suspend, and the freezer

[Alan Stern]

Folks:

I recently uncovered a bug in the block layer.  It uses a workqueue to
periodically probe removable drives for media or other state changes,
and the workqueue it uses is system_nrt_wq.

The bug is that system_nrt_wq is not freezable, so it keeps on running
even while the system is in the process of suspending or hibernating.  
Doing I/O to a suspended drive doesn't work well and in some cases
causes nasty problems.  Obviously these polls need to stop during a 
suspend transition.

A search through the kernel shows that system_nrt_wq is also used in a
few other subsystems:

./fs/cifs/cifssmb.c:	queue_work(system_nrt_wq, &rdata->work);
./fs/cifs/cifssmb.c:	queue_work(system_nrt_wq, &wdata->work);
./fs/cifs/misc.c:				queue_work(system_nrt_wq,
./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &tcp_ses->echo, SMB_ECHO_INTERVAL);
./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &cifs_sb->prune_tlinks,
./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &cifs_sb->prune_tlinks,

./drivers/mmc/core/host.c:		queue_work(system_nrt_wq, &host->clk_gate_work);

./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0);

./security/keys/gc.c:		queue_work(system_nrt_wq, &key_gc_work);
./security/keys/gc.c:	queue_work(system_nrt_wq, &key_gc_work);
./security/keys/gc.c:	queue_work(system_nrt_wq, &key_gc_work);
./security/keys/gc.c:		queue_work(system_nrt_wq, &key_gc_work);
./security/keys/key.c:			queue_work(system_nrt_wq, &key_gc_work);

My question to all of you: Should system_nrt_wq be made freezable, or 
should I create a new workqueue that is both freezable and 
non-reentrant?  And if I do, which of the usages above should be 
converted to the new workqueue?

Thanks,

Alan Stern

Re: system_nrt_wq, system suspend, and the freezer

[David Howells]

Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> My question to all of you: Should system_nrt_wq be made freezable, or 
> should I create a new workqueue that is both freezable and 
> non-reentrant?  And if I do, which of the usages above should be 
> converted to the new workqueue?

As far as keys are concerned, it's only for garbage collection purposes, so
having it freezable shouldn't be a problem.

David

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

On Thu, Feb 16, 2012 at 03:22:24PM +0000, David Howells wrote:
> Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> 
> > My question to all of you: Should system_nrt_wq be made freezable, or 
> > should I create a new workqueue that is both freezable and 
> > non-reentrant?  And if I do, which of the usages above should be 
> > converted to the new workqueue?
> 
> As far as keys are concerned, it's only for garbage collection purposes, so
> having it freezable shouldn't be a problem.

If freezing is not strictly necessary, please avoid marking it as
freezable.

Thanks.

-- 
tejun

Re: system_nrt_wq, system suspend, and the freezer

[David Howells]

Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > > My question to all of you: Should system_nrt_wq be made freezable, or 
> > > should I create a new workqueue that is both freezable and 
> > > non-reentrant?  And if I do, which of the usages above should be 
> > > converted to the new workqueue?
> > 
> > As far as keys are concerned, it's only for garbage collection purposes, so
> > having it freezable shouldn't be a problem.
> 
> If freezing is not strictly necessary, please avoid marking it as
> freezable.

?

The key_garbage_collector work item is marked neither freezable nor
unfreezable that I can see.

David

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

On Thu, Feb 16, 2012 at 04:35:51PM +0000, David Howells wrote:
> The key_garbage_collector work item is marked neither freezable nor
> unfreezable that I can see.

Heh, was too brief apparently. :)

I was trying to say that if it doesn't require freezing, please don't
put it on a freezable workqueue.  So, IOW, I was just saying that
nothing should change for key_garbage_collector.

Thanks.

-- 
tejun

Re: system_nrt_wq, system suspend, and the freezer

[Jeff Layton]

On Thu, 16 Feb 2012 09:41:34 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> Folks:
> 
> I recently uncovered a bug in the block layer.  It uses a workqueue to
> periodically probe removable drives for media or other state changes,
> and the workqueue it uses is system_nrt_wq.
> 
> The bug is that system_nrt_wq is not freezable, so it keeps on running
> even while the system is in the process of suspending or hibernating.  
> Doing I/O to a suspended drive doesn't work well and in some cases
> causes nasty problems.  Obviously these polls need to stop during a 
> suspend transition.
> 
> A search through the kernel shows that system_nrt_wq is also used in a
> few other subsystems:
> 
> ./fs/cifs/cifssmb.c:	queue_work(system_nrt_wq, &rdata->work);
> ./fs/cifs/cifssmb.c:	queue_work(system_nrt_wq, &wdata->work);
> ./fs/cifs/misc.c:				queue_work(system_nrt_wq,
> ./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &server->echo, SMB_ECHO_INTERVAL);
> ./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &tcp_ses->echo, SMB_ECHO_INTERVAL);
> ./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &cifs_sb->prune_tlinks,
> ./fs/cifs/connect.c:	queue_delayed_work(system_nrt_wq, &cifs_sb->prune_tlinks,
> 

These should  all be freezable and we might even be able to get away
with WQ_UNBOUND for some of these.

I think we put most of these in system_nrt_wq because Tejun put an
earlier job into that queue when he converted it from slow_work and we
just cargo-cult copied that...

I'll spend some time looking at this in the next day or two, but I
suspect that the right answer is to just move these off of the "public"
workqueues altogether.

> ./drivers/mmc/core/host.c:		queue_work(system_nrt_wq, &host->clk_gate_work);
> 
> ./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD);
> ./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD);
> ./drivers/gpu/drm/drm_crtc_helper.c:		queue_delayed_work(system_nrt_wq, &dev->mode_config.output_poll_work, 0);
> 
> ./security/keys/gc.c:		queue_work(system_nrt_wq, &key_gc_work);
> ./security/keys/gc.c:	queue_work(system_nrt_wq, &key_gc_work);
> ./security/keys/gc.c:	queue_work(system_nrt_wq, &key_gc_work);
> ./security/keys/gc.c:		queue_work(system_nrt_wq, &key_gc_work);
> ./security/keys/key.c:			queue_work(system_nrt_wq, &key_gc_work);
> 
> My question to all of you: Should system_nrt_wq be made freezable, or 
> should I create a new workqueue that is both freezable and 
> non-reentrant?  And if I do, which of the usages above should be 
> converted to the new workqueue?
> 
> Thanks,
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@poochiereds.net>

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

Hello,

On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
> These should  all be freezable and we might even be able to get away
> with WQ_UNBOUND for some of these.

In general, I would recommend specifying as few special attribute as
possible.  If WQ_UNBOUND is necessary (large amount of CPU cycles
consumed, extremely high concurrency), sure, but I think we're
generally better off using as default attributes as possible.  It just
makes things much easier later when we need to implement new features
or update the implementation.

> I think we put most of these in system_nrt_wq because Tejun put an
> earlier job into that queue when he converted it from slow_work and we
> just cargo-cult copied that...
> 
> I'll spend some time looking at this in the next day or two, but I
> suspect that the right answer is to just move these off of the "public"
> workqueues altogether.

If freezing & nrt is everything necessary, just create
system_nrt_freezable_wq and use that.

Thanks.

-- 
tejun

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello,
> 
> On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
> > These should  all be freezable and we might even be able to get away
> > with WQ_UNBOUND for some of these.
> 
> In general, I would recommend specifying as few special attribute as
> possible.  If WQ_UNBOUND is necessary (large amount of CPU cycles
> consumed, extremely high concurrency), sure, but I think we're
> generally better off using as default attributes as possible.  It just
> makes things much easier later when we need to implement new features
> or update the implementation.
> 
> > I think we put most of these in system_nrt_wq because Tejun put an
> > earlier job into that queue when he converted it from slow_work and we
> > just cargo-cult copied that...
> > 
> > I'll spend some time looking at this in the next day or two, but I
> > suspect that the right answer is to just move these off of the "public"
> > workqueues altogether.
> 
> If freezing & nrt is everything necessary, just create
> system_nrt_freezable_wq and use that.

Here's my proposed patch.  If nobody objects, I'll submit it to Rafael
with an appropriate patch description.  Then anybody who wants can
convert over to the new workqueue.

Alan Stern



 block/genhd.c             |   10 +++++-----
 include/linux/workqueue.h |    4 ++++
 kernel/workqueue.c        |    7 ++++++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===================================================================
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
 	intv = disk_events_poll_jiffies(disk);
 	set_timer_slack(&ev->dwork.timer, intv / 4);
 	if (check_now)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	else if (intv)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
 out_unlock:
 	spin_unlock_irqrestore(&ev->lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
 	ev->clearing |= mask;
 	if (!ev->block) {
 		cancel_delayed_work(&ev->dwork);
-		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	}
 	spin_unlock_irq(&ev->lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
 
 	/* uncondtionally schedule event check and wait for it to finish */
 	disk_block_events(disk);
-	queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+	queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	flush_delayed_work(&ev->dwork);
 	__disk_unblock_events(disk, false);
 
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
 
 	intv = disk_events_poll_jiffies(disk);
 	if (!ev->block && intv)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
 
 	spin_unlock_irq(&ev->lock);
 
Index: usb-3.3/include/linux/workqueue.h
===================================================================
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
 
 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===================================================================
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/workqueue.h>
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
 					    WQ_UNBOUND_MAX_ACTIVE);
 	system_freezable_wq = alloc_workqueue("events_freezable",
 					      WQ_FREEZABLE, 0);
+	system_nrt_freezable_wq = alloc_workqueue("events_nrt_freezable",
+			WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
 	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-	       !system_unbound_wq || !system_freezable_wq);
+	       !system_unbound_wq || !system_freezable_wq ||
+		!system_nrt_freezable_wq);
 	return 0;
 }
 early_initcall(init_workqueues);

Re: system_nrt_wq, system suspend, and the freezer

[Jeff Layton]

On Thu, 16 Feb 2012 08:29:51 -0800
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
> > These should  all be freezable and we might even be able to get away
> > with WQ_UNBOUND for some of these.
> 
> In general, I would recommend specifying as few special attribute as
> possible.  If WQ_UNBOUND is necessary (large amount of CPU cycles
> consumed, extremely high concurrency), sure, but I think we're
> generally better off using as default attributes as possible.  It just
> makes things much easier later when we need to implement new features
> or update the implementation.
> 

Ok, fair enough. Probably no need to make it unbound...

> > I think we put most of these in system_nrt_wq because Tejun put an
> > earlier job into that queue when he converted it from slow_work and we
> > just cargo-cult copied that...
> > 
> > I'll spend some time looking at this in the next day or two, but I
> > suspect that the right answer is to just move these off of the "public"
> > workqueues altogether.
> 
> If freezing & nrt is everything necessary, just create
> system_nrt_freezable_wq and use that.
> 

The other problem here is that we really ought to be submitting the
write completion handler to a workqueue that has WQ_MEM_RECLAIM set.
Since none of the public wq's have that then I guess we'll have to make
our own?

-- 
Jeff Layton <jlayton@samba.org>

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

Hello, Jeff.

On Thu, Feb 16, 2012 at 01:59:45PM -0500, Jeff Layton wrote:
> The other problem here is that we really ought to be submitting the
> write completion handler to a workqueue that has WQ_MEM_RECLAIM set.
> Since none of the public wq's have that then I guess we'll have to make
> our own?

Yeap, if a rescuer should be there, a separate wq is necessary.

Thank you.

-- 
tejun

[PATCH] Block: use a freezable workqueue for disk-event polling

[Alan Stern]

This patch (as1519) fixes a bug in the block layer's disk-events
polling.  The polling is done by a work routine queued on the
system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
polling continues even in the middle of a system sleep transition.

Obviously, polling a suspended drive for media changes and such isn't
a good thing to do; in the case of USB mass-storage devices it can
lead to real problems requiring device resets and even re-enumeration.

The patch fixes things by creating a new system-wide, non-reentrant,
freezable workqueue and using it for disk-events polling.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Tejun Heo <tj@kernel.org>
CC: <stable@kernel.org>

---

I'm not sure who to send this patch to, since it is relevant to both
the block and PM subsystems.  Jens, is it okay if Rafael takes it?



 block/genhd.c             |   10 +++++-----
 include/linux/workqueue.h |    4 ++++
 kernel/workqueue.c        |    7 ++++++-
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: usb-3.3/block/genhd.c
===================================================================
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
 	intv = disk_events_poll_jiffies(disk);
 	set_timer_slack(&ev->dwork.timer, intv / 4);
 	if (check_now)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	else if (intv)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
 out_unlock:
 	spin_unlock_irqrestore(&ev->lock, flags);
 }
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
 	ev->clearing |= mask;
 	if (!ev->block) {
 		cancel_delayed_work(&ev->dwork);
-		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	}
 	spin_unlock_irq(&ev->lock);
 }
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
 
 	/* uncondtionally schedule event check and wait for it to finish */
 	disk_block_events(disk);
-	queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+	queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
 	flush_delayed_work(&ev->dwork);
 	__disk_unblock_events(disk, false);
 
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
 
 	intv = disk_events_poll_jiffies(disk);
 	if (!ev->block && intv)
-		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
 
 	spin_unlock_irq(&ev->lock);
 
Index: usb-3.3/include/linux/workqueue.h
===================================================================
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
  *
  * system_freezable_wq is equivalent to system_wq except that it's
  * freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
 extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
 
 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===================================================================
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
 struct workqueue_struct *system_nrt_wq __read_mostly;
 struct workqueue_struct *system_unbound_wq __read_mostly;
 struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
 EXPORT_SYMBOL_GPL(system_wq);
 EXPORT_SYMBOL_GPL(system_long_wq);
 EXPORT_SYMBOL_GPL(system_nrt_wq);
 EXPORT_SYMBOL_GPL(system_unbound_wq);
 EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/workqueue.h>
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
 					    WQ_UNBOUND_MAX_ACTIVE);
 	system_freezable_wq = alloc_workqueue("events_freezable",
 					      WQ_FREEZABLE, 0);
+	system_nrt_freezable_wq = alloc_workqueue("events_nrt_freezable",
+			WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
 	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-	       !system_unbound_wq || !system_freezable_wq);
+	       !system_unbound_wq || !system_freezable_wq ||
+		!system_nrt_freezable_wq);
 	return 0;
 }
 early_initcall(init_workqueues);

Re: [PATCH] Block: use a freezable workqueue for disk-event polling

[Tejun Heo]

On Fri, Feb 17, 2012 at 04:22:13PM -0500, Alan Stern wrote:
> This patch (as1519) fixes a bug in the block layer's disk-events
> polling.  The polling is done by a work routine queued on the
> system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
> polling continues even in the middle of a system sleep transition.
> 
> Obviously, polling a suspended drive for media changes and such isn't
> a good thing to do; in the case of USB mass-storage devices it can
> lead to real problems requiring device resets and even re-enumeration.
> 
> The patch fixes things by creating a new system-wide, non-reentrant,
> freezable workqueue and using it for disk-events polling.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Tejun Heo <tj@kernel.org>
> CC: <stable@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

-- 
tejun

Re: [PATCH] Block: use a freezable workqueue for disk-event polling

[Rafael J. Wysocki]

On Friday, February 17, 2012, Alan Stern wrote:
> This patch (as1519) fixes a bug in the block layer's disk-events
> polling.  The polling is done by a work routine queued on the
> system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
> polling continues even in the middle of a system sleep transition.
> 
> Obviously, polling a suspended drive for media changes and such isn't
> a good thing to do; in the case of USB mass-storage devices it can
> lead to real problems requiring device resets and even re-enumeration.
> 
> The patch fixes things by creating a new system-wide, non-reentrant,
> freezable workqueue and using it for disk-events polling.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Tejun Heo <tj@kernel.org>
> CC: <stable@kernel.org>

Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

Thanks,
Rafael


> ---
> 
> I'm not sure who to send this patch to, since it is relevant to both
> the block and PM subsystems.  Jens, is it okay if Rafael takes it?
> 
> 
> 
>  block/genhd.c             |   10 +++++-----
>  include/linux/workqueue.h |    4 ++++
>  kernel/workqueue.c        |    7 ++++++-
>  3 files changed, 15 insertions(+), 6 deletions(-)
> 
> Index: usb-3.3/block/genhd.c
> ===================================================================
> --- usb-3.3.orig/block/genhd.c
> +++ usb-3.3/block/genhd.c
> @@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
>  	intv = disk_events_poll_jiffies(disk);
>  	set_timer_slack(&ev->dwork.timer, intv / 4);
>  	if (check_now)
> -		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
> +		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
>  	else if (intv)
> -		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
> +		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
>  out_unlock:
>  	spin_unlock_irqrestore(&ev->lock, flags);
>  }
> @@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
>  	ev->clearing |= mask;
>  	if (!ev->block) {
>  		cancel_delayed_work(&ev->dwork);
> -		queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
> +		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
>  	}
>  	spin_unlock_irq(&ev->lock);
>  }
> @@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
>  
>  	/* uncondtionally schedule event check and wait for it to finish */
>  	disk_block_events(disk);
> -	queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
> +	queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, 0);
>  	flush_delayed_work(&ev->dwork);
>  	__disk_unblock_events(disk, false);
>  
> @@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
>  
>  	intv = disk_events_poll_jiffies(disk);
>  	if (!ev->block && intv)
> -		queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
> +		queue_delayed_work(system_nrt_freezable_wq, &ev->dwork, intv);
>  
>  	spin_unlock_irq(&ev->lock);
>  
> Index: usb-3.3/include/linux/workqueue.h
> ===================================================================
> --- usb-3.3.orig/include/linux/workqueue.h
> +++ usb-3.3/include/linux/workqueue.h
> @@ -289,12 +289,16 @@ enum {
>   *
>   * system_freezable_wq is equivalent to system_wq except that it's
>   * freezable.
> + *
> + * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
> + * it's freezable.
>   */
>  extern struct workqueue_struct *system_wq;
>  extern struct workqueue_struct *system_long_wq;
>  extern struct workqueue_struct *system_nrt_wq;
>  extern struct workqueue_struct *system_unbound_wq;
>  extern struct workqueue_struct *system_freezable_wq;
> +extern struct workqueue_struct *system_nrt_freezable_wq;
>  
>  extern struct workqueue_struct *
>  __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
> Index: usb-3.3/kernel/workqueue.c
> ===================================================================
> --- usb-3.3.orig/kernel/workqueue.c
> +++ usb-3.3/kernel/workqueue.c
> @@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
>  struct workqueue_struct *system_nrt_wq __read_mostly;
>  struct workqueue_struct *system_unbound_wq __read_mostly;
>  struct workqueue_struct *system_freezable_wq __read_mostly;
> +struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
>  EXPORT_SYMBOL_GPL(system_wq);
>  EXPORT_SYMBOL_GPL(system_long_wq);
>  EXPORT_SYMBOL_GPL(system_nrt_wq);
>  EXPORT_SYMBOL_GPL(system_unbound_wq);
>  EXPORT_SYMBOL_GPL(system_freezable_wq);
> +EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/workqueue.h>
> @@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
>  					    WQ_UNBOUND_MAX_ACTIVE);
>  	system_freezable_wq = alloc_workqueue("events_freezable",
>  					      WQ_FREEZABLE, 0);
> +	system_nrt_freezable_wq = alloc_workqueue("events_nrt_freezable",
> +			WQ_NON_REENTRANT | WQ_FREEZABLE, 0);
>  	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
> -	       !system_unbound_wq || !system_freezable_wq);
> +	       !system_unbound_wq || !system_freezable_wq ||
> +		!system_nrt_freezable_wq);
>  	return 0;
>  }
>  early_initcall(init_workqueues);
> 
> 
> 

Re: [PATCH] Block: use a freezable workqueue for disk-event polling

[Jens Axboe]

On 02/17/2012 10:22 PM, Alan Stern wrote:
> This patch (as1519) fixes a bug in the block layer's disk-events
> polling.  The polling is done by a work routine queued on the
> system_nrt_wq workqueue.  Since that workqueue isn't freezable, the
> polling continues even in the middle of a system sleep transition.
> 
> Obviously, polling a suspended drive for media changes and such isn't
> a good thing to do; in the case of USB mass-storage devices it can
> lead to real problems requiring device resets and even re-enumeration.
> 
> The patch fixes things by creating a new system-wide, non-reentrant,
> freezable workqueue and using it for disk-events polling.

Thanks Alan, picked up.

-- 
Jens Axboe

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

Hello, (cc'ing Rafael and Jens)

On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote:
> My question to all of you: Should system_nrt_wq be made freezable, or 
> should I create a new workqueue that is both freezable and 
> non-reentrant?  And if I do, which of the usages above should be 
> converted to the new workqueue?

Let's make it explicit that the wq is freezable.  I'm a bit
uncomfortable with the way it's headed.  What we should be doing is
implementing plugging of request queue for all regular requests while
suspend is in progress and then annotate the ones which should go
through.  We're trying to do it the other way around.

Also, in general, I don't think using freezing widely for kernel
threads / wqs is a good idea.  Plugging device access at subsystem
layer should cover most cases and we have notifiers to implement such
support and to handle special cases.  There are even code paths which
try to determine whether system went through PM operation by looking
at whether %current went through the freezer.  IMHO, we'll be better
off with removing freezer support for kthreads.  :(

Thanks.

-- 
tejun

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello, (cc'ing Rafael and Jens)
> 
> On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote:
> > My question to all of you: Should system_nrt_wq be made freezable, or 
> > should I create a new workqueue that is both freezable and 
> > non-reentrant?  And if I do, which of the usages above should be 
> > converted to the new workqueue?
> 
> Let's make it explicit that the wq is freezable.  I'm a bit
> uncomfortable with the way it's headed.  What we should be doing is
> implementing plugging of request queue for all regular requests while
> suspend is in progress and then annotate the ones which should go
> through.  We're trying to do it the other way around.

Um.  I don't think I can audit all the calls in the kernel that submit
block requests and determine which ones need to be allowed while a
system sleep is in progress.

> Also, in general, I don't think using freezing widely for kernel
> threads / wqs is a good idea.  Plugging device access at subsystem
> layer should cover most cases and we have notifiers to implement such
> support and to handle special cases.  There are even code paths which
> try to determine whether system went through PM operation by looking
> at whether %current went through the freezer.  IMHO, we'll be better
> off with removing freezer support for kthreads.  :(

Well, there are some dedicated threads that exist for no other purpose
than to do I/O to devices and to handle hotplug/unplug events.  I don't
see any reason not allow such threads to be freezable.  It's a quick, 
convenient method for getting them out of the way.

More general-purpose threads, like the async_schedule reservoir and 
workqueue threads, are a different story.

Alan Stern

Re: system_nrt_wq, system suspend, and the freezer

[Tejun Heo]

Hello,

On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote:
> Um.  I don't think I can audit all the calls in the kernel that submit
> block requests and determine which ones need to be allowed while a
> system sleep is in progress.

??? we need to do that anyway and the ones which should go through are
much smaller than the ones which shouldn't go through.

> > Also, in general, I don't think using freezing widely for kernel
> > threads / wqs is a good idea.  Plugging device access at subsystem
> > layer should cover most cases and we have notifiers to implement such
> > support and to handle special cases.  There are even code paths which
> > try to determine whether system went through PM operation by looking
> > at whether %current went through the freezer.  IMHO, we'll be better
> > off with removing freezer support for kthreads.  :(
> 
> Well, there are some dedicated threads that exist for no other purpose
> than to do I/O to devices and to handle hotplug/unplug events.  I don't
> see any reason not allow such threads to be freezable.  It's a quick, 
> convenient method for getting them out of the way.

Well, it's convenient to use incorrectly.  If you look at most of
freezable kthreads, they're sadly broken.  I mean, a lot of kthread
users don't even get kthread_should_stop() right.  With freezable()
thrown into the mix, it seems hopeless.  With wq, it's better as
freezing is handled by wq proper.  Even then, I don't know.  It just
seems to lead people to think "ooh, I marked it freezable so I don't
have to think about synchronization across PM events.  Freezer will
magically solve this for me!".  :(

Thanks.

-- 
tejun

Re: system_nrt_wq, system suspend, and the freezer

[Alan Stern]

On Thu, 16 Feb 2012, Tejun Heo wrote:

> Hello,
> 
> On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote:
> > Um.  I don't think I can audit all the calls in the kernel that submit
> > block requests and determine which ones need to be allowed while a
> > system sleep is in progress.
> 
> ??? we need to do that anyway and the ones which should go through are
> much smaller than the ones which shouldn't go through.

Agreed.  I'm just saying that it's too big a job for me to handle by
myself.  And all the marking has to be done before you plug the
unmarked requests; otherwise you could break hibernation.


> > Well, there are some dedicated threads that exist for no other purpose
> > than to do I/O to devices and to handle hotplug/unplug events.  I don't
> > see any reason not allow such threads to be freezable.  It's a quick, 
> > convenient method for getting them out of the way.
> 
> Well, it's convenient to use incorrectly.  If you look at most of
> freezable kthreads, they're sadly broken.  I mean, a lot of kthread
> users don't even get kthread_should_stop() right.  With freezable()
> thrown into the mix, it seems hopeless.  With wq, it's better as
> freezing is handled by wq proper.  Even then, I don't know.  It just
> seems to lead people to think "ooh, I marked it freezable so I don't
> have to think about synchronization across PM events.  Freezer will
> magically solve this for me!".  :(

Certainly there are issues which need to be considered carefully.  That 
doesn't mean it should never be used.

Alan Stern