[PATCH 1/6] block, sx8: kill blk_insert_request()

[PATCH 1/6] block, sx8: kill blk_insert_request()

[Tejun Heo]

The only user left for blk_insert_request() is sx8 and it can be
trivially switched to use blk_execute_rq_nowait() - special requests
aren't included in io stat and sx8 doesn't use block layer tagging.
Switch sx8 and kill blk_insert_requeset().

This patch doesn't introduce any functional difference.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jeff Garzik <jgarzik@pobox.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c       |   48 ------------------------------------------------
 drivers/block/sx8.c    |   12 ++++++++----
 include/linux/blkdev.h |    1 -
 3 files changed, 8 insertions(+), 53 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 7e15235..0f3baf4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1005,54 +1005,6 @@ static void add_acct_request(struct request_queue *q, struct request *rq,
 	__elv_add_request(q, rq, where);
 }
 
-/**
- * blk_insert_request - insert a special request into a request queue
- * @q:		request queue where request should be inserted
- * @rq:		request to be inserted
- * @at_head:	insert request at head or tail of queue
- * @data:	private data
- *
- * Description:
- *    Many block devices need to execute commands asynchronously, so they don't
- *    block the whole kernel from preemption during request execution.  This is
- *    accomplished normally by inserting aritficial requests tagged as
- *    REQ_TYPE_SPECIAL in to the corresponding request queue, and letting them
- *    be scheduled for actual execution by the request queue.
- *
- *    We have the option of inserting the head or the tail of the queue.
- *    Typically we use the tail for new ioctls and so forth.  We use the head
- *    of the queue for things like a QUEUE_FULL message from a device, or a
- *    host that is unable to accept a particular command.
- */
-void blk_insert_request(struct request_queue *q, struct request *rq,
-			int at_head, void *data)
-{
-	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
-	unsigned long flags;
-
-	/*
-	 * tell I/O scheduler that this isn't a regular read/write (ie it
-	 * must not attempt merges on this) and that it acts as a soft
-	 * barrier
-	 */
-	rq->cmd_type = REQ_TYPE_SPECIAL;
-
-	rq->special = data;
-
-	spin_lock_irqsave(q->queue_lock, flags);
-
-	/*
-	 * If command is tagged, release the tag
-	 */
-	if (blk_rq_tagged(rq))
-		blk_queue_end_tag(q, rq);
-
-	add_acct_request(q, rq, where);
-	__blk_run_queue(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-}
-EXPORT_SYMBOL(blk_insert_request);
-
 static void part_round_stats_single(int cpu, struct hd_struct *part,
 				    unsigned long now)
 {
diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index b70f0fc..e7472f5 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -619,8 +619,10 @@ static int carm_array_info (struct carm_host *host, unsigned int array_idx)
 	       host->state == HST_DEV_SCAN);
 	spin_unlock_irq(&host->lock);
 
-	DPRINTK("blk_insert_request, tag == %u\n", idx);
-	blk_insert_request(host->oob_q, crq->rq, 1, crq);
+	DPRINTK("blk_execute_rq_nowait, tag == %u\n", idx);
+	crq->rq->cmd_type = REQ_TYPE_SPECIAL;
+	crq->rq->special = crq;
+	blk_execute_rq_nowait(host->oob_q, NULL, crq->rq, true, NULL);
 
 	return 0;
 
@@ -658,8 +660,10 @@ static int carm_send_special (struct carm_host *host, carm_sspc_t func)
 	BUG_ON(rc < 0);
 	crq->msg_bucket = (u32) rc;
 
-	DPRINTK("blk_insert_request, tag == %u\n", idx);
-	blk_insert_request(host->oob_q, crq->rq, 1, crq);
+	DPRINTK("blk_execute_rq_nowait, tag == %u\n", idx);
+	crq->rq->cmd_type = REQ_TYPE_SPECIAL;
+	crq->rq->special = crq;
+	blk_execute_rq_nowait(host->oob_q, NULL, crq->rq, true, NULL);
 
 	return 0;
 }
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5267cd2..45563b0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -660,7 +660,6 @@ extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
 extern struct request *blk_make_request(struct request_queue *, struct bio *,
 					gfp_t);
-extern void blk_insert_request(struct request_queue *, struct request *, int, void *);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		unsigned int len);
-- 
1.7.3.1

Re: [PATCH 1/6] block, sx8: kill blk_insert_request()

[Jeff Garzik]

On 10/20/2011 11:56 PM, Tejun Heo wrote:
> The only user left for blk_insert_request() is sx8 and it can be
> trivially switched to use blk_execute_rq_nowait() - special requests
> aren't included in io stat and sx8 doesn't use block layer tagging.
> Switch sx8 and kill blk_insert_requeset().
>
> This patch doesn't introduce any functional difference.
>
> Only compile tested.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Jeff Garzik<jgarzik@pobox.com>
> Cc: Jens Axboe<axboe@kernel.dk>

Acked-by: Jeff Garzik <jgarzik@redhat.com>

[PATCHSET block:for-3.2/core] further updates to blk_cleanup_queue(), take#2

[Tejun Heo]

Hello,

This is the second take of "further updates to blk_cleanup_queue()"
patchset.  The only change from the last take[L] is rebase to
for-3.2/core and update to 0002 (and the cascades from it) so that it
runs queue synchrononously if called from sleepable context.  This
should resolve hch's concern about drivers which could use the path
frequently.

Original description follows.

Patchset "fix request_queue life-cycle management"[1] tried to fix
lifecycle management by making blk_cleanup_queue() drain and shut down
the queue; however, there still are some holes.  This patchset
tightens externally visible API a bit and plugs those holes.

 0001-block-sx8-kill-blk_insert_request.patch
 0002-block-allow-blk_execute_rq_nowait-to-be-called-from-.patch
 0003-block-ide-unexport-elv_add_request.patch
 0004-block-add-blk_queue_dead.patch
 0005-block-fix-drain_all-condition-in-blk_drain_queue.patch
 0006-block-add-missing-blk_queue_dead-checks.patch

0001-0003 remove/unexport two request insertion functions which don't
have proper DEAD check.  Users are switched to
blk_execute_rq_nowait().

0004 adds blk_queue_dead() macro for convenience.

0005 updates blk_drain_queue() such that it also waits for requests
which weren't allocated from block layer.

0006 adds missing DEAD checks.

This patchset is on top of the block:for-3.2/core 334c2b0b8b
"blk-throttle: use queue_is_locked() instead of lockdep_is_held()" and
available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-ref

