| Message ID | 997df532a176a435c3581ce180f318a298084393.1544023832.git.zhangweiping@didiglobal.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | get rid of BLK_MAX_TIMEOUT | expand |
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.
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.
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.
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,
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(-)