diff mbox series

[v2,3/4] blk-flush: kill the flush state machine

Message ID 20230725130102.3030032-4-chengming.zhou@linux.dev (mailing list archive)
State New, archived
Headers show
Series blk-flush: optimize non-postflush requests | expand

Commit Message

Chengming Zhou July 25, 2023, 1:01 p.m. UTC
From: Chengming Zhou <zhouchengming@bytedance.com>

Since now we put preflush and postflush requests in separate queues,
we don't need the flush sequence to record anymore.

REQ_FSEQ_PREFLUSH: blk_enqueue_preflush()
REQ_FSEQ_POSTFLUSH: blk_enqueue_postflush()
REQ_FSEQ_DONE: blk_end_flush()

In blk_flush_complete(), we have two list to handle: preflush_running
and postflush_running. We just blk_end_flush() directly for postflush
requests, but need to move preflush requests to requeue_list to
dispatch.

This patch just kill the flush state machine and directly call these
functions, in preparation for the next patch.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-flush.c      | 158 ++++++++++++++++++-----------------------
 include/linux/blk-mq.h |   1 -
 2 files changed, 70 insertions(+), 89 deletions(-)

Comments

Christoph Hellwig July 31, 2023, 6:19 a.m. UTC | #1
On Tue, Jul 25, 2023 at 09:01:01PM +0800, chengming.zhou@linux.dev wrote:
> From: Chengming Zhou <zhouchengming@bytedance.com>
> 
> Since now we put preflush and postflush requests in separate queues,
> we don't need the flush sequence to record anymore.
> 
> REQ_FSEQ_PREFLUSH: blk_enqueue_preflush()
> REQ_FSEQ_POSTFLUSH: blk_enqueue_postflush()
> REQ_FSEQ_DONE: blk_end_flush()
> 
> In blk_flush_complete(), we have two list to handle: preflush_running
> and postflush_running. We just blk_end_flush() directly for postflush
> requests, but need to move preflush requests to requeue_list to
> dispatch.
> 
> This patch just kill the flush state machine and directly call these
> functions, in preparation for the next patch.

> +static void blk_enqueue_postflush(struct request *rq, struct blk_flush_queue *fq)

Please avoid the overly long here.  Maybe just rename enqueue to queue
here and for the preflush version as we don't really use enqueue in
the flush code anyway.

