Re: [Patch v3] block: introduce block_rq_error tracepoint

[Steven Rostedt <rostedt@goodmis.org>]

On Sun,  2 Feb 2020 21:36:50 -0800
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  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.



> +
> +	TP_printk("%d,%d %s %s %llu + %u [%d]",
> +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  __get_str(name), __entry->rwbs,
> +		  (unsigned long long)__entry->sector,
> +		  __entry->nr_sector, __entry->error)
> +);
> +

Other than my comment above, for the trace event correctness point of view:

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve