linux-kernel - Re: [Patch v2] 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] [day] [month] [year] [list]

Message-ID: <CAM_iQpU_Z6CFZh2hBQLneqy6=EFSZCUGET+vh+d78YDrOgTj+w@mail.gmail.com>
Date:   Sun, 26 Jan 2020 12:26:00 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, linux-block@...r.kernel.org,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [Patch v2] block: introduce block_rq_error tracepoint

On Tue, Jan 21, 2020 at 4:01 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Mon, 20 Jan 2020 14:26:18 -0800
> Cong Wang <xiyou.wangcong@...il.com> wrote:
> > +/**
> > + * block_rq_error - block IO operation error reported by device driver
> > + * @rq: block operations request
> > + * @error: status code
> > + * @nr_bytes: number of completed bytes
> > + *
> > + * The block_rq_error tracepoint event indicates that some portion
> > + * of operation request has failed as reported by the device driver.
> > + */
> > +TRACE_EVENT(block_rq_error,
> > +
> > +     TP_PROTO(struct request *rq, int error, unsigned int nr_bytes),
> > +
> > +     TP_ARGS(rq, error, nr_bytes),
> > +
> > +     TP_STRUCT__entry(
> > +             __field(  dev_t,        dev                     )
> > +             __dynamic_array( char,  name,   DISK_NAME_LEN   )
>
> Hmm, looks like I need to go and do a clean up of the kernel, and
> educate people on how to use dynamic arrays :-/

Yeah.

>
> The "len" field of a __dynamic_array() is to be a function to determine
> the length needed for each instance of an event. By having a constant
> there, it will be the same for every events, plus the meta data to hold
> the "dynamic" part of the array. This would be much better to simple
> use __array() instead.
>
> But as you use "__assign_str()" below, then it's expected that name is
> a nul terminated string. In which case, you want to define this as:
>
>                 __string( name, rq->rq_disk ? rq->rq_disk->disk_name : "?"  )

Ah, I wanted to use string() but all the existing users initialize/assign
them twice: once in TP_STRUCT__entry() and once in TP_fast_assign().


>
>
> > +             __field(  sector_t,     sector                  )
> > +             __field(  unsigned int, nr_sector               )
> > +             __field(  int,          error                   )
> > +             __array(  char,         rwbs,   RWBS_LEN        )
> > +             __dynamic_array( char,  cmd,    1               )
>
> Not sure what you are doing with cmd. It appears to be always hard
> coded as an empty string?

It is supposed to be a string of one-char commands. This is a
copy-n-paste from existing block_rq_requeue(). I don't know why
its length is 1, but yeah it looks wrong to me too.

Thanks!