diffstat follows.

 block/blk-core.c         |  105 ++++++++++++++++++++---------------------------
 block/blk-exec.c         |   34 +++++++++++----
 block/blk-flush.c        |    2 
 block/blk-sysfs.c        |    4 -
 block/blk-throttle.c     |    4 -
 block/blk.h              |    3 -
 block/elevator.c         |   16 +------
 drivers/block/sx8.c      |   12 +++--
 drivers/ide/ide-atapi.c  |    7 +--
 drivers/ide/ide-park.c   |    2 
 include/linux/blkdev.h   |    2 
 include/linux/elevator.h |    2 
 12 files changed, 94 insertions(+), 99 deletions(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.kernel/1206028
[1] http://thread.gmane.org/gmane.linux.kernel/1205150

[PATCH 1/6] block, sx8: kill blk_insert_request()

[Tejun Heo]

The only user left for blk_insert_request() is sx8 and it can be
trivially switched to use blk_execute_rq_nowait() - special requests
aren't included in io stat and sx8 doesn't use block layer tagging.
Switch sx8 and kill blk_insert_requeset().

This patch doesn't introduce any functional difference.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jeff Garzik <jgarzik@pobox.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c       |   48 ------------------------------------------------
 drivers/block/sx8.c    |   12 ++++++++----
 include/linux/blkdev.h |    1 -
 3 files changed, 8 insertions(+), 53 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f658711..a36738b 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1005,54 +1005,6 @@ static void add_acct_request(struct request_queue *q, struct request *rq,
 	__elv_add_request(q, rq, where);
 }
 
-/**
- * blk_insert_request - insert a special request into a request queue
- * @q:		request queue where request should be inserted
- * @rq:		request to be inserted
- * @at_head:	insert request at head or tail of queue
- * @data:	private data
- *
- * Description:
- *    Many block devices need to execute commands asynchronously, so they don't
- *    block the whole kernel from preemption during request execution.  This is
- *    accomplished normally by inserting aritficial requests tagged as
- *    REQ_TYPE_SPECIAL in to the corresponding request queue, and letting them
- *    be scheduled for actual execution by the request queue.
- *
- *    We have the option of inserting the head or the tail of the queue.
- *    Typically we use the tail for new ioctls and so forth.  We use the head
- *    of the queue for things like a QUEUE_FULL message from a device, or a
- *    host that is unable to accept a particular command.
- */
-void blk_insert_request(struct request_queue *q, struct request *rq,
-			int at_head, void *data)
-{
-	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
-	unsigned long flags;
-
-	/*
-	 * tell I/O scheduler that this isn't a regular read/write (ie it
-	 * must not attempt merges on this) and that it acts as a soft
-	 * barrier
-	 */
-	rq->cmd_type = REQ_TYPE_SPECIAL;
-
-	rq->special = data;
-
-	spin_lock_irqsave(q->queue_lock, flags);
-
-	/*
-	 * If command is tagged, release the tag
-	 */
-	if (blk_rq_tagged(rq))
-		blk_queue_end_tag(q, rq);
-
-	add_acct_request(q, rq, where);
-	__blk_run_queue(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-}
-EXPORT_SYMBOL(blk_insert_request);
-
 static void part_round_stats_single(int cpu, struct hd_struct *part,
 				    unsigned long now)
 {
diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index b70f0fc..e7472f5 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -619,8 +619,10 @@ static int carm_array_info (struct carm_host *host, unsigned int array_idx)
 	       host->state == HST_DEV_SCAN);
 	spin_unlock_irq(&host->lock);
 
-	DPRINTK("blk_insert_request, tag == %u\n", idx);
-	blk_insert_request(host->oob_q, crq->rq, 1, crq);
+	DPRINTK("blk_execute_rq_nowait, tag == %u\n", idx);
+	crq->rq->cmd_type = REQ_TYPE_SPECIAL;
+	crq->rq->special = crq;
+	blk_execute_rq_nowait(host->oob_q, NULL, crq->rq, true, NULL);
 
 	return 0;
 
@@ -658,8 +660,10 @@ static int carm_send_special (struct carm_host *host, carm_sspc_t func)
 	BUG_ON(rc < 0);
 	crq->msg_bucket = (u32) rc;
 
-	DPRINTK("blk_insert_request, tag == %u\n", idx);
-	blk_insert_request(host->oob_q, crq->rq, 1, crq);
+	DPRINTK("blk_execute_rq_nowait, tag == %u\n", idx);
+	crq->rq->cmd_type = REQ_TYPE_SPECIAL;
+	crq->rq->special = crq;
+	blk_execute_rq_nowait(host->oob_q, NULL, crq->rq, true, NULL);
 
 	return 0;
 }
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5267cd2..45563b0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -660,7 +660,6 @@ extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
 extern struct request *blk_make_request(struct request_queue *, struct bio *,
 					gfp_t);
-extern void blk_insert_request(struct request_queue *, struct request *, int, void *);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		unsigned int len);
-- 
1.7.3.1

[PATCH 2/6] block: allow blk_execute_rq_nowait() to be called from IRQ context

[Tejun Heo]

Currently blk_execute_rq_nowait() directly calls __blk_run_queue() and
thus must be called from sleepable context.  This patch updates the
function such that it can be called from non-sleepable context and
schedules async execution in such cases.  This will be used to
unexport elv_add_request().

While at it, add FIXME comment for REQ_TYPE_PM_RESUME special case.

-v2: hch pointed out that blk_execute_rq_nowait() can be hot path for
     some drivers.  Retained direct execution from sleepable context.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
---
 block/blk-exec.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/block/blk-exec.c b/block/blk-exec.c