> +{
> +	unsigned int nr_requeue = 0;
> +	struct list_head *preflush_running;
> +	struct list_head *postflush_running;
> +	struct request *rq, *n;
> +
> +	preflush_running = &fq->preflush_queue[fq->flush_running_idx];
> +	postflush_running = &fq->postflush_queue[fq->flush_running_idx];

I'd initialize these ad declaration time:

	struct list_head *preflush_running =
		&fq->preflush_queue[fq->flush_running_idx];
	struct list_head *postflush_running =
		&fq->postflush_queue[fq->flush_running_idx];
	unsigned int nr_requeue = 0;
	struct request *rq, *n;

> +
> +	list_for_each_entry_safe(rq, n, postflush_running, queuelist) {
> +		blk_end_flush(rq, fq, error);
>  	}

No need for the braces.
Chengming Zhou July 31, 2023, 2:27 p.m. UTC | #2
On 2023/7/31 14:19, Christoph Hellwig wrote:
> On Tue, Jul 25, 2023 at 09:01:01PM +0800, chengming.zhou@linux.dev wrote:
>> From: Chengming Zhou <zhouchengming@bytedance.com>
>>
>> Since now we put preflush and postflush requests in separate queues,
>> we don't need the flush sequence to record anymore.
>>
>> REQ_FSEQ_PREFLUSH: blk_enqueue_preflush()
>> REQ_FSEQ_POSTFLUSH: blk_enqueue_postflush()
>> REQ_FSEQ_DONE: blk_end_flush()
>>
>> In blk_flush_complete(), we have two list to handle: preflush_running
>> and postflush_running. We just blk_end_flush() directly for postflush
>> requests, but need to move preflush requests to requeue_list to
>> dispatch.
>>
>> This patch just kill the flush state machine and directly call these
>> functions, in preparation for the next patch.
> 
>> +static void blk_enqueue_postflush(struct request *rq, struct blk_flush_queue *fq)
> 
> Please avoid the overly long here.  Maybe just rename enqueue to queue
> here and for the preflush version as we don't really use enqueue in
> the flush code anyway.

Ok, will rename to queue.

> 
>> +{
>> +	unsigned int nr_requeue = 0;
>> +	struct list_head *preflush_running;
>> +	struct list_head *postflush_running;
>> +	struct request *rq, *n;
>> +
>> +	preflush_running = &fq->preflush_queue[fq->flush_running_idx];
>> +	postflush_running = &fq->postflush_queue[fq->flush_running_idx];
> 
> I'd initialize these ad declaration time:
> 
> 	struct list_head *preflush_running =
> 		&fq->preflush_queue[fq->flush_running_idx];
> 	struct list_head *postflush_running =
> 		&fq->postflush_queue[fq->flush_running_idx];
> 	unsigned int nr_requeue = 0;
> 	struct request *rq, *n;
> 

LGTM, will change these.

Thanks for your review!

>> +
>> +	list_for_each_entry_safe(rq, n, postflush_running, queuelist) {
>> +		blk_end_flush(rq, fq, error);
>>  	}
> 
> No need for the braces.
>
diff mbox series

Patch

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 4993c3c3b502..ed195c760617 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -117,11 +117,6 @@  static unsigned int blk_flush_policy(unsigned long fflags, struct request *rq)
 	return policy;
 }
 
-static unsigned int blk_flush_cur_seq(struct request *rq)
-{
-	return 1 << ffz(rq->flush.seq);
-}
-
 static void blk_flush_restore_request(struct request *rq)
 {
 	/*
@@ -147,75 +142,81 @@  static void blk_account_io_flush(struct request *rq)
 	part_stat_unlock();
 }
 
-/**
- * blk_flush_complete_seq - complete flush sequence
- * @rq: PREFLUSH/FUA request being sequenced
- * @fq: flush queue
- * @seq: sequences to complete (mask of %REQ_FSEQ_*, can be zero)
- * @error: whether an error occurred
- *
- * @rq just completed @seq part of its flush sequence, record the
- * completion and trigger the next step.
- *
- * CONTEXT:
- * spin_lock_irq(fq->mq_flush_lock)
- */
-static void blk_flush_complete_seq(struct request *rq,
-				   struct blk_flush_queue *fq,
-				   unsigned int seq, blk_status_t error)
+static void blk_enqueue_preflush(struct request *rq, struct blk_flush_queue *fq)
 {
 	struct request_queue *q = rq->q;
-	struct list_head *pending;
+	struct list_head *pending = &fq->preflush_queue[fq->flush_pending_idx];
 
-	BUG_ON(rq->flush.seq & seq);
-	rq->flush.seq |= seq;
+	if (!fq->flush_pending_since)
+		fq->flush_pending_since = jiffies;
+	list_move_tail(&rq->queuelist, pending);
 
-	if (likely(!error))
-		seq = blk_flush_cur_seq(rq);
-	else
-		seq = REQ_FSEQ_DONE;
+	blk_kick_flush(q, fq);
+}
 
-	switch (seq) {
-	case REQ_FSEQ_PREFLUSH:
-		pending = &fq->preflush_queue[fq->flush_pending_idx];
-		/* queue for flush */
-		if (!fq->flush_pending_since)
-			fq->flush_pending_since = jiffies;
-		list_move_tail(&rq->queuelist, pending);
-		break;
+static void blk_enqueue_postflush(struct request *rq, struct blk_flush_queue *fq)
+{
+	struct request_queue *q = rq->q;
+	struct list_head *pending = &fq->postflush_queue[fq->flush_pending_idx];
 
-	case REQ_FSEQ_DATA:
-		fq->flush_data_in_flight++;
-		spin_lock(&q->requeue_lock);
-		list_move(&rq->queuelist, &q->requeue_list);
-		spin_unlock(&q->requeue_lock);
-		blk_mq_kick_requeue_list(q);
-		break;
+	if (!fq->flush_pending_since)
+		fq->flush_pending_since = jiffies;
+	list_move_tail(&rq->queuelist, pending);
 
-	case REQ_FSEQ_POSTFLUSH:
-		pending = &fq->postflush_queue[fq->flush_pending_idx];
-		/* queue for flush */
-		if (!fq->flush_pending_since)
-			fq->flush_pending_since = jiffies;
-		list_move_tail(&rq->queuelist, pending);
-		break;
+	blk_kick_flush(q, fq);
+}
 
-	case REQ_FSEQ_DONE:
-		/*
-		 * @rq was previously adjusted by blk_insert_flush() for
-		 * flush sequencing and may already have gone through the
-		 * flush data request completion path.  Restore @rq for
-		 * normal completion and end it.
-		 */
-		list_del_init(&rq->queuelist);
-		blk_flush_restore_request(rq);
-		blk_mq_end_request(rq, error);
-		break;
+static void blk_end_flush(struct request *rq, struct blk_flush_queue *fq,
+			  blk_status_t error)
+{
+	struct request_queue *q = rq->q;
 
-	default:
-		BUG();
+	/*
+	 * @rq was previously adjusted by blk_insert_flush() for
+	 * flush sequencing and may already have gone through the
+	 * flush data request completion path.  Restore @rq for
+	 * normal completion and end it.
+	 */
+	list_del_init(&rq->queuelist);
+	blk_flush_restore_request(rq);
+	blk_mq_end_request(rq, error);
+
+	blk_kick_flush(q, fq);
+}
+
+static void blk_flush_complete(struct request_queue *q,
+			       struct blk_flush_queue *fq,
+			       blk_status_t error)
+{
+	unsigned int nr_requeue = 0;
+	struct list_head *preflush_running;
+	struct list_head *postflush_running;
+	struct request *rq, *n;
+
+	preflush_running = &fq->preflush_queue[fq->flush_running_idx];
+	postflush_running = &fq->postflush_queue[fq->flush_running_idx];
+
+	list_for_each_entry_safe(rq, n, postflush_running, queuelist) {
+		blk_end_flush(rq, fq, error);
 	}
 
+	list_for_each_entry_safe(rq, n, preflush_running, queuelist) {
+		if (unlikely(error || !blk_rq_sectors(rq)))
+			blk_end_flush(rq, fq, error);
+		else
+			nr_requeue++;
+	}
+
+	if (nr_requeue) {
+		fq->flush_data_in_flight += nr_requeue;
+		spin_lock(&q->requeue_lock);
+		list_splice_init(preflush_running, &q->requeue_list);
+		spin_unlock(&q->requeue_lock);
+		blk_mq_kick_requeue_list(q);
+	}
+
+	/* account completion of the flush request */
+	fq->flush_running_idx ^= 1;
 	blk_kick_flush(q, fq);
 }
 
@@ -223,8 +224,6 @@  static enum rq_end_io_ret flush_end_io(struct request *flush_rq,
 				       blk_status_t error)
 {
 	struct request_queue *q = flush_rq->q;
-	struct list_head *preflush_running, *postflush_running;
-	struct request *rq, *n;
 	unsigned long flags = 0;
 	struct blk_flush_queue *fq = blk_get_flush_queue(q, flush_rq->mq_ctx);
 
@@ -256,27 +255,9 @@  static enum rq_end_io_ret flush_end_io(struct request *flush_rq,
 		flush_rq->internal_tag = BLK_MQ_NO_TAG;
 	}
 
-	preflush_running = &fq->preflush_queue[fq->flush_running_idx];
-	postflush_running = &fq->postflush_queue[fq->flush_running_idx];
 	BUG_ON(fq->flush_pending_idx == fq->flush_running_idx);
 
-	/* account completion of the flush request */
-	fq->flush_running_idx ^= 1;
-
-	/* and push the waiting requests to the next stage */
-	list_for_each_entry_safe(rq, n, preflush_running, queuelist) {
-		unsigned int seq = blk_flush_cur_seq(rq);
-
-		BUG_ON(seq != REQ_FSEQ_PREFLUSH && seq != REQ_FSEQ_POSTFLUSH);
-		blk_flush_complete_seq(rq, fq, seq, error);
-	}
-
-	list_for_each_entry_safe(rq, n, postflush_running, queuelist) {
-		unsigned int seq = blk_flush_cur_seq(rq);
-
-		BUG_ON(seq != REQ_FSEQ_PREFLUSH && seq != REQ_FSEQ_POSTFLUSH);
-		blk_flush_complete_seq(rq, fq, seq, error);
-	}
+	blk_flush_complete(q, fq, error);
 
 	spin_unlock_irqrestore(&fq->mq_flush_lock, flags);
 	return RQ_END_IO_NONE;
@@ -401,7 +382,10 @@  static enum rq_end_io_ret mq_flush_data_end_io(struct request *rq,
 	 * re-initialize rq->queuelist before reusing it here.
 	 */
 	INIT_LIST_HEAD(&rq->queuelist);
-	blk_flush_complete_seq(rq, fq, REQ_FSEQ_DATA, error);
+	if (likely(!error))
+		blk_enqueue_postflush(rq, fq);
+	else
+		blk_end_flush(rq, fq, error);
 	spin_unlock_irqrestore(&fq->mq_flush_lock, flags);
 
 	blk_mq_sched_restart(hctx);
@@ -410,7 +394,6 @@  static enum rq_end_io_ret mq_flush_data_end_io(struct request *rq,
 
 static void blk_rq_init_flush(struct request *rq)
 {
-	rq->flush.seq = 0;
 	rq->rq_flags |= RQF_FLUSH_SEQ;
 	rq->flush.saved_end_io = rq->end_io; /* Usually NULL */
 	rq->end_io = mq_flush_data_end_io;
@@ -469,7 +452,6 @@  bool blk_insert_flush(struct request *rq)
 		 * the post flush, and then just pass the command on.
 		 */
 		blk_rq_init_flush(rq);
-		rq->flush.seq |= REQ_FSEQ_PREFLUSH;
 		spin_lock_irq(&fq->mq_flush_lock);
 		fq->flush_data_in_flight++;
 		spin_unlock_irq(&fq->mq_flush_lock);
@@ -481,7 +463,7 @@  bool blk_insert_flush(struct request *rq)
 		 */
 		blk_rq_init_flush(rq);
 		spin_lock_irq(&fq->mq_flush_lock);
-		blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
+		blk_enqueue_preflush(rq, fq);
 		spin_unlock_irq(&fq->mq_flush_lock);
 		return true;
 	}
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 01e8c31db665..d46fefdacea8 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -177,7 +177,6 @@  struct request {
 	} elv;
 
 	struct {
-		unsigned int		seq;
 		rq_end_io_fn		*saved_end_io;
 	} flush;