Re: [PATCH v6 3/6] block: revert back to synchronous request_queue removal

2020-06-19 Thread Luis Chamberlain
On Fri, Jun 12, 2020 at 06:53:40PM -0700, Bart Van Assche wrote:
> On 2020-06-08 10:01, Luis Chamberlain wrote:
> > + * Drivers exist which depend on the release of the request_queue to be
> > + * synchronous, it should not be deferred.
> 
> This sounds mysterious. Which drivers? Why do these depend on this
> function being synchronous?

Sorry that should be "Userspace can exist". I've fixed that.

> Anyway:
> 
> Reviewed-by: Bart Van Assche 

  Luis


Re: [PATCH v6 3/6] block: revert back to synchronous request_queue removal

2020-06-12 Thread Bart Van Assche
On 2020-06-08 10:01, Luis Chamberlain wrote:
> + * Drivers exist which depend on the release of the request_queue to be
> + * synchronous, it should not be deferred.

This sounds mysterious. Which drivers? Why do these depend on this
function being synchronous? Anyway:

Reviewed-by: Bart Van Assche 


[PATCH v6 3/6] block: revert back to synchronous request_queue removal

2020-06-08 Thread Luis Chamberlain
Commit dc9edc44de6c ("block: Fix a blk_exit_rl() regression") merged on
v4.12 moved the work behind blk_release_queue() into a workqueue after a
splat floated around which indicated some work on blk_release_queue()
could sleep in blk_exit_rl(). This splat would be possible when a driver
called blk_put_queue() or blk_cleanup_queue() (which calls blk_put_queue()
as its final call) from an atomic context.

blk_put_queue() decrements the refcount for the request_queue kobject,
and upon reaching 0 blk_release_queue() is called. Although blk_exit_rl()
is now removed through commit db6d9952356 ("block: remove request_list code")
on v5.0, we reserve the right to be able to sleep within blk_release_queue()
context.

The last reference for the request_queue must not be called from atomic
context. *When* the last reference to the request_queue reaches 0 varies,
and so let's take the opportunity to document when that is expected to
happen and also document the context of the related calls as best as possible
so we can avoid future issues, and with the hopes that the synchronous
request_queue removal sticks.

We revert back to synchronous request_queue removal because asynchronous
removal creates a regression with expected userspace interaction with
several drivers. An example is when removing the loopback driver, one
uses ioctls from userspace to do so, but upon return and if successful,
one expects the device to be removed. Likewise if one races to add another
device the new one may not be added as it is still being removed. This was
expected behavior before and it now fails as the device is still present
and busy still. Moving to asynchronous request_queue removal could have
broken many scripts which relied on the removal to have been completed if
there was no error. Document this expectation as well so that this
doesn't regress userspace again.

Using asynchronous request_queue removal however has helped us find
other bugs. In the future we can test what could break with this
arrangement by enabling CONFIG_DEBUG_KOBJECT_RELEASE.

While at it, update the docs with the context expectations for the
request_queue / gendisk refcount decrement, and make these
expectations explicit by using might_sleep().

Cc: Bart Van Assche 
Cc: Omar Sandoval 
Cc: Hannes Reinecke 
Cc: Nicolai Stange 
Cc: Greg Kroah-Hartman 
Cc: Michal Hocko 
Cc: yu kuai 
Suggested-by: Nicolai Stange 
Fixes: dc9edc44de6c ("block: Fix a blk_exit_rl() regression")
Reviewed-by: Christoph Hellwig 
Reviewed-by: Bart Van Assche 
Signed-off-by: Luis Chamberlain 
---
 block/blk-core.c   |  8 
 block/blk-sysfs.c  | 43 +-
 block/genhd.c  | 17 +
 include/linux/blkdev.h |  2 --
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 14c09daf55f3..a5126c0be777 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -327,6 +327,9 @@ EXPORT_SYMBOL_GPL(blk_clear_pm_only);
  *
  * Decrements the refcount of the request_queue kobject. When this reaches 0
  * we'll have blk_release_queue() called.
+ *
+ * Context: Any context, but the last reference must not be dropped from
+ *  atomic context.
  */
 void blk_put_queue(struct request_queue *q)
 {
@@ -359,9 +362,14 @@ EXPORT_SYMBOL_GPL(blk_set_queue_dying);
  *
  * Mark @q DYING, drain all pending requests, mark @q DEAD, destroy and
  * put it.  All future requests will be failed immediately with -ENODEV.
+ *
+ * Context: can sleep
  */
 void blk_cleanup_queue(struct request_queue *q)
 {
+   /* cannot be called from atomic context */
+   might_sleep();
+
WARN_ON_ONCE(blk_queue_registered(q));
 
/* mark @q DYING, no new request or merges will be allowed afterwards */
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 02643e149d5e..561624d4cc4e 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -873,22 +873,32 @@ static void blk_exit_queue(struct request_queue *q)
bdi_put(q->backing_dev_info);
 }
 
-
 /**
- * __blk_release_queue - release a request queue
- * @work: pointer to the release_work member of the request queue to be 
released
+ * blk_release_queue - releases all allocated resources of the request_queue
+ * @kobj: pointer to a kobject, whose container is a request_queue
+ *
+ * This function releases all allocated resources of the request queue.
+ *
+ * The struct request_queue refcount is incremented with blk_get_queue() and
+ * decremented with blk_put_queue(). Once the refcount reaches 0 this function
+ * is called.
+ *
+ * For drivers that have a request_queue on a gendisk and added with
+ * __device_add_disk() the refcount to request_queue will reach 0 with
+ * the last put_disk() called by the driver. For drivers which don't use
+ * __device_add_disk() this happens with blk_cleanup_queue().
  *
- * Description:
- * This function is called when a block device is being unregistered. The
- * process of releasing a request