linux-kernel - Re: [Patch v3] block: introduce block_rq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200203132638.5a37f2f2@oasis.local.home>
Date:   Mon, 3 Feb 2020 13:26:38 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [Patch v3] block: introduce block_rq_error tracepoint

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

> Cc: Jens Axboe <axboe@...nel.dk>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.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@...dmis.org>

-- Steve