| Message ID | 20220922113558.1085314-2-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-wbt: simple improvment to enable wbt correctly | expand |
On Thu 22-09-22 19:35:54, Yu Kuai wrote: > Currently, if wbt is initialized and then disabled by > wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which > will confuse users that wbt is still enabled. > > This patch shows wbt_lat_usec as zero and forbid to set it while wbt > is disabled. So I agree we should show 0 in wbt_lat_usec if wbt is disabled by wbt_disable_default(). But why do you forbid setting of wbt_lat_usec? IMHO if wbt_lat_usec is set, admin wants to turn on wbt so we should just update rwb->enable_state to WBT_STATE_ON_MANUAL. Honza > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > Reported-and-tested-by: Holger Hoffstätte <holger@applied-asynchrony.com> > --- > block/blk-sysfs.c | 9 ++++++++- > block/blk-wbt.c | 7 +++++++ > block/blk-wbt.h | 5 +++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c > index e1f009aba6fd..1955bb6a284d 100644 > --- a/block/blk-sysfs.c > +++ b/block/blk-sysfs.c > @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page, > > static ssize_t queue_wb_lat_show(struct request_queue *q, char *page) > { > + u64 lat; > + > if (!wbt_rq_qos(q)) > return -EINVAL; > > - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000)); > + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000); > + > + return sprintf(page, "%llu\n", lat); > } > > static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, > @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, > return ret; > } > > + if (wbt_disabled(q)) > + return -EINVAL; > + > if (val == -1) > val = wbt_default_latency_nsec(q); > else if (val >= 0) > diff --git a/block/blk-wbt.c b/block/blk-wbt.c > index a9982000b667..68851c2c02d2 100644 > --- a/block/blk-wbt.c > +++ b/block/blk-wbt.c > @@ -422,6 +422,13 @@ static void wbt_update_limits(struct rq_wb *rwb) > rwb_wake_all(rwb); > } > > +bool wbt_disabled(struct request_queue *q) > +{ > + struct rq_qos *rqos = wbt_rq_qos(q); > + > + return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT; > +} > + > u64 wbt_get_min_lat(struct request_queue *q) > { > struct rq_qos *rqos = wbt_rq_qos(q); > diff --git a/block/blk-wbt.h b/block/blk-wbt.h > index 7e44eccc676d..e42465ddcbb6 100644 > --- a/block/blk-wbt.h > +++ b/block/blk-wbt.h > @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *); > > u64 wbt_get_min_lat(struct request_queue *q); > void wbt_set_min_lat(struct request_queue *q, u64 val); > +bool wbt_disabled(struct request_queue *); > > void wbt_set_write_cache(struct request_queue *, bool); > > @@ -125,6 +126,10 @@ static inline u64 wbt_default_latency_nsec(struct request_queue *q) > { > return 0; > } > +static inline bool wbt_disabled(struct request_queue *q) > +{ > + return true; > +} > > #endif /* CONFIG_BLK_WBT */ > > -- > 2.31.1 >
Hi, Jan 在 2022/09/26 17:44, Jan Kara 写道: > On Thu 22-09-22 19:35:54, Yu Kuai wrote: >> Currently, if wbt is initialized and then disabled by >> wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which >> will confuse users that wbt is still enabled. >> >> This patch shows wbt_lat_usec as zero and forbid to set it while wbt >> is disabled. > > So I agree we should show 0 in wbt_lat_usec if wbt is disabled by > wbt_disable_default(). But why do you forbid setting of wbt_lat_usec? > IMHO if wbt_lat_usec is set, admin wants to turn on wbt so we should just > update rwb->enable_state to WBT_STATE_ON_MANUAL. I was thinking that don't enable wbt if elevator is bfq. Since we know that performance is bad, thus it doesn't make sense to me to do that, and user might doesn't aware of the problem. However, perhaps admin should be responsible if wbt is turned on while elevator is bfq. Thanks, Kuai > > Honza > >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> Reported-and-tested-by: Holger Hoffstätte <holger@applied-asynchrony.com> >> --- >> block/blk-sysfs.c | 9 ++++++++- >> block/blk-wbt.c | 7 +++++++ >> block/blk-wbt.h | 5 +++++ >> 3 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c >> index e1f009aba6fd..1955bb6a284d 100644 >> --- a/block/blk-sysfs.c >> +++ b/block/blk-sysfs.c >> @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page, >> >> static ssize_t queue_wb_lat_show(struct request_queue *q, char *page) >> { >> + u64 lat; >> + >> if (!wbt_rq_qos(q)) >> return -EINVAL; >> >> - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000)); >> + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000); >> + >> + return sprintf(page, "%llu\n", lat); >> } >> >> static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, >> @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, >> return ret; >> } >> >> + if (wbt_disabled(q)) >> + return -EINVAL; >> + >> if (val == -1) >> val = wbt_default_latency_nsec(q); >> else if (val >= 0) >> diff --git a/block/blk-wbt.c b/block/blk-wbt.c >> index a9982000b667..68851c2c02d2 100644 >> --- a/block/blk-wbt.c >> +++ b/block/blk-wbt.c >> @@ -422,6 +422,13 @@ static void wbt_update_limits(struct rq_wb *rwb) >> rwb_wake_all(rwb); >> } >> >> +bool wbt_disabled(struct request_queue *q) >> +{ >> + struct rq_qos *rqos = wbt_rq_qos(q); >> + >> + return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT; >> +} >> + >> u64 wbt_get_min_lat(struct request_queue *q) >> { >> struct rq_qos *rqos = wbt_rq_qos(q); >> diff --git a/block/blk-wbt.h b/block/blk-wbt.h >> index 7e44eccc676d..e42465ddcbb6 100644 >> --- a/block/blk-wbt.h >> +++ b/block/blk-wbt.h >> @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *); >> >> u64 wbt_get_min_lat(struct request_queue *q); >> void wbt_set_min_lat(struct request_queue *q, u64 val); >> +bool wbt_disabled(struct request_queue *); >> >> void wbt_set_write_cache(struct request_queue *, bool); >> >> @@ -125,6 +126,10 @@ static inline u64 wbt_default_latency_nsec(struct request_queue *q) >> { >> return 0; >> } >> +static inline bool wbt_disabled(struct request_queue *q) >> +{ >> + return true; >> +} >> >> #endif /* CONFIG_BLK_WBT */ >> >> -- >> 2.31.1 >>
On Mon 26-09-22 18:25:18, Yu Kuai wrote: > Hi, Jan > > 在 2022/09/26 17:44, Jan Kara 写道: > > On Thu 22-09-22 19:35:54, Yu Kuai wrote: > > > Currently, if wbt is initialized and then disabled by > > > wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which > > > will confuse users that wbt is still enabled. > > > > > > This patch shows wbt_lat_usec as zero and forbid to set it while wbt > > > is disabled. > > > > So I agree we should show 0 in wbt_lat_usec if wbt is disabled by > > wbt_disable_default(). But why do you forbid setting of wbt_lat_usec? > > IMHO if wbt_lat_usec is set, admin wants to turn on wbt so we should just > > update rwb->enable_state to WBT_STATE_ON_MANUAL. > > I was thinking that don't enable wbt if elevator is bfq. Since we know > that performance is bad, thus it doesn't make sense to me to do that, > and user might doesn't aware of the problem. Yeah, I don't think it is a good idea (that is the reason why it is disabled by default) but in priciple I don't see a reason why we should block admin from enabling it. Honza > > > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > > Reported-and-tested-by: Holger Hoffstätte <holger@applied-asynchrony.com> > > > --- > > > block/blk-sysfs.c | 9 ++++++++- > > > block/blk-wbt.c | 7 +++++++ > > > block/blk-wbt.h | 5 +++++ > > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c > > > index e1f009aba6fd..1955bb6a284d 100644 > > > --- a/block/blk-sysfs.c > > > +++ b/block/blk-sysfs.c > > > @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page, > > > static ssize_t queue_wb_lat_show(struct request_queue *q, char *page) > > > { > > > + u64 lat; > > > + > > > if (!wbt_rq_qos(q)) > > > return -EINVAL; > > > - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000)); > > > + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000); > > > + > > > + return sprintf(page, "%llu\n", lat); > > > } > > > static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, > > > @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, > > > return ret; > > > } > > > + if (wbt_disabled(q)) > > > + return -EINVAL; > > > + > > > if (val == -1) > > > val = wbt_default_latency_nsec(q); > > > else if (val >= 0) > > > diff --git a/block/blk-wbt.c b/block/blk-wbt.c > > > index a9982000b667..68851c2c02d2 100644 > > > --- a/block/blk-wbt.c > > > +++ b/block/blk-wbt.c > > > @@ -422,6 +422,13 @@ static void wbt_update_limits(struct rq_wb *rwb) > > > rwb_wake_all(rwb); > > > } > > > +bool wbt_disabled(struct request_queue *q) > > > +{ > > > + struct rq_qos *rqos = wbt_rq_qos(q); > > > + > > > + return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT; > > > +} > > > + > > > u64 wbt_get_min_lat(struct request_queue *q) > > > { > > > struct rq_qos *rqos = wbt_rq_qos(q); > > > diff --git a/block/blk-wbt.h b/block/blk-wbt.h > > > index 7e44eccc676d..e42465ddcbb6 100644 > > > --- a/block/blk-wbt.h > > > +++ b/block/blk-wbt.h > > > @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *); > > > u64 wbt_get_min_lat(struct request_queue *q); > > > void wbt_set_min_lat(struct request_queue *q, u64 val); > > > +bool wbt_disabled(struct request_queue *); > > > void wbt_set_write_cache(struct request_queue *, bool); > > > @@ -125,6 +126,10 @@ static inline u64 wbt_default_latency_nsec(struct request_queue *q) > > > { > > > return 0; > > > } > > > +static inline bool wbt_disabled(struct request_queue *q) > > > +{ > > > + return true; > > > +} > > > #endif /* CONFIG_BLK_WBT */ > > > -- > > > 2.31.1 > > > >
Hi, Jan 在 2022/09/26 19:47, Jan Kara 写道: > On Mon 26-09-22 18:25:18, Yu Kuai wrote: >> Hi, Jan >> >> 在 2022/09/26 17:44, Jan Kara 写道: >>> On Thu 22-09-22 19:35:54, Yu Kuai wrote: >>>> Currently, if wbt is initialized and then disabled by >>>> wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which >>>> will confuse users that wbt is still enabled. >>>> >>>> This patch shows wbt_lat_usec as zero and forbid to set it while wbt >>>> is disabled. >>> >>> So I agree we should show 0 in wbt_lat_usec if wbt is disabled by >>> wbt_disable_default(). But why do you forbid setting of wbt_lat_usec? >>> IMHO if wbt_lat_usec is set, admin wants to turn on wbt so we should just >>> update rwb->enable_state to WBT_STATE_ON_MANUAL. >> >> I was thinking that don't enable wbt if elevator is bfq. Since we know >> that performance is bad, thus it doesn't make sense to me to do that, >> and user might doesn't aware of the problem. > > Yeah, I don't think it is a good idea (that is the reason why it is > disabled by default) but in priciple I don't see a reason why we should > block admin from enabling it. I'll enable it in next version. Thanks, Kuai
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c index e1f009aba6fd..1955bb6a284d 100644 --- a/block/blk-sysfs.c +++ b/block/blk-sysfs.c @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page, static ssize_t queue_wb_lat_show(struct request_queue *q, char *page) { + u64 lat; + if (!wbt_rq_qos(q)) return -EINVAL; - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000)); + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000); + + return sprintf(page, "%llu\n", lat); } static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page, return ret; } + if (wbt_disabled(q)) + return -EINVAL; + if (val == -1) val = wbt_default_latency_nsec(q); else if (val >= 0) diff --git a/block/blk-wbt.c b/block/blk-wbt.c index a9982000b667..68851c2c02d2 100644 --- a/block/blk-wbt.c +++ b/block/blk-wbt.c @@ -422,6 +422,13 @@ static void wbt_update_limits(struct rq_wb *rwb) rwb_wake_all(rwb); } +bool wbt_disabled(struct request_queue *q) +{ + struct rq_qos *rqos = wbt_rq_qos(q); + + return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT; +} + u64 wbt_get_min_lat(struct request_queue *q) { struct rq_qos *rqos = wbt_rq_qos(q); diff --git a/block/blk-wbt.h b/block/blk-wbt.h index 7e44eccc676d..e42465ddcbb6 100644 --- a/block/blk-wbt.h +++ b/block/blk-wbt.h @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *); u64 wbt_get_min_lat(struct request_queue *q); void wbt_set_min_lat(struct request_queue *q, u64 val); +bool wbt_disabled(struct request_queue *); void wbt_set_write_cache(struct request_queue *, bool); @@ -125,6 +126,10 @@ static inline u64 wbt_default_latency_nsec(struct request_queue *q) { return 0; } +static inline bool wbt_disabled(struct request_queue *q) +{ + return true; +} #endif /* CONFIG_BLK_WBT */
Currently, if wbt is initialized and then disabled by wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which will confuse users that wbt is still enabled. This patch shows wbt_lat_usec as zero and forbid to set it while wbt is disabled. Signed-off-by: Yu Kuai <yukuai3@huawei.com> Reported-and-tested-by: Holger Hoffstätte <holger@applied-asynchrony.com> --- block/blk-sysfs.c | 9 ++++++++- block/blk-wbt.c | 7 +++++++ block/blk-wbt.h | 5 +++++ 3 files changed, 20 insertions(+), 1 deletion(-)