index a1ebceb..b686f2b 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -49,6 +49,8 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 			   rq_end_io_fn *done)
 {
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
+	bool may_sleep = !preempt_count() && !irqs_disabled();
+	unsigned long flags;
 
 	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
 		rq->errors = -ENXIO;
@@ -59,14 +61,27 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 
 	rq->rq_disk = bd_disk;
 	rq->end_io = done;
-	WARN_ON(irqs_disabled());
-	spin_lock_irq(q->queue_lock);
+
+	spin_lock_irqsave(q->queue_lock, flags);
 	__elv_add_request(q, rq, where);
-	__blk_run_queue(q);
-	/* the queue is stopped so it won't be run */
-	if (rq->cmd_type == REQ_TYPE_PM_RESUME)
-		q->request_fn(q);
-	spin_unlock_irq(q->queue_lock);
+
+	/*
+	 * Some drivers beat this path pretty hard.  As an optimization, if
+	 * we're being called from sleepable context, run @q directly.
+	 */
+	if (may_sleep) {
+		__blk_run_queue(q);
+		/*
+		 * The queue is stopped so it won't be run.
+		 * FIXME: Please kill me along with REQ_TYPE_PM_RESUME.
+		 */
+		if (rq->cmd_type == REQ_TYPE_PM_RESUME)
+			q->request_fn(q);
+	} else {
+		blk_run_queue_async(q);
+	}
+
+	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 EXPORT_SYMBOL_GPL(blk_execute_rq_nowait);
 
-- 
1.7.3.1

[PATCH 3/6] block, ide: unexport elv_add_request()

[Tejun Heo]

The only elv_add_request() user outside of block layer proper is
ide-atapi and ide-park.  Now that blk_execute_rq_nowait() is allowed
from irq context, they can both be switched to
blk_execute_rq_nowait().  Switch IDE users, make [__]elv_add_request()
internal to block and drop the queue_lock grabbing version.

IDE changes lightly tested.  Block layer changes doesn't introduce any
behavior change.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c         |    6 +++---
 block/blk-exec.c         |    2 +-
 block/blk-flush.c        |    2 +-
 block/blk.h              |    1 +
 block/elevator.c         |   16 +++-------------
 drivers/ide/ide-atapi.c  |    7 ++++---
 drivers/ide/ide-park.c   |    2 +-
 include/linux/elevator.h |    2 --
 8 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a36738b..1efb943 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1002,7 +1002,7 @@ static void add_acct_request(struct request_queue *q, struct request *rq,
 			     int where)
 {
 	drive_stat_acct(rq, 1);
-	__elv_add_request(q, rq, where);
+	elv_add_request(q, rq, where);
 }
 
 static void part_round_stats_single(int cpu, struct hd_struct *part,
@@ -2763,9 +2763,9 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 		 * rq is already accounted, so use raw insert
 		 */
 		if (rq->cmd_flags & (REQ_FLUSH | REQ_FUA))
-			__elv_add_request(q, rq, ELEVATOR_INSERT_FLUSH);
+			elv_add_request(q, rq, ELEVATOR_INSERT_FLUSH);
 		else
-			__elv_add_request(q, rq, ELEVATOR_INSERT_SORT_MERGE);
+			elv_add_request(q, rq, ELEVATOR_INSERT_SORT_MERGE);
 
 		depth++;
 	}
diff --git a/block/blk-exec.c b/block/blk-exec.c
index b686f2b..951eda7 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -63,7 +63,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	rq->end_io = done;
 
 	spin_lock_irqsave(q->queue_lock, flags);
-	__elv_add_request(q, rq, where);
+	elv_add_request(q, rq, where);
 
 	/*
 	 * Some drivers beat this path pretty hard.  As an optimization, if
diff --git a/block/blk-flush.c b/block/blk-flush.c
index 720ad60..a21d43e 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -288,7 +288,7 @@ static void flush_data_end_io(struct request *rq, int error)
  * blk_insert_flush - insert a new FLUSH/FUA request
  * @rq: request to insert
  *
- * To be called from __elv_add_request() for %ELEVATOR_INSERT_FLUSH insertions.
+ * To be called from elv_add_request() for %ELEVATOR_INSERT_FLUSH insertions.
  * @rq is being submitted.  Analyze what needs to be done and put it on the
  * right queue.
  *
diff --git a/block/blk.h b/block/blk.h
index 3f6551b..77ad885 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -52,6 +52,7 @@ static inline void blk_clear_rq_complete(struct request *rq)
  */
 #define ELV_ON_HASH(rq)		(!hlist_unhashed(&(rq)->hash))
 
+void elv_add_request(struct request_queue *q, struct request *rq, int where);
 void blk_insert_flush(struct request *rq);
 void blk_abort_flushes(struct request_queue *q);
 
diff --git a/block/elevator.c b/block/elevator.c
index 66343d6..51447dd 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -599,7 +599,7 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
-	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
+	elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
 void elv_drain_elevator(struct request_queue *q)
@@ -636,8 +636,9 @@ void elv_quiesce_end(struct request_queue *q)
 	spin_unlock_irq(q->queue_lock);
 }
 
-void __elv_add_request(struct request_queue *q, struct request *rq, int where)
+void elv_add_request(struct request_queue *q, struct request *rq, int where)
 {
+	lockdep_is_held(q->queue_lock);
 	trace_block_rq_insert(q, rq);
 
 	rq->q = q;
@@ -715,17 +716,6 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 		BUG();
 	}
 }
