> rq->timeout = 0; > > - if (blk_mq_need_time_stamp(rq)) > - rq->start_time_ns = ktime_get_ns(); > - else > - rq->start_time_ns = 0; > + rq->start_time_ns = now; > rq->part = NULL; > #ifdef CONFIG_BLK_RQ_ALLOC_TIME > - rq->alloc_time_ns = alloc_time_ns; > + rq->alloc_time_ns = now; > #endif Given that rq->start_time_ns and rq->alloc_time_ns are not always the same (except for the odd flush case), do we even need both fields? > static inline struct request * > __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data, > - u64 alloc_time_ns) > + u64 now) Nit: this fits into the previous line now. > return blk_mq_rq_ctx_init(data, blk_mq_tags_from_data(data), tag, > - alloc_time_ns); > + now); Same here. > static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, > - struct request *rq, bool last) > + struct request *rq, bool last, u64 now) Overly long line. FYI, this becomes a lot more readable anyway with the two tab indent: static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, struct request *rq, bool last, u64 now) > @@ -2521,6 +2528,7 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, > * Any other error (busy), just add it to our list as we > * previously would have done. > */ > + rq->io_start_time_ns = now; Looking though this more I hate all these extra assignments, and it will change the accounting a bit from what we had. Why can't we pass the time to blk_mq_start_request and just keep it where it was? > + u64 now = 0; > + > + if (blk_queue_stat(rq->q)) > + now = ktime_get_ns(); This code pattern is duplicated a lot. Can't we have a nice helper and shortcut it to: u64 now = blk_ktime_get_ns_for_stats(rq->q); and use the opportunity to document the logic in the helpe. > -static blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last) > +static blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last, u64 now) Overly long line here. But I really wonder why we even needthis helper.
