RQF_PM fix/cleanup

RQF_PM fix/cleanup

[Christoph Hellwig]

Hi Jens,

this series fixed the blocking/passing of requests during PM IFF there were
more than one.  That currently isn't the case, but relying on that not only
is fragile, but also leads to more obscure code than handling it properly.

[PATCH 1/2] block: move __elv_next_request to blk-core.c

[Christoph Hellwig]

No need to have this helper inline in a header.  Also drop the __ prefix.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 block/blk.h      | 39 ---------------------------------------
 2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 048be4aa6024..14f7674fa0b1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2517,6 +2517,45 @@ void blk_account_io_start(struct request *rq, bool new_io)
 	part_stat_unlock();
 }
 
+static struct request *elv_next_request(struct request_queue *q)
+{
+	struct request *rq;
+	struct blk_flush_queue *fq = blk_get_flush_queue(q, NULL);
+
+	WARN_ON_ONCE(q->mq_ops);
+
+	while (1) {
+		if (!list_empty(&q->queue_head)) {
+			rq = list_entry_rq(q->queue_head.next);
+			return rq;
+		}
+
+		/*
+		 * Flush request is running and flush request isn't queueable
+		 * in the drive, we can hold the queue till flush request is
+		 * finished. Even we don't do this, driver can't dispatch next
+		 * requests and will requeue them. And this can improve
+		 * throughput too. For example, we have request flush1, write1,
+		 * flush 2. flush1 is dispatched, then queue is hold, write1
+		 * isn't inserted to queue. After flush1 is finished, flush2
+		 * will be dispatched. Since disk cache is already clean,
+		 * flush2 will be finished very soon, so looks like flush2 is
+		 * folded to flush1.
+		 * Since the queue is hold, a flag is set to indicate the queue
+		 * should be restarted later. Please see flush_end_io() for
+		 * details.
+		 */
+		if (fq->flush_pending_idx != fq->flush_running_idx &&
+				!queue_flush_queueable(q)) {
+			fq->flush_queue_delayed = 1;
+			return NULL;
+		}
+		if (unlikely(blk_queue_bypass(q)) ||
+		    !q->elevator->type->ops.sq.elevator_dispatch_fn(q, 0))
+			return NULL;
+	}
+}
+
 /**
  * blk_peek_request - peek at the top of a request queue
  * @q: request queue to peek at
@@ -2538,8 +2577,7 @@ struct request *blk_peek_request(struct request_queue *q)
 	lockdep_assert_held(q->queue_lock);
 	WARN_ON_ONCE(q->mq_ops);
 
-	while ((rq = __elv_next_request(q)) != NULL) {
-
+	while ((rq = elv_next_request(q)) != NULL) {
 		rq = blk_pm_peek_request(q, rq);
 		if (!rq)
 			break;
diff --git a/block/blk.h b/block/blk.h
index fcb9775b997d..fda5a4632aba 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -148,45 +148,6 @@ static inline void blk_clear_rq_complete(struct request *rq)
 
 void blk_insert_flush(struct request *rq);
 
-static inline struct request *__elv_next_request(struct request_queue *q)
-{
-	struct request *rq;
-	struct blk_flush_queue *fq = blk_get_flush_queue(q, NULL);
-
-	WARN_ON_ONCE(q->mq_ops);
-
-	while (1) {
-		if (!list_empty(&q->queue_head)) {
-			rq = list_entry_rq(q->queue_head.next);
-			return rq;
-		}
-
-		/*
-		 * Flush request is running and flush request isn't queueable
-		 * in the drive, we can hold the queue till flush request is
-		 * finished. Even we don't do this, driver can't dispatch next
-		 * requests and will requeue them. And this can improve
-		 * throughput too. For example, we have request flush1, write1,
-		 * flush 2. flush1 is dispatched, then queue is hold, write1
-		 * isn't inserted to queue. After flush1 is finished, flush2
-		 * will be dispatched. Since disk cache is already clean,
-		 * flush2 will be finished very soon, so looks like flush2 is
-		 * folded to flush1.
-		 * Since the queue is hold, a flag is set to indicate the queue
-		 * should be restarted later. Please see flush_end_io() for
-		 * details.
-		 */
-		if (fq->flush_pending_idx != fq->flush_running_idx &&
-				!queue_flush_queueable(q)) {
-			fq->flush_queue_delayed = 1;
-			return NULL;
-		}
-		if (unlikely(blk_queue_bypass(q)) ||
-		    !q->elevator->type->ops.sq.elevator_dispatch_fn(q, 0))
-			return NULL;
-	}
-}
-
 static inline void elv_activate_rq(struct request_queue *q, struct request *rq)
 {
 	struct elevator_queue *e = q->elevator;
-- 
2.14.1

[PATCH 2/2] block: fix peeking requests during PM

[Christoph Hellwig]

We need to look for an active PM request until the next softbarrier
instead of looking for the first non-PM request.  Otherwise any cause
of request reordering might starve the PM request(s).

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 14f7674fa0b1..2b704759913a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2464,20 +2464,24 @@ void blk_account_io_done(struct request *req)
  * Don't process normal requests when queue is suspended
  * or in the process of suspending/resuming
  */
-static struct request *blk_pm_peek_request(struct request_queue *q,
-					   struct request *rq)
+static bool blk_pm_allow_request(struct request *rq)
 {
-	if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
-	    (q->rpm_status != RPM_ACTIVE && !(rq->rq_flags & RQF_PM))))
-		return NULL;
-	else
-		return rq;
+	if (!rq->q->dev)
+		return true;
+
+	switch (rq->q->rpm_status) {
+	case RPM_SUSPENDED:
+		return false;
+	case RPM_ACTIVE:
+		return rq->rq_flags & RQF_PM;
+	default:
+		return true;
+	}
 }
 #else
-static inline struct request *blk_pm_peek_request(struct request_queue *q,
-						  struct request *rq)
+static bool blk_pm_allow_request(struct request *rq)
 {
-	return rq;
+	return true;
 }
 #endif
 
@@ -2525,9 +2529,12 @@ static struct request *elv_next_request(struct request_queue *q)
 	WARN_ON_ONCE(q->mq_ops);
 
 	while (1) {
-		if (!list_empty(&q->queue_head)) {
-			rq = list_entry_rq(q->queue_head.next);
-			return rq;
+		list_for_each_entry(rq, &q->queue_head, queuelist) {
+			if (blk_pm_allow_request(rq))
+				return rq;
+
+			if (rq->rq_flags & RQF_SOFTBARRIER)
+				break;
 		}
 
 		/*
@@ -2578,10 +2585,6 @@ struct request *blk_peek_request(struct request_queue *q)
 	WARN_ON_ONCE(q->mq_ops);
 
 	while ((rq = elv_next_request(q)) != NULL) {
-		rq = blk_pm_peek_request(q, rq);
-		if (!rq)
-			break;
-
 		if (!(rq->rq_flags & RQF_STARTED)) {
 			/*
 			 * This is the first time the device driver
-- 
2.14.1

Re: RQF_PM fix/cleanup

[Jens Axboe]

On 10/03/2017 02:46 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series fixed the blocking/passing of requests during PM IFF there were
> more than one.  That currently isn't the case, but relying on that not only
> is fragile, but also leads to more obscure code than handling it properly.

Looks good to me, added for 4.15.

-- 
Jens Axboe

Re: [PATCH 2/2] block: fix peeking requests during PM

[Bart Van Assche]

T24gVHVlLCAyMDE3LTEwLTAzIGF0IDEwOjQ3ICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gLXN0YXRpYyBzdHJ1Y3QgcmVxdWVzdCAqYmxrX3BtX3BlZWtfcmVxdWVzdChzdHJ1Y3Qg
cmVxdWVzdF9xdWV1ZSAqcSwNCj4gLQkJCQkJICAgc3RydWN0IHJlcXVlc3QgKnJxKQ0KPiArc3Rh
dGljIGJvb2wgYmxrX3BtX2FsbG93X3JlcXVlc3Qoc3RydWN0IHJlcXVlc3QgKnJxKQ0KPiAgew0K
PiAtCWlmIChxLT5kZXYgJiYgKHEtPnJwbV9zdGF0dXMgPT0gUlBNX1NVU1BFTkRFRCB8fA0KPiAt
CSAgICAocS0+cnBtX3N0YXR1cyAhPSBSUE1fQUNUSVZFICYmICEocnEtPnJxX2ZsYWdzICYgUlFG
X1BNKSkpKQ0KPiAtCQlyZXR1cm4gTlVMTDsNCj4gLQllbHNlDQo+IC0JCXJldHVybiBycTsNCj4g
KwlpZiAoIXJxLT5xLT5kZXYpDQo+ICsJCXJldHVybiB0cnVlOw0KPiArDQo+ICsJc3dpdGNoIChy
cS0+cS0+cnBtX3N0YXR1cykgew0KPiArCWNhc2UgUlBNX1NVU1BFTkRFRDoNCj4gKwkJcmV0dXJu
IGZhbHNlOw0KPiArCWNhc2UgUlBNX0FDVElWRToNCj4gKwkJcmV0dXJuIHJxLT5ycV9mbGFncyAm
IFJRRl9QTTsNCj4gKwlkZWZhdWx0Og0KPiArCQlyZXR1cm4gdHJ1ZTsNCj4gKwl9DQo+ICB9DQoN
CkhlbGxvIENocmlzdG9waCwNCg0KVGhlIG9sZCBjb2RlIGRvZXMgbm90IHByb2Nlc3Mgbm9uLVBN
IHJlcXVlc3RzIGluIHRoZSBSUE1fU1VTUEVORElORyBtb2RlIG5vcg0KaW4gdGhlIFJQTV9SRVNV
TUlORyBjb2RlLiBJIHRoaW5rIHRoZSBuZXcgY29kZSBwcm9jZXNzZXMgYWxsIHJlcXVlc3RzIGlu
DQp0aGVzZSB0d28gbW9kZXMuIFdhcyB0aGF0IGJlaGF2aW9yIGNoYW5nZSBpbnRlbmRlZD8NCg0K
VGhhbmtzLA0KDQpCYXJ0Lg==

Re: [PATCH 2/2] block: fix peeking requests during PM

[hch]

On Tue, Oct 03, 2017 at 03:13:43PM +0000, Bart Van Assche wrote:
> > +	switch (rq->q->rpm_status) {
> > +	case RPM_SUSPENDED:
> > +		return false;
> > +	case RPM_ACTIVE:
> > +		return rq->rq_flags & RQF_PM;
> > +	default:
> > +		return true;
> > +	}
> >  }
> 
> Hello Christoph,
> 
> The old code does not process non-PM requests in the RPM_SUSPENDING mode nor
> in the RPM_RESUMING code. I think the new code processes all requests in
> these two modes. Was that behavior change intended?

Yeah, looks like this version has the RPM_ACTIVE and default cases
switched.  Back to the drawing board.