On Thu 11-06-20 16:11:10, Paolo Valente wrote:
>
>
> > Il giorno 5 giu 2020, alle ore 16:16, Jan Kara <jack@xxxxxxx> ha scritto:
> >
> > Currently whenever bfq queue has a request queued we add now -
> > last_completion_time to the think time statistics. This is however
> > misleading in case the process is able to submit several requests in
> > parallel because e.g. if the queue has request completed at time T0 and
> > then queues new requests at times T1, T2, then we will add T1-T0 and
> > T2-T0 to think time statistics which just doesn't make any sence (the
> > queue's think time is penalized by the queue being able to submit more
> > IO).
>
> I've thought about this issue several times. My concern is that, by
> updating the think time only when strictly meaningful, we reduce the
> number of samples. In some cases, we may reduce it to a very low
> value. For this reason, so far I have desisted from changing the
> current scheme. IOW, I opted for dirtier statistics to avoid the risk
> of too scarse statistics. Any feedback is very welcome.
I understand the concern. But:
a) I don't think adding these samples to statistics helps in any way (you
cannot improve the prediction power of the statistics by including in it
some samples that are not directly related to the thing you try to
predict). And think time is used to predict the answer to the question: If
bfq queue becomes idle, how long will it take for new request to arrive? So
second and further requests are simply irrelevant.
b) From my testing with 4 fio sequential readers (the workload mentioned in
the previous patch) this patch caused a noticeable change in computed think
time and that allowed fio processes to be reliably marked as having short
think time (without this patch they were oscilating between short ttime and
not short ttime) and consequently achieving better throughput because we
were idling for new requests from these bfq queues. Note that this was with
somewhat aggressive slice_idle setting (2ms). Having slice_idle >= 4ms was
enough hide the ttime computation issue but still this demonstrates that
these bogus samples noticeably raise computed average.
Honza
> > So add to think time statistics only time intervals when the queue
> > had no IO pending.
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
> > block/bfq-iosched.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> > index c66c3eaa9e26..4b1c9c5f57b6 100644
> > --- a/block/bfq-iosched.c
> > +++ b/block/bfq-iosched.c
> > @@ -5192,8 +5192,16 @@ static void bfq_update_io_thinktime(struct bfq_data *bfqd,
> > struct bfq_queue *bfqq)
> > {
> > struct bfq_ttime *ttime = &bfqq->ttime;
> > - u64 elapsed = ktime_get_ns() - bfqq->ttime.last_end_request;
> > -
> > + u64 elapsed;
> > +
> > + /*
> > + * We are really interested in how long it takes for the queue to
> > + * become busy when there is no outstanding IO for this queue. So
> > + * ignore cases when the bfq queue has already IO queued.
> > + */
> > + if (bfqq->dispatched || bfq_bfqq_busy(bfqq))
> > + return;
> > + elapsed = ktime_get_ns() - bfqq->ttime.last_end_request;
> > elapsed = min_t(u64, elapsed, 2ULL * bfqd->bfq_slice_idle);
> >
> > ttime->ttime_samples = (7*ttime->ttime_samples + 256) / 8;
> > --
> > 2.16.4
> >
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
