Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT
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.
Re: [PATCH 1/2] block: get rid of BLK_MAX_TIMEOUT
Bart Van Assche 于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
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.
[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.
Signed-off-by: Weiping Zhang
---
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(>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
#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
