blk-mq: Don't reserve a tag for flush request

PATCH In review Shaohua Li 2 participants 2 messages Dec 31, 2013 Block layer

Share & tools

Shaohua Li

author

2013-12-31 03:39 UTC #0 lore raw

PATCH 2/2 blk-mq: Don't reserve a tag for flush request

Reserving a tag (request) for flush to avoid dead lock is a overkill. tag is valuable resource. We can track flush request number and disallow too many pending flush requests allocated. With this patch, blk_mq_alloc_request_pinned() could do a busy nop (but not a dead loop) if too pending requests are allocated and new flush request is allocating. But this should not be a problem, too many pending flush requests are very rare case.

I verified this can fix the deadlock caused by too many pending flush requests.

Signed-off-by Shaohua Li <[email protected]>

---
 block/blk-flush.c      |    8 +++++---
 block/blk-mq.c         |   46 ++++++++++++++++++++++++++++++----------------
 include/linux/blk-mq.h |    3 +++
 3 files changed, 38 insertions(+), 19 deletions(-)

block/blk-flush.c +5 -3

Index: linux/block/blk-flush.c
===================================================================
@@ -284,9 +284,8 @@ static void mq_flush_work(struct work_st
 
 	q = container_of(work, struct request_queue, mq_flush_work);
 
-	/* We don't need set REQ_FLUSH_SEQ, it's for consistency */
 	rq = blk_mq_alloc_request(q, WRITE_FLUSH|REQ_FLUSH_SEQ,
-		__GFP_WAIT|GFP_ATOMIC, true);
+		__GFP_WAIT|GFP_ATOMIC, false);
 	rq->cmd_type = REQ_TYPE_FS;
 	rq->end_io = flush_end_io;

@@ -408,8 +407,11 @@ void blk_insert_flush(struct request *rq
 	/*
 	 * @policy now records what operations need to be done.  Adjust
 	 * REQ_FLUSH and FUA for the driver.
+	 * We keep REQ_FLUSH for mq to track flush requests. For !FUA,
+	 * we never dispatch the request directly.
 	 */
-	rq->cmd_flags &= ~REQ_FLUSH;
+	if (rq->cmd_flags & REQ_FUA)
+		rq->cmd_flags &= ~REQ_FLUSH;
 	if (!(fflags & REQ_FUA))
 		rq->cmd_flags &= ~REQ_FUA;

block/blk-mq.c +30 ~~-16~~

88 lines not on this page · raw message

include/linux/blk-mq.h +3 -0

Index: linux/include/linux/blk-mq.h
===================================================================
@@ -36,12 +36,15 @@ struct blk_mq_hw_ctx {
 	struct list_head	page_list;
 	struct blk_mq_tags	*tags;
 
+	atomic_t		pending_flush;
+
 	unsigned long		queued;
 	unsigned long		run;
 #define BLK_MQ_MAX_DISPATCH_ORDER	10
 	unsigned long		dispatched[BLK_MQ_MAX_DISPATCH_ORDER];
 
 	unsigned int		queue_depth;
+	unsigned int		reserved_tags;
 	unsigned int		numa_node;
 	unsigned int		cmd_size;	/* per-request extra data */

Jens Axboe

maintainer

2013-12-31 16:13 UTC #1 lore raw

Quoted 9 lines · Shaohua Li

Reserving a tag (request) for flush to avoid dead lock is a overkill. tag is valuable resource. We can track flush request number and disallow too many pending flush requests allocated. With this patch, blk_mq_alloc_request_pinned() could do a busy nop (but not a dead loop) if too pending requests are allocated and new flush request is allocating. But this should not be a problem, too many pending flush requests are very rare case.
I verified this can fix the deadlock caused by too many pending flush requests.

I think this looks pretty good, and it's a lot better than the hack that is currently in place. The atomic inc/dec should be a noop on a per-hctx basis, especially when it's only per-flush...

I'll get this queued up, with the ida fix too.

Signature · 1 line

Jens Axboe