-EXPORT_SYMBOL(__elv_add_request);
-
-void elv_add_request(struct request_queue *q, struct request *rq, int where)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	__elv_add_request(q, rq, where);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-}
-EXPORT_SYMBOL(elv_add_request);
 
 struct request *elv_latter_request(struct request_queue *q, struct request *rq)
 {
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 6f218e01..2f1b474 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -221,6 +221,8 @@ EXPORT_SYMBOL_GPL(ide_prep_sense);
 
 int ide_queue_sense_rq(ide_drive_t *drive, void *special)
 {
+	struct request *rq = &drive->sense_rq;
+
 	/* deferred failure from ide_prep_sense() */
 	if (!drive->sense_rq_armed) {
 		printk(KERN_WARNING PFX "%s: error queuing a sense request\n",
@@ -228,12 +230,11 @@ int ide_queue_sense_rq(ide_drive_t *drive, void *special)
 		return -ENOMEM;
 	}
 
-	drive->sense_rq.special = special;
 	drive->sense_rq_armed = false;
-
 	drive->hwif->rq = NULL;
 
-	elv_add_request(drive->queue, &drive->sense_rq, ELEVATOR_INSERT_FRONT);
+	rq->special = special;
+	blk_execute_rq_nowait(drive->queue, rq->rq_disk, rq, true, NULL);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ide_queue_sense_rq);
diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
index 6ab9ab2..0c64957 100644
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -52,7 +52,7 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
 	rq->cmd[0] = REQ_UNPARK_HEADS;
 	rq->cmd_len = 1;
 	rq->cmd_type = REQ_TYPE_SPECIAL;
-	elv_add_request(q, rq, ELEVATOR_INSERT_FRONT);
+	blk_execute_rq_nowait(q, NULL, rq, true, NULL);
 
 out:
 	return;
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 1d0f7a2..34d71f5 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -105,8 +105,6 @@ struct elevator_queue
  */
 extern void elv_dispatch_sort(struct request_queue *, struct request *);
 extern void elv_dispatch_add_tail(struct request_queue *, struct request *);
-extern void elv_add_request(struct request_queue *, struct request *, int);
-extern void __elv_add_request(struct request_queue *, struct request *, int);
 extern int elv_merge(struct request_queue *, struct request **, struct bio *);
 extern int elv_try_merge(struct request *, struct bio *);
 extern void elv_merge_requests(struct request_queue *, struct request *,
-- 
1.7.3.1

[PATCH 4/6] block: add blk_queue_dead()

[Tejun Heo]

There are a number of QUEUE_FLAG_DEAD tests.  Add blk_queue_dead()
macro and use it.

This patch doesn't introduce any functional difference.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c       |    6 +++---
 block/blk-exec.c       |    2 +-
 block/blk-sysfs.c      |    4 ++--
 block/blk-throttle.c   |    4 ++--
 block/blk.h            |    2 +-
 include/linux/blkdev.h |    1 +
 6 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1efb943..7d39897 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -603,7 +603,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue_node);
 
 int blk_get_queue(struct request_queue *q)
 {
-	if (likely(!test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
+	if (likely(!blk_queue_dead(q))) {
 		kobject_get(&q->kobj);
 		return 0;
 	}
@@ -750,7 +750,7 @@ static struct request *get_request(struct request_queue *q, int rw_flags,
 	const bool is_sync = rw_is_sync(rw_flags) != 0;
 	int may_queue;
 
-	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+	if (unlikely(blk_queue_dead(q)))
 		return NULL;
 
 	may_queue = elv_may_queue(q, rw_flags);
@@ -870,7 +870,7 @@ static struct request *get_request_wait(struct request_queue *q, int rw_flags,
 		struct io_context *ioc;
 		struct request_list *rl = &q->rq;
 
-		if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+		if (unlikely(blk_queue_dead(q)))
 			return NULL;
 
 		prepare_to_wait_exclusive(&rl->wait[is_sync], &wait,
diff --git a/block/blk-exec.c b/block/blk-exec.c
index 951eda7..8716557 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -52,7 +52,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	bool may_sleep = !preempt_count() && !irqs_disabled();
 	unsigned long flags;
 
-	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
+	if (unlikely(blk_queue_dead(q))) {
 		rq->errors = -ENXIO;
 		if (rq->end_io)
 			rq->end_io(rq, rq->errors);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e7f9f65..f0b2ca8 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -425,7 +425,7 @@ queue_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
 	if (!entry->show)
 		return -EIO;
 	mutex_lock(&q->sysfs_lock);
-	if (test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)) {
+	if (blk_queue_dead(q)) {
 		mutex_unlock(&q->sysfs_lock);
 		return -ENOENT;
 	}
@@ -447,7 +447,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 
 	q = container_of(kobj, struct request_queue, kobj);
 	mutex_lock(&q->sysfs_lock);
-	if (test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)) {
+	if (blk_queue_dead(q)) {
 		mutex_unlock(&q->sysfs_lock);
 		return -ENOENT;
 	}
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 4553245..5eed6a7 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -310,7 +310,7 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	struct request_queue *q = td->queue;
 
 	/* no throttling for dead queue */
-	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+	if (unlikely(blk_queue_dead(q)))
 		return NULL;
 
 	rcu_read_lock();
@@ -335,7 +335,7 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	spin_lock_irq(q->queue_lock);
 
 	/* Make sure @q is still alive */
-	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
+	if (unlikely(blk_queue_dead(q))) {
 		kfree(tg);
 		return NULL;
 	}
diff --git a/block/blk.h b/block/blk.h
index 77ad885..f8c797c 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -86,7 +86,7 @@ static inline struct request *__elv_next_request(struct request_queue *q)
 			q->flush_queue_delayed = 1;
 			return NULL;
 		}
-		if (test_bit(QUEUE_FLAG_DEAD, &q->queue_flags) ||
+		if (unlikely(blk_queue_dead(q)) ||
 		    !q->elevator->ops->elevator_dispatch_fn(q, 0))
 			return NULL;
 	}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 45563b0..6a4ab84 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -481,6 +481,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
+#define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
 #define blk_queue_noxmerges(q)	\
 	test_bit(QUEUE_FLAG_NOXMERGES, &(q)->queue_flags)
-- 
1.7.3.1

[PATCH 5/6] block: fix drain_all condition in blk_drain_queue()

[Tejun Heo]

When trying to drain all requests, blk_drain_queue() checked only
q->rq.count[]; however, this only tracks REQ_ALLOCED requests.  This
patch updates blk_drain_queue() such that it looks at all the counters
and queues so that request_queue is actually empty on completion.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 7d39897..4224e0a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -358,7 +358,8 @@ EXPORT_SYMBOL(blk_put_queue);
 void blk_drain_queue(struct request_queue *q, bool drain_all)
 {
 	while (true) {
-		int nr_rqs;
+		bool drain = false;
+		int i;
 
 		spin_lock_irq(q->queue_lock);
 
@@ -368,14 +369,25 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 
 		__blk_run_queue(q);
 
-		if (drain_all)
-			nr_rqs = q->rq.count[0] + q->rq.count[1];
-		else
-			nr_rqs = q->rq.elvpriv;
+		drain |= q->rq.elvpriv;
+
+		/*
+		 * Unfortunately, requests are queued at and tracked from
+		 * multiple places and there's no single counter which can
+		 * be drained.  Check all the queues and counters.
+		 */
+		if (drain_all) {
+			drain |= !list_empty(&q->queue_head);
+			for (i = 0; i < 2; i++) {
+				drain |= q->rq.count[i];
+				drain |= q->in_flight[i];
+				drain |= !list_empty(&q->flush_queue[i]);
+			}
+		}
 
 		spin_unlock_irq(q->queue_lock);
 
-		if (!nr_rqs)
+		if (!drain)
 			break;
 		msleep(10);
 	}
-- 
1.7.3.1

[PATCH 6/6] block: add missing blk_queue_dead() checks

[Tejun Heo]

blk_insert_cloned_request(), blk_execute_rq_nowait() and
blk_flush_plug_list() either didn't check whether the queue was dead
or did it without holding queue_lock.  Update them so that dead state
is checked while holding queue_lock.

AFAICS, this plugs all holes (requeue doesn't matter as the request is
transitioning atomically from in_flight to queued).

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 block/blk-core.c |   21 +++++++++++++++++++++
 block/blk-exec.c |    5 +++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 4224e0a..8267409 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1722,6 +1722,10 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)
 		return -EIO;
 
 	spin_lock_irqsave(q->queue_lock, flags);
+	if (unlikely(blk_queue_dead(q))) {
+		spin_unlock_irqrestore(q->queue_lock, flags);
+		return -ENODEV;
+	}
 
 	/*
 	 * Submitting request must be dequeued before calling this function
@@ -2696,6 +2700,14 @@ static void queue_unplugged(struct request_queue *q, unsigned int depth,
 	trace_block_unplug(q, depth, !from_schedule);
 
 	/*
+	 * Don't mess with dead queue.
+	 */
+	if (unlikely(blk_queue_dead(q))) {
+		spin_unlock(q->queue_lock);
+		return;
+	}
+
+	/*
 	 * If we are punting this to kblockd, then we can safely drop
 	 * the queue_lock before waking kblockd (which needs to take
 	 * this lock).
@@ -2771,6 +2783,15 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 			depth = 0;
 			spin_lock(q->queue_lock);
 		}
+
+		/*
+		 * Short-circuit if @q is dead
+		 */
+		if (unlikely(blk_queue_dead(q))) {
+			__blk_end_request_all(rq, -ENODEV);
+			continue;
+		}
+
 		/*
 		 * rq is already accounted, so use raw insert
 		 */
diff --git a/block/blk-exec.c b/block/blk-exec.c
index 8716557..660a722 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -52,7 +52,10 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	bool may_sleep = !preempt_count() && !irqs_disabled();
 	unsigned long flags;
 
+	spin_lock_irqsave(q->queue_lock, flags);
+
 	if (unlikely(blk_queue_dead(q))) {
+		spin_unlock_irqrestore(q->queue_lock, flags);
 		rq->errors = -ENXIO;
 		if (rq->end_io)
 			rq->end_io(rq, rq->errors);
@@ -61,8 +64,6 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 
 	rq->rq_disk = bd_disk;
 	rq->end_io = done;
-
-	spin_lock_irqsave(q->queue_lock, flags);
 	elv_add_request(q, rq, where);
 
 	/*
-- 
1.7.3.1

Re: [PATCH 1/6] block, sx8: kill blk_insert_request()

[Jeff Garzik]

On 10/25/2011 09:02 PM, Tejun Heo wrote:
> The only user left for blk_insert_request() is sx8 and it can be
> trivially switched to use blk_execute_rq_nowait() - special requests
> aren't included in io stat and sx8 doesn't use block layer tagging.
> Switch sx8 and kill blk_insert_requeset().
>
> This patch doesn't introduce any functional difference.
>
> Only compile tested.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Jeff Garzik<jgarzik@pobox.com>
> Cc: Jens Axboe<axboe@kernel.dk>

As before,

Acked-by: Jeff Garzik <jgarzik@redhat.com>

Re: [PATCH 2/6] block: allow blk_execute_rq_nowait() to be called from IRQ context

[Jens Axboe]

On 2011-10-26 03:02, Tejun Heo wrote:
> Currently blk_execute_rq_nowait() directly calls __blk_run_queue() and
> thus must be called from sleepable context.  This patch updates the
> function such that it can be called from non-sleepable context and
> schedules async execution in such cases.  This will be used to
> unexport elv_add_request().
> 
> While at it, add FIXME comment for REQ_TYPE_PM_RESUME special case.
> 
> -v2: hch pointed out that blk_execute_rq_nowait() can be hot path for
>      some drivers.  Retained direct execution from sleepable context.

Ugh, this looks nasty:

> +	bool may_sleep = !preempt_count() && !irqs_disabled();

please don't ever do that. Pass the context in instead.

> +	/*
> +	 * Some drivers beat this path pretty hard.  As an optimization, if
> +	 * we're being called from sleepable context, run @q directly.
> +	 */
> +	if (may_sleep) {
> +		__blk_run_queue(q);
> +		/*
> +		 * The queue is stopped so it won't be run.
> +		 * FIXME: Please kill me along with REQ_TYPE_PM_RESUME.
> +		 */
> +		if (rq->cmd_type == REQ_TYPE_PM_RESUME)
> +			q->request_fn(q);

This is very nasty as well.

-- 
Jens Axboe

Re: [PATCH 1/6] block, sx8: kill blk_insert_request()

[Jens Axboe]

On 2011-10-26 03:02, Tejun Heo wrote:
> The only user left for blk_insert_request() is sx8 and it can be
> trivially switched to use blk_execute_rq_nowait() - special requests
> aren't included in io stat and sx8 doesn't use block layer tagging.
> Switch sx8 and kill blk_insert_requeset().
> 
> This patch doesn't introduce any functional difference.

Applied.

-- 
Jens Axboe

Re: [PATCH 4/6] block: add blk_queue_dead()

[Jens Axboe]

On 2011-10-26 03:02, Tejun Heo wrote:
> There are a number of QUEUE_FLAG_DEAD tests.  Add blk_queue_dead()
> macro and use it.
> 
> This patch doesn't introduce any functional difference.

Applied 4-6 as well, thanks!

-- 
Jens Axboe

Re: [PATCH 4/6] block: add blk_queue_dead()

[Vivek Goyal]

On Tue, Oct 25, 2011 at 06:02:05PM -0700, Tejun Heo wrote:
> There are a number of QUEUE_FLAG_DEAD tests.  Add blk_queue_dead()
> macro and use it.
> 
> This patch doesn't introduce any functional difference.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>  block/blk-core.c       |    6 +++---
>  block/blk-exec.c       |    2 +-
>  block/blk-sysfs.c      |    4 ++--
>  block/blk-throttle.c   |    4 ++--
>  block/blk.h            |    2 +-
>  include/linux/blkdev.h |    1 +
>  6 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1efb943..7d39897 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -603,7 +603,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue_node);
>  
>  int blk_get_queue(struct request_queue *q)
>  {
> -	if (likely(!test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
> +	if (likely(!blk_queue_dead(q))) {
>  		kobject_get(&q->kobj);
>  		return 0;

I thought DEAD flag is now synchronized with queue lock. So the protocol
is that caller should be holding queue lock here first?

Thanks
Vivek

Re: [PATCH 2/6] block: allow blk_execute_rq_nowait() to be called from IRQ context

[Tejun Heo]

Hey, Jens.

On Wed, Oct 26, 2011 at 10:11:00AM +0200, Jens Axboe wrote:
> On 2011-10-26 03:02, Tejun Heo wrote:
> > Currently blk_execute_rq_nowait() directly calls __blk_run_queue() and
> > thus must be called from sleepable context.  This patch updates the
> > function such that it can be called from non-sleepable context and
> > schedules async execution in such cases.  This will be used to
> > unexport elv_add_request().
> > 
> > While at it, add FIXME comment for REQ_TYPE_PM_RESUME special case.
> > 
> > -v2: hch pointed out that blk_execute_rq_nowait() can be hot path for
> >      some drivers.  Retained direct execution from sleepable context.
> 
> Ugh, this looks nasty:
> 
> > +	bool may_sleep = !preempt_count() && !irqs_disabled();
> 
> please don't ever do that. Pass the context in instead.

Hmmm... I don't know.  This is strictly for optimization in block
layer proper.  It's kinda nasty to expose that, especially when the
interface is named _nowait and we already have separate request
allocation API.  Let's leave this alone for now.  The only offending
driver is ide anyway.

> > +	/*
> > +	 * Some drivers beat this path pretty hard.  As an optimization, if
> > +	 * we're being called from sleepable context, run @q directly.
> > +	 */
> > +	if (may_sleep) {
> > +		__blk_run_queue(q);
> > +		/*
> > +		 * The queue is stopped so it won't be run.
> > +		 * FIXME: Please kill me along with REQ_TYPE_PM_RESUME.
> > +		 */
> > +		if (rq->cmd_type == REQ_TYPE_PM_RESUME)
> > +			q->request_fn(q);
> 
> This is very nasty as well.

That's what the current code has.  We really need to take out those
odd request types.

Thanks.

-- 
tejun

Re: [PATCH 4/6] block: add blk_queue_dead()

[Tejun Heo]

Hello,

On Wed, Oct 26, 2011 at 01:20:49PM -0400, Vivek Goyal wrote:
> On Tue, Oct 25, 2011 at 06:02:05PM -0700, Tejun Heo wrote:
> > @@ -603,7 +603,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue_node);
> >  
> >  int blk_get_queue(struct request_queue *q)
> >  {
> > -	if (likely(!test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
> > +	if (likely(!blk_queue_dead(q))) {
> >  		kobject_get(&q->kobj);
> >  		return 0;
> 
> I thought DEAD flag is now synchronized with queue lock. So the protocol
> is that caller should be holding queue lock here first?

The requirement is that issue and processing of requests don't happen
once DEAD is set and to guarantee that it's necessary to set DEAD and
check DEAD in rq alloc/issue paths.

blk_get_queue() is inherently opportunistic as holding a reference
doesn't prevent it from being killed.  It doesn't make any sense to
require its holder to grab spinlock - either the caller doesn't need a
refcnt (as it's holding spinlock) or the result of the check becomes
stale as soon as it drops the spinlock.  Whether testing DEAD is
helpful (it isn't necessary per-se) is another question tho.

Thanks.

-- 
tejun