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

[Guoqing Jiang <guoqing.jiang@xxxxxxxxxxxxxxx>]

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