* [PATCH 0/2] get rid of BLK_MAX_TIMEOUT @ 2018-12-05 15:36 Weiping Zhang 2018-12-05 15:37 ` [PATCH 1/2] block: " Weiping Zhang 2018-12-05 15:38 ` [PATCH 2/2] block: add BLK_DEF_TIMEOUT Weiping Zhang 0 siblings, 2 replies; 6+ messages in thread From: Weiping Zhang @ 2018-12-05 15:36 UTC (permalink / raw) To: axboe; +Cc: linux-block 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT 2018-12-05 15:36 [PATCH 0/2] get rid of BLK_MAX_TIMEOUT Weiping Zhang @ 2018-12-05 15:37 ` Weiping Zhang 2018-12-05 15:58 ` Bart Van Assche 2018-12-05 15:38 ` [PATCH 2/2] block: add BLK_DEF_TIMEOUT Weiping Zhang 1 sibling, 1 reply; 6+ messages in thread From: Weiping Zhang @ 2018-12-05 15:37 UTC (permalink / raw) To: axboe; +Cc: linux-block 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT 2018-12-05 15:37 ` [PATCH 1/2] block: " Weiping Zhang @ 2018-12-05 15:58 ` Bart Van Assche 2018-12-06 14:18 ` Weiping Zhang 0 siblings, 1 reply; 6+ messages in thread From: Bart Van Assche @ 2018-12-05 15:58 UTC (permalink / raw) To: Weiping Zhang, axboe; +Cc: linux-block 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT 2018-12-05 15:58 ` Bart Van Assche @ 2018-12-06 14:18 ` Weiping Zhang 2018-12-06 16:14 ` Bart Van Assche 0 siblings, 1 reply; 6+ messages in thread From: Weiping Zhang @ 2018-12-06 14:18 UTC (permalink / raw) To: bvanassche; +Cc: zhangweiping, Jens Axboe, linux-block 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT 2018-12-06 14:18 ` Weiping Zhang @ 2018-12-06 16:14 ` Bart Van Assche 0 siblings, 0 replies; 6+ messages in thread From: Bart Van Assche @ 2018-12-06 16:14 UTC (permalink / raw) To: Weiping Zhang; +Cc: zhangweiping, Jens Axboe, linux-block 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] block: add BLK_DEF_TIMEOUT 2018-12-05 15:36 [PATCH 0/2] get rid of BLK_MAX_TIMEOUT Weiping Zhang 2018-12-05 15:37 ` [PATCH 1/2] block: " Weiping Zhang @ 2018-12-05 15:38 ` Weiping Zhang 1 sibling, 0 replies; 6+ messages in thread From: Weiping Zhang @ 2018-12-05 15:38 UTC (permalink / raw) To: axboe; +Cc: linux-block 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-06 16:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-05 15:36 [PATCH 0/2] get rid of BLK_MAX_TIMEOUT Weiping Zhang 2018-12-05 15:37 ` [PATCH 1/2] block: " Weiping Zhang 2018-12-05 15:58 ` Bart Van Assche 2018-12-06 14:18 ` Weiping Zhang 2018-12-06 16:14 ` Bart Van Assche 2018-12-05 15:38 ` [PATCH 2/2] block: add BLK_DEF_TIMEOUT Weiping Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).