Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3 1/5] wbt: don't show valid wbt_lat_usec in sysfs while
 wbt is disabled
To:     Jan Kara <jack@suse.cz>
Cc:     paolo.valente@linaro.org, axboe@kernel.dk,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yukuai1@huaweicloud.com, yi.zhang@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20220922113558.1085314-1-yukuai3@huawei.com>
 <20220922113558.1085314-2-yukuai3@huawei.com>
 <20220926094434.jrl6gnlbjqkex3wa@quack3>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <6736753f-b5ae-39f1-b0c4-508b7f45d701@huaweicloud.com>
Date:   Mon, 26 Sep 2022 18:25:18 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20220926094434.jrl6gnlbjqkex3wa@quack3>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
>>