Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200203053650.8923-1-xiyou.wangcong@gmail.com> <20200203132638.5a37f2f2@oasis.local.home>
In-Reply-To: <20200203132638.5a37f2f2@oasis.local.home>
From:   Cong Wang <xiyou.wangcong@gmail.com>
Date:   Mon, 3 Feb 2020 12:24:54 -0800
Message-ID: <CAM_iQpXz+O-GHqotZiGC4FgvgcvNHkFg4QFH8_t4j8pHgvAD2Q@mail.gmail.com>
Subject: Re: [Patch v3] block: introduce block_rq_error tracepoint
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-block@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Jens Axboe <axboe@kernel.dk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 3, 2020 at 10:26 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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.

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.