On Mon, Feb 3, 2020 at 10:26 AM Steven Rostedt [off-list ref] wrote:

On Sun,  2 Feb 2020 21:36:50 -0800
Cong Wang [off-list ref] wrote:

quoted

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