[PATCH 0/2] get rid of BLK_MAX_TIMEOUT

[PATCH 0/2] get rid of BLK_MAX_TIMEOUT

[Weiping Zhang]

Get rid of BLK_MAX_TIMEOUT, since we want use io_timeout to control
the maximun timeouts of a request, so remove this limitation.

Weiping Zhang (2):
  block: get rid of BLK_MAX_TIMEOUT
  block: add BLK_DEF_TIMEOUT

 block/blk-mq.c      |  2 +-
 block/blk-timeout.c | 13 +------------
 block/blk.h         |  5 ++---
 3 files changed, 4 insertions(+), 16 deletions(-)

-- 
2.14.1

[PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT

[Weiping Zhang]

Since io_timeout was added to sysfs, the user can tune timeouts
by that attribute, so kill this limitation.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 block/blk-timeout.c | 13 +------------
 block/blk.h         |  4 ----
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 124c26128bf6..6b2b0f7e5929 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -89,17 +89,6 @@ void blk_abort_request(struct request *req)
 }
 EXPORT_SYMBOL_GPL(blk_abort_request);
 
-unsigned long blk_rq_timeout(unsigned long timeout)
-{
-	unsigned long maxt;
-
-	maxt = round_jiffies_up(jiffies + BLK_MAX_TIMEOUT);
-	if (time_after(timeout, maxt))
-		timeout = maxt;
-
-	return timeout;
-}
-
 /**
  * blk_add_timer - Start timeout timer for a single request
  * @req:	request that is about to start running.
@@ -130,7 +119,7 @@ void blk_add_timer(struct request *req)
 	 * than an existing one, modify the timer. Round up to next nearest
 	 * second.
 	 */
-	expiry = blk_rq_timeout(round_jiffies_up(expiry));
+	expiry = round_jiffies_up(expiry);
 
 	if (!timer_pending(&q->timeout) ||
 	    time_before(expiry, q->timeout.expires)) {
diff --git a/block/blk.h b/block/blk.h
index 848278c52030..f0d0a18fb276 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -7,9 +7,6 @@
 #include <xen/xen.h>
 #include "blk-mq.h"
 
-/* Max future timer expiry for timeouts */
-#define BLK_MAX_TIMEOUT		(5 * HZ)
-
 #ifdef CONFIG_DEBUG_FS
 extern struct dentry *blk_debugfs_root;
 #endif
@@ -151,7 +148,6 @@ static inline bool bio_integrity_endio(struct bio *bio)
 }
 #endif /* CONFIG_BLK_DEV_INTEGRITY */
 
-unsigned long blk_rq_timeout(unsigned long timeout);
 void blk_add_timer(struct request *req);
 
 bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
-- 
2.14.1

[PATCH 2/2] block: add BLK_DEF_TIMEOUT

[Weiping Zhang]

Add an obvious definition for default request timeout.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
 block/blk-mq.c | 2 +-
 block/blk.h    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 900550594651..53337d5c37af 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2845,7 +2845,7 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 		goto err_hctxs;
 
 	INIT_WORK(&q->timeout_work, blk_mq_timeout_work);
-	blk_queue_rq_timeout(q, set->timeout ? set->timeout : 30 * HZ);
+	blk_queue_rq_timeout(q, set->timeout ? set->timeout : BLK_DEF_TIMEOUT);
 
 	q->tag_set = set;
 
diff --git a/block/blk.h b/block/blk.h
index f0d0a18fb276..7905b952b7b3 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -7,6 +7,9 @@
 #include <xen/xen.h>
 #include "blk-mq.h"
 
+/* the default request timeout */
+#define BLK_DEF_TIMEOUT (30 * HZ)
+
 #ifdef CONFIG_DEBUG_FS
 extern struct dentry *blk_debugfs_root;
 #endif
-- 
2.14.1

Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT

[Bart Van Assche]

On Wed, 2018-12-05 at 23:37 +0800, Weiping Zhang wrote:
> @@ -130,7 +119,7 @@ void blk_add_timer(struct request *req)
>  	 * than an existing one, modify the timer. Round up to next nearest
>  	 * second.
>  	 */
> -	expiry = blk_rq_timeout(round_jiffies_up(expiry));
> +	expiry = round_jiffies_up(expiry);

If you would have read the comment above this code, you would have known
that this patch does not do what you think it does and additionally that
it introduces a regression.

Bart.

Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT

[Weiping Zhang]

Bart Van Assche <bvanassche@acm.org> 于2018年12月5日周三 下午11:59写道：
>
> On Wed, 2018-12-05 at 23:37 +0800, Weiping Zhang wrote:
> > @@ -130,7 +119,7 @@ void blk_add_timer(struct request *req)
> >        * than an existing one, modify the timer. Round up to next nearest
> >        * second.
> >        */
> > -     expiry = blk_rq_timeout(round_jiffies_up(expiry));
> > +     expiry = round_jiffies_up(expiry);
>
> If you would have read the comment above this code, you would have known
> that this patch does not do what you think it does and additionally that
> it introduces a regression.
>
Let's paste full comments here:
        /*
         * If the timer isn't already pending or this timeout is earlier
         * than an existing one, modify the timer. Round up to next nearest
         * second.
         */
Before this patch, even we set io_timeout to 30*HZ(default), but
blk_rq_timeout always return jiffies +5*HZ,
  [1]. if there no pending request in timeout list, the timer callback
blk_rq_timed_out_timer will be called after 5*HZ, and then
blk_mq_check_expired will check is there exist some request
was delayed by compare jiffies and request->deadline, obvious
request is not timeout because we set request's timeouts is 30*HZ.
So for this case timer callback should be called at jiffies + 30 instead
of jiffies + 5*HZ.

  [2]. if there are pending request in timeout list, we compare request's
expiry and queue's expiry. If time_after(request->expire, queue->expire) modify
queue->timeout.expire to request->expire, otherwise do nothing.

So I think this patch just solve problem in [1], no other regression, or what's
I missing here ?

Thanks
Weiping
> Bart.

Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT

[Bart Van Assche]

On Thu, 2018-12-06 at 22:18 +0800, Weiping Zhang wrote:
> Before this patch, even we set io_timeout to 30*HZ(default), but
> blk_rq_timeout always return jiffies +5*HZ,
>   [1]. if there no pending request in timeout list, the timer callback
> blk_rq_timed_out_timer will be called after 5*HZ, and then
> blk_mq_check_expired will check is there exist some request
> was delayed by compare jiffies and request->deadline, obvious
> request is not timeout because we set request's timeouts is 30*HZ.
> So for this case timer callback should be called at jiffies + 30 instead
> of jiffies + 5*HZ.
> 
>   [2]. if there are pending request in timeout list, we compare request's
> expiry and queue's expiry. If time_after(request->expire, queue->expire) modify
> queue->timeout.expire to request->expire, otherwise do nothing.
> 
> So I think this patch just solve problem in [1], no other regression, or what's
> I missing here ?

The blk_rq_timeout() function was introduced by commit 0d2602ca30e4 ("blk-mq:
improve support for shared tags maps"). I think the purpose of that function
is to make sure that the nr_active counter in struct blk_mq_hw_ctx gets updated
at least once every five seconds. So there are two problems with this patch:
- It reduces the frequency of 'nr_active' updates. I think that is wrong and
  also that it will negatively affect drivers that rely on this functionality,
  e.g. the SCSI core.
- The patch description does not match the code changes in this patch.

Bart.