Skip to contentSkip to conversation

further updates to blk_cleanup_queue(), take#2

PATCH New Tejun Heo 4 participants 14 messages Oct 26, 2011

Subsystems (2)
Block layer cgroup blkio
lore
Share & tools
lore

Tejun Heo

author maintainer
#0 lore raw

PATCH 1/1 further updates to blk_cleanup_queue(), take#2

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

author maintainer
#1 lore raw

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

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 <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Jens Axboe <[email protected]>
---
 block/blk-core.c       |   48 ------------------------------------------------
 drivers/block/sx8.c    |   12 ++++++++----
 include/linux/blkdev.h |    1 -
 3 files changed, 8 insertions(+), 53 deletions(-)
block/blk-core.c +0 -48
index f658711..a36738b 100644
@@ -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)
 {
drivers/block/sx8.c +8 -4
index b70f0fc..e7472f5 100644
@@ -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;
 }
include/linux/blkdev.h +0 -1
index 5267cd2..45563b0 100644
@@ -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);
Signature · 1 line
1.7.3.1
Acked-by on v1 1/6
#2 lore raw

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

Quoted 5 lines · 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().

Quoted 7 lines

This patch doesn't introduce any functional difference.

Only compile tested.

Signed-off-by Tejun Heo<[email protected]>
Cc Jeff Garzik<[email protected]>
Cc Jens Axboe<[email protected]>

As before,

Acked-by Jeff Garzik <[email protected]>
maintainer Applied
#3 lore raw

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

In reply to Tejun Heo
Quoted 6 lines · 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.

Applied.

Signature · 1 line
Jens Axboe
author maintainer
#4 lore raw

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

In reply to 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 <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 block/blk-exec.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)
block/blk-exec.c +22 -7
index a1ebceb..b686f2b 100644
@@ -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);
Signature · 1 line
1.7.3.1
maintainer
#5 lore raw

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

Quoted 10 lines · 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.

Ugh, this looks nasty:

block/blk-exec.c void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, · line 52

Quoted 1 line
block/blk-exec.c +1 -0
+	bool may_sleep = !preempt_count() && !irqs_disabled();

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

block/blk-exec.c void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, · line 79

Quoted 12 lines
block/blk-exec.c +12 -0
+	/*
+	 * 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.

Signature · 1 line
Jens Axboe
author maintainer
#6 lore raw

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

Hey, Jens.

Quoted 9 lines · Jens Axboe
Quoted 8 lines · 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.

Quoted 8 lines
Quoted 2 lines

-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:

block/blk-exec.c void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, · line 52

Quoted 1 line
block/blk-exec.c +1 -0
+	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.

Quoted 6 lines
Quoted 6 lines
block/blk-exec.c +6 -0
+	/*
+	 * 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);
Quoted 8 lines

block/blk-exec.c void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk, · line 79

Quoted 6 lines
block/blk-exec.c +6 -0
+		/*
+		 * 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.

Signature · 1 line
tejun
author maintainer
#7 lore raw

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

In reply to 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 <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Jens Axboe <[email protected]>
---
 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(-)
block/blk-core.c +3 -3
index a36738b..1efb943 100644
@@ -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++;
 	}
block/blk-exec.c +1 -1
index b686f2b..951eda7 100644
@@ -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
block/blk-flush.c +1 -1
index 720ad60..a21d43e 100644
@@ -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.
  *
block/blk.h +1 -0
index 3f6551b..77ad885 100644
@@ -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);
block/elevator.c +3 -13
index 66343d6..51447dd 100644
@@ -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)
 {
drivers/ide/ide-atapi.c +4 -3
index 6f218e01..2f1b474 100644
@@ -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);
drivers/ide/ide-park.c +1 -1
index 6ab9ab2..0c64957 100644
@@ -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;
include/linux/elevator.h +0 -2
index 1d0f7a2..34d71f5 100644
@@ -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 *,
Signature · 1 line
1.7.3.1
author maintainer
#8 lore raw

PATCH 4/6 block: add blk_queue_dead()

In reply to 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 <[email protected]>
Cc Jens Axboe <[email protected]>
---
 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(-)
block/blk-core.c +3 -3
index 1efb943..7d39897 100644
@@ -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,
block/blk-exec.c +1 -1
index 951eda7..8716557 100644
@@ -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);
block/blk-sysfs.c +2 -2
index e7f9f65..f0b2ca8 100644
@@ -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;
 	}
block/blk-throttle.c +2 -2
index 4553245..5eed6a7 100644
@@ -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;
 	}
block/blk.h +1 -1
index 77ad885..f8c797c 100644
@@ -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;
 	}
include/linux/blkdev.h +1 -0
index 45563b0..6a4ab84 100644
@@ -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)
Signature · 1 line
1.7.3.1
maintainer
#9 lore raw

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

Quoted 4 lines · 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.

Applied 4-6 as well, thanks!

Signature · 1 line
Jens Axboe
#10 lore raw

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

In reply to Tejun Heo
Quoted 21 lines · 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 <[email protected]>
Cc Jens Axboe <[email protected]>
---
 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(-)
block/blk-core.c +0 -0
index 1efb943..7d39897 100644

block/blk-core.c EXPORT_SYMBOL(blk_init_allocated_queue_node); · line 608

Quoted 7 lines
block/blk-core.c +1 -1
@@ -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

author maintainer
#11 lore raw

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

Hello,

Quoted 12 lines · Vivek Goyal

block/blk-core.c EXPORT_SYMBOL(blk_init_allocated_queue_node); · line 608

Quoted 8 lines · Tejun Heo
block/blk-core.c +1 -1
@@ -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.

Signature · 1 line
tejun
author maintainer
#12 lore raw

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

In reply to 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 <[email protected]>
Cc Jens Axboe <[email protected]>
---
 block/blk-core.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)
block/blk-core.c +18 -6
index 7d39897..4224e0a 100644
@@ -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);
 	}
Signature · 1 line
1.7.3.1
author maintainer
#13 lore raw

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

In reply to 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 <[email protected]>
Cc Jens Axboe <[email protected]>
---
 block/blk-core.c |   21 +++++++++++++++++++++
 block/blk-exec.c |    5 +++--
 2 files changed, 24 insertions(+), 2 deletions(-)
block/blk-core.c +21 -0
index 4224e0a..8267409 100644
@@ -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
 		 */
block/blk-exec.c +3 -2
index 8716557..660a722 100644
@@ -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);
 
 	/*
Signature · 1 line
1.7.3.1