get rid of BLK_MAX_TIMEOUT
cover get rid of BLK_MAX_TIMEOUT
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(-)
Signature · 1 line
2.14.1
PATCH 1/2 block: get rid of BLK_MAX_TIMEOUT
Since io_timeout was added to sysfs, the user can tune timeouts by that attribute, so kill this limitation.
--- block/blk-timeout.c | 13 +------------ block/blk.h | 4 ---- 2 files changed, 1 insertion(+), 16 deletions(-)
block/blk-timeout.c +1 -12
@@ -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)) {
block/blk.h +0 -4
@@ -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,
Signature · 1 line
2.14.1
Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT
Review
Quoted 5 lines · Weiping Zhang
@@-130,7 +-119,7 @@void
blk_add_timer(struct request *req)*than an existing one, modify the timer. Round up to next nearest *second. */(quoted diff) +0
-1- 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
Quoted 13 lines · Bart Van Assche <[email protected]> 于2018年12月5日周三 下午11:59写道:
block/blk-timeout.c void blk_add_timer(struct request *req) · line 122
Quoted 6 lines · Weiping Zhang
block/blk-timeout.c +1
-1@@ -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 callbackblk_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
Quoted 1 line
Bart.
Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT
Quoted 10 lines · Weiping Zhang
Before this patch, even we set
io_timeoutto 30*HZ(default), butblk_rq_timeoutalways return jiffies +-5*HZ,
[1] if there no pending request in timeout list, the timer callbackblk_rq_timed_out_timerwill be called after 5*HZ, and thenblk_mq_check_expiredwill check is there exist some request was delayed by compare jiffies andrequest->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.
Quoted 6 lines
[2] if there are pending request in timeout list, we compare request's
expiry and queue's expiry. Iftime_after(request->expire, queue->expire)modifyqueue->timeout.expiretorequest->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.
PATCH 2/2 block: add BLK_DEF_TIMEOUT
In reply to Weiping ZhangAdd an obvious definition for default request timeout.
--- block/blk-mq.c | 2 +- block/blk.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
block/blk-mq.c +1 -1
@@ -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;
block/blk.h +3 -0
@@ -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
Signature · 1 line
2.14.1