Re: [PATCH RFC V2 1/4] block: add a statistic table for io latency

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Florian-Ewald Mueller <florian-ewald.mueller@cloud.ionos.com>
Subject: Re: [PATCH RFC V2 1/4] block: add a statistic table for io latency
Date: Tue, 14 Jul 2020 10:25:02 +0200	[thread overview]
Message-ID: <62737df2-482a-7efc-1dcb-2b9ac5e89392@cloud.ionos.com> (raw)
In-Reply-To: <SN4PR0401MB3598BB82CD8660B4A8ACBC479B610@SN4PR0401MB3598.namprd04.prod.outlook.com>

On 7/14/20 9:43 AM, Johannes Thumshirn wrote:
> On 13/07/2020 23:14, Guoqing Jiang wrote:
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index d9d632639bd1..036eb04782de 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1411,6 +1411,34 @@ static void update_io_ticks(struct hd_struct *part, unsigned long now, bool end)
>>   	}
>>   }
>>   
>> +#ifdef CONFIG_BLK_ADDITIONAL_DISKSTAT
>> +/*
>> + * Either account additional stat for request if req is not NULL or account for bio.
>> + */
>> +static void blk_additional_latency(struct hd_struct *part, const int sgrp,
>> +				   struct request *req, unsigned long start_jiffies)
>> +{
>> +	unsigned int idx;
>> +	unsigned long duration, now = READ_ONCE(jiffies);
>> +
>> +	if (req)
>> +		duration = jiffies_to_nsecs(now) - req->start_time_ns;
>> +	else
>> +		duration = jiffies_to_nsecs(now - start_jiffies);
>> +
>> +	duration /= NSEC_PER_MSEC;
>> +	duration /= HZ_TO_MSEC_NUM;
>> +	if (likely(duration > 0)) {
>> +		idx = ilog2(duration);
>> +		if (idx > ADD_STAT_NUM - 1)
>> +			idx = ADD_STAT_NUM - 1;
>> +	} else
>> +		idx = 0;
>> +	part_stat_inc(part, latency_table[idx][sgrp]);
>> +
>> +}
>> +#endif
>> +
>>   static void blk_account_io_completion(struct request *req, unsigned int bytes)
>>   {
>>   	if (req->part && blk_do_io_stat(req)) {
>> @@ -1440,6 +1468,9 @@ void blk_account_io_done(struct request *req, u64 now)
>>   		part = req->part;
>>   
>>   		update_io_ticks(part, jiffies, true);
>> +#ifdef CONFIG_BLK_ADDITIONAL_DISKSTAT
>> +		blk_additional_latency(part, sgrp, req, 0);
>> +#endif
> Not commenting on the general idea here but only the code. The above introduces quite a
> lot of ifdefs in code. Please at least move the #ifdef CONFIG_BLK_ADDITIONAL_DISKSTAT
> into the function body of blk_additional_latency() so you don't need any ifdefs at the
> call sites.

Sure, will do it, thanks for your suggestion.

Thanks,
Guoqing