[PATCH 0/4] cfq-iosched: Fixes for conversion from jiffies to ns

[PATCH 0/4] cfq-iosched: Fixes for conversion from jiffies to ns

[Jan Kara]

Hi,

Commit 9a7f38c42c2b (cfq-iosched: Convert from jiffies to nanoseconds)
introduced couple of issues into cfq io scheduler. Most notably our QA
runs within SUSE uncovered a regression in bonnie++ rewrite performance
due to a broken condition detecting starved sync IO and further review
unveiled other three mostly cosmetic issues (but still worth fixing).

Jens, please queue these patches to avoid regressions when the above
mentioned commit gets merged in the next merge window. If you want to
fold them into the above commit, I'm fine with that as well. Thanks!

							Honza

[PATCH 1/4] block: Convert fifo_time from ulong to u64

[Jan Kara]

Currently rq->fifo_time is unsigned long but CFQ stores nanosecond
timestamp in it which would overflow on 32-bit archs. Convert it to u64
to avoid the overflow. Since the rq->fifo_time is unioned with struct
call_single_data(), this does not change the size of struct request in
any way.

We have to slightly fixup block/deadline-iosched.c so that comparison
happens in the right types.

Fixes: 9a7f38c42c2b92391d9dabaf9f51df7cfe5608e4
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/deadline-iosched.c | 5 +++--
 include/linux/blkdev.h   | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/deadline-iosched.c b/block/deadline-iosched.c
index d0dd7882d8c7..26a9d3c8057a 100644
--- a/block/deadline-iosched.c
+++ b/block/deadline-iosched.c
@@ -173,7 +173,8 @@ deadline_merged_requests(struct request_queue *q, struct request *req,
 	 * and move into next position (next will be deleted) in fifo
 	 */
 	if (!list_empty(&req->queuelist) && !list_empty(&next->queuelist)) {
-		if (time_before(next->fifo_time, req->fifo_time)) {
+		if (time_before((unsigned long)next->fifo_time,
+				(unsigned long)req->fifo_time)) {
 			list_move(&req->queuelist, &next->queuelist);
 			req->fifo_time = next->fifo_time;
 		}
@@ -227,7 +228,7 @@ static inline int deadline_check_fifo(struct deadline_data *dd, int ddir)
 	/*
 	 * rq is expired!
 	 */
-	if (time_after_eq(jiffies, rq->fifo_time))
+	if (time_after_eq(jiffies, (unsigned long)rq->fifo_time))
 		return 1;
 
 	return 0;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d9cf326574f..1bef2c3b59be 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -90,7 +90,7 @@ struct request {
 	struct list_head queuelist;
 	union {
 		struct call_single_data csd;
-		unsigned long fifo_time;
+		u64 fifo_time;
 	};
 
 	struct request_queue *q;
-- 
2.6.6

[PATCH 2/4] cfq-iosched: Convert slice_resid from u64 to s64

[Jan Kara]

slice_resid can be both positive and negative. Commit 9a7f38c42c2b
(cfq-iosched: Convert from jiffies to nanoseconds) converted it from
long to u64. Although this did not introduce any functional regression
(the operations just overflow and the result was fine), it is certainly
wrong and could cause issues in future. So convert the type to more
appropriate s64.

Fixes: 9a7f38c42c2b92391d9dabaf9f51df7cfe5608e4
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/cfq-iosched.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index dab606852e24..98cece4e62a9 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -132,7 +132,7 @@ struct cfq_queue {
 	/* time when first request from queue completed and slice started. */
 	u64 slice_start;
 	u64 slice_end;
-	u64 slice_resid;
+	s64 slice_resid;
 
 	/* pending priority requests */
 	int prio_pending;
@@ -2681,7 +2681,7 @@ __cfq_slice_expired(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 			cfqq->slice_resid = cfq_scaled_cfqq_slice(cfqd, cfqq);
 		else
 			cfqq->slice_resid = cfqq->slice_end - ktime_get_ns();
-		cfq_log_cfqq(cfqd, cfqq, "resid=%llu", cfqq->slice_resid);
+		cfq_log_cfqq(cfqd, cfqq, "resid=%lld", cfqq->slice_resid);
 	}
 
 	cfq_group_served(cfqd, cfqq->cfqg, cfqq);
-- 
2.6.6

[PATCH 3/4] cfq-iosched: Fix regression in bonnie++ rewrite performance

[Jan Kara]

Commit 9a7f38c42c2 (cfq-iosched: Convert from jiffies to nanoseconds)
broke the condition for detecting starved sync IO in
cfq_completed_request() because rq->start_time remained in jiffies but
we compared it with nanosecond values. This manifested as a regression
in bonnie++ rewrite performance because we always ended up considering
sync IO starved and thus never increased async IO queue depth.

Since rq->start_time is used in a lot of places, converting it to ns
values would be non-trivial. So just revert the condition in CFQ to use
comparison with jiffies. This will lead to suboptimal results if
cfq_fifo_expire[1] will ever come close to 1 jiffie but so far we are
relatively far from that with the storage used with CFQ (the default
value is 128 ms).

Fixes: 9a7f38c42c2b92391d9dabaf9f51df7cfe5608e4
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/cfq-iosched.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 98cece4e62a9..e074dd4f1384 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -4233,7 +4233,16 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 					cfqq_type(cfqq));
 
 		st->ttime.last_end_request = now;
-		if (!(rq->start_time + cfqd->cfq_fifo_expire[1] > now))
+		/*
+		 * We have to do this check in jiffies since start_time is in
+		 * jiffies and it is not trivial to convert to ns. If
+		 * cfq_fifo_expire[1] ever comes close to 1 jiffie, this test
+		 * will become problematic but so far we are fine (the default
+		 * is 128 ms).
+		 */
+		if (!time_after(rq->start_time +
+				  nsecs_to_jiffies(cfqd->cfq_fifo_expire[1]),
+				jiffies))
 			cfqd->last_delayed_sync = now;
 	}
 
-- 
2.6.6

[PATCH 4/4] cfq-iosched: Charge at least 1 jiffie instead of 1 ns

[Jan Kara]

Commit 9a7f38c42c2b (cfq-iosched: Convert from jiffies to nanoseconds)
could result in charging just 1 ns to a cgroup submitting IO instead of 1
jiffie we always charged before. It is arguable what is the right amount
to change but for now lets retain the old behavior of always charging at
least one jiffie.

Fixes: 9a7f38c42c2b92391d9dabaf9f51df7cfe5608e4
Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/cfq-iosched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index e074dd4f1384..81accf9c4eaf 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -1455,7 +1455,8 @@ static inline u64 cfq_cfqq_slice_usage(struct cfq_queue *cfqq,
 		 * a single request on seeky media and cause lots of seek time
 		 * and group will never know it.
 		 */
-		slice_used = max_t(u64, (now - cfqq->dispatch_start), 1);
+		slice_used = max_t(u64, (now - cfqq->dispatch_start),
+					jiffies_to_nsecs(1));
 	} else {
 		slice_used = now - cfqq->slice_start;
 		if (slice_used > cfqq->allocated_slice) {
-- 
2.6.6

Re: [PATCH 0/4] cfq-iosched: Fixes for conversion from jiffies to ns

[Jens Axboe]

On 06/28/2016 01:03 AM, Jan Kara wrote:
> Hi,
>
> Commit 9a7f38c42c2b (cfq-iosched: Convert from jiffies to nanoseconds)
> introduced couple of issues into cfq io scheduler. Most notably our QA
> runs within SUSE uncovered a regression in bonnie++ rewrite performance
> due to a broken condition detecting starved sync IO and further review
> unveiled other three mostly cosmetic issues (but still worth fixing).
>
> Jens, please queue these patches to avoid regressions when the above
> mentioned commit gets merged in the next merge window. If you want to
> fold them into the above commit, I'm fine with that as well. Thanks!

Thanks Jan, added for 4.8!

-- 
Jens Axboe