linux-kernel - Re: [v5 PATCH] 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: <CAHbLzkqNdnECNHGn0=kfUOfaxq6LLrwPSVvJKRHNvnuFtPve2w@mail.gmail.com>
Date:   Thu, 27 Jan 2022 10:18:05 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Jens Axboe <axboe@...nel.dk>, Steven Rostedt <rostedt@...dmis.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        linux-block@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [v5 PATCH] block: introduce block_rq_error tracepoint

On Thu, Jan 27, 2022 at 1:53 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> On Wed, Jan 26, 2022 at 10:51:53AM -0800, Yang Shi wrote:
> > +             __entry->dev       = rq->q->disk ? disk_devt(rq->q->disk) : 0;
> > +             __assign_str(name,   rq->q->disk ? rq->q->disk->disk_name : "?");
>
> None f the other tracepoints has the disk name, why does this one need
> it?  And if you add it please avoid the overly long line.

I guess the disk name was added to ease some handling in userspace
tools. But if all other tracepoints don't have disk name shown, I
think I'd better follow the convention. I did overlook this when I
ported this patch. Will remove it.

>
> > +             __entry->sector    = blk_rq_pos(rq);
> > +             __entry->nr_sector = nr_bytes >> 9;
> > +             __entry->error     = blk_status_to_errno(error);
>
> This still converts the block status to an errno.

I may misunderstand your comments. I just followed what
block_rq_complete tracepoint does. Or the linux-block community is
converting all tracepoints to show blk status code instead of
conventional errno?

And the userspace tool doesn't know blk status code and still expects
the conventional errno. For example, rasdaemon reads block_rq_complete
events now and have the below:

static const struct {
        int             error;
        const char      *name;
} blk_errors[] = {
        { -EOPNOTSUPP, "operation not supported error" },
        { -ETIMEDOUT, "timeout error" },
        { -ENOSPC,    "critical space allocation error" },
        { -ENOLINK,   "recoverable transport error" },
        { -EREMOTEIO, "critical target error" },
        { -EBADE,     "critical nexus error" },
        { -ENODATA,   "critical medium error" },
        { -EILSEQ,    "protection error" },
        { -ENOMEM,    "kernel resource error" },
        { -EBUSY,     "device resource error" },
        { -EAGAIN,    "nonblocking retry error" },
        { -EREMCHG, "dm internal retry error" },
        { -EIO,       "I/O error" },
};

This patch aims to add block_rq_err tracepoint to replace
block_rq_complete in rasdaemon.