Re: [Patch v3] block: introduce block_rq_error tracepoint

[Cong Wang]

On Mon, Feb 3, 2020 at 10:26 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Sun,  2 Feb 2020 21:36:50 -0800
> Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
>
> > Cc: Jens Axboe <axboe@xxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Signed-off-by: Cong Wang <xiyou.wangcong@xxxxxxxxx>
> > ---
> >  block/blk-core.c             |  4 +++-
> >  include/trace/events/block.h | 41 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 089e890ab208..0c7ad70d06be 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1450,8 +1450,10 @@ bool blk_update_request(struct request *req, blk_status_t error,
> >  #endif
> >
> >       if (unlikely(error && !blk_rq_is_passthrough(req) &&
> > -                  !(req->rq_flags & RQF_QUIET)))
> > +                  !(req->rq_flags & RQF_QUIET))) {
> > +             trace_block_rq_error(req, blk_status_to_errno(error), nr_bytes);
>
> I'm curious to why you don't just pass error into the trace event.
> Looks like blk_status_to_errno() is a function call and that injects
> code at the location of the call. Note, it is not a big deal as I
> believe (haven't looked at the objdump of it), the call may be placed
> in the nop portion of the code, and not hit when the trace point is not
> enabled. But moving the blk_status_to_errno() call to the
> TP_fast_assign() will move it to another section entirely.
>
> I did see trace_blk_rq_complete() does the same thing, so perhaps that
> could just be a clean up change after this on both trace events.

Yes, it is clearly another copy-n-paste of trace_blk_rq_complete().
I trust the current code base as I believe it already passed
your reviews when it was merged. It looks like not the case.

Anyway, I am happy to address all of these in a followup patch.

Thanks.