linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Tony Asleson <tasleson@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	linux-ide@vger.kernel.org, Hannes Reinecke <hare@suse.de>
Subject: Re: [v5 01/12] struct device: Add function callback durable_name
Date: Thu, 1 Oct 2020 13:48:32 +0200	[thread overview]
Message-ID: <20201001114832.GC2368232@kroah.com> (raw)
In-Reply-To: <c6b031b8-f617-0580-52a5-26532da4ee03@redhat.com>

On Wed, Sep 30, 2020 at 09:35:52AM -0500, Tony Asleson wrote:
> On 9/30/20 2:38 AM, Greg Kroah-Hartman wrote:
> > On Tue, Sep 29, 2020 at 05:04:32PM -0500, Tony Asleson wrote:
> >> I'm trying to figure out a way to positively identify which storage
> >> device an error belongs to over time.
> > 
> > "over time" is not the kernel's responsibility.
> > 
> > This comes up every 5 years or so. The kernel provides you, at runtime,
> > a mapping between a hardware device and a "logical" device.  It can
> > provide information to userspace about this mapping, but once that
> > device goes away, the kernel is free to reuse that logical device again.
> > 
> > If you want to track what logical devices match up to what physical
> > device, then do it in userspace, by parsing the log files.
> 
> I don't understand why people think it's acceptable to ask user space to
> parse text that is subject to change.

What text is changing?  The format of of the prefix of dev_*() is well
known and has been stable for 15+ years now, right?  What is difficult
in parsing it?

> >> Thank you for supplying some feedback and asking questions.  I've been
> >> asking for suggestions and would very much like to have a discussion on
> >> how this issue is best solved.  I'm not attached to what I've provided.
> >> I'm just trying to get towards a solution.
> > 
> > Again, solve this in userspace, you have the information there at
> > runtime, why not use it?
> 
> We usually don't have the needed information if you remove the
> expectation that user space should parse the human readable portion of
> the error message.

I don't expect that userspace should have to parse any human readable
portion, if they don't want to.  But if you do want it to, it is pretty
trivial to parse what you have today:

	scsi 2:0:0:0: Direct-Access     Generic  STORAGE DEVICE   1531 PQ: 0 ANSI: 6

If you really have a unique identifier, then great, parse it today:

	usb 4-1.3.1: Product: USB3.0 Card Reader
	usb 4-1.3.1: Manufacturer: Generic
	usb 4-1.3.1: SerialNumber: 000000001531

What's keeping that from working now?

In fact, I would argue that it does seem to work, as there are many
commercial tools out there that seem to handle it just fine...

> >> We've looked at user space quite a bit and there is an inherit race
> >> condition with trying to fetch the unique hardware id for a message when
> >> it gets emitted from the kernel as udev rules haven't even run (assuming
> >> we even have the meta-data to make the association).
> > 
> > But one moment later you do have the information, so you can properly
> > correlate it, right?
> 
> We could have the information if all the storage paths went through
> dev_printk.  Here is what we get today when we encounter a read error
> which uses printk in the block layer:
> 
> {
>         "_HOSTNAME" : "pn",
>         "_TRANSPORT" : "kernel",
>         "__MONOTONIC_TIMESTAMP" : "1806379233",
>         "SYSLOG_IDENTIFIER" : "kernel",
>         "_SOURCE_MONOTONIC_TIMESTAMP" : "1805611354",
>         "SYSLOG_FACILITY" : "0",
>         "MESSAGE" : "blk_update_request: critical medium error, dev
> nvme0n1, sector 10000 op 0x0:(READ) flags 0x80700 phys_seg 3 prio class 0",
>         "PRIORITY" : "3",
>         "_MACHINE_ID" : "3f31a0847cea4c95b7a9cec13d07deeb",
>         "__REALTIME_TIMESTAMP" : "1601471260802301",
>         "_BOOT_ID" : "b03ed610f21d46ab8243a495ba5a0058",
>         "__CURSOR" :
> "s=a063a22bbb384da0b0412e8f652deabb;i=23c2;b=b03ed610f21d46ab8243a495ba5a0058;m=6bab28e1;t=5b087959e3cfd;x=20528862f8f765c9"
> }

Ok, messy stuff, don't do that :)

> Unless you parse the message text you cannot make the association.  If
> the same message was changed to dev_printk we would get:
> 
> 
> {
>         "__REALTIME_TIMESTAMP" : "1589401901093443",
>         "__CURSOR" :
> "s=caac9703b34a48fd92f7875adae55a2f;i=1c713;b=e2ae14a9def345aa803a13648b95429c;m=7d25b4f;t=5a58d77b85243;x=b034c2d3fb853870",
>         "SYSLOG_IDENTIFIER" : "kernel",
>         "_KERNEL_DEVICE" : "b259:917504",
>         "__MONOTONIC_TIMESTAMP" : "131226447",
>         "_UDEV_SYSNAME" : "nvme0n1",
>         "PRIORITY" : "3",
>         "_KERNEL_SUBSYSTEM" : "block",
>         "_SOURCE_MONOTONIC_TIMESTAMP" : "130941917",
>         "_TRANSPORT" : "kernel",
>         "_MACHINE_ID" : "3f31a0847cea4c95b7a9cec13d07deeb",
>         "_HOSTNAME" : "pn",
>         "SYSLOG_FACILITY" : "0",
>         "_BOOT_ID" : "e2ae14a9def345aa803a13648b95429c",
>         "_UDEV_DEVLINK" : [
>                 "/dev/disk/by-uuid/22fc262a-d621-452a-a951-7761d9fcf0dc",
>                 "/dev/disk/by-path/pci-0000:00:05.0-nvme-1",
> 
> "/dev/disk/by-id/nvme-nvme.8086-4445414442454546-51454d55204e564d65204374726c-00000001",
>                 "/dev/disk/by-id/nvme-QEMU_NVMe_Ctrl_DEADBEEF"
>         ],
>         "MESSAGE" : "block nvme0n1: blk_update_request: critical medium
> error, dev nvme0n1, sector 10000 op 0x0:(READ) flags 0x0 phys_seg 1 prio
> class 0",
>         "_UDEV_DEVNODE" : "/dev/nvme0n1"
> }

Great, you have a udev sysname, a kernel subsystem and a way to
associate that with a real device, what more are you wanting?

> Journald already knows how to utilize the dev_printk meta data.

And if you talk to the printk developers (which you seem to be keeping
out of the loop here), they are ripping out the meta data facility as
fast as possible.  So don't rely on extending that please.

> One idea that I've suggested along the way is creating a dev_printk
> function that doesn't change the message text.  We then avoid breaking
> people that are parsing.  Is this something that would be acceptable to
> folks?  It doesn't solve early boot where udev rules haven't even run,
> but it's better.

I still fail to understand the root problem here.

Ok, no, I think I understand what you think the problem is, I just don't
see why it is up to the kernel to change what we have today when there
are lots of tools out there working just fine without any kernel changes
needed.

So try explaining the problem as you see it please, so we all know where
to work from.

But again, cutting out the developers of the subsystems you were wanting
to modify might just be making me really grumpy about this whole
thing...

greg k-h

  reply	other threads:[~2020-10-01 11:48 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-25 16:19 [v5 00/12] Add persistent durable identifier to storage log messages Tony Asleson
2020-09-25 16:19 ` [v5 01/12] struct device: Add function callback durable_name Tony Asleson
2020-09-26  9:08   ` Sergei Shtylyov
2020-09-27 14:22     ` Tony Asleson
2020-09-27 16:15       ` Sergei Shtylyov
2020-09-29 17:51   ` Christoph Hellwig
2020-09-29 18:04     ` Greg Kroah-Hartman
2020-09-29 22:04       ` Tony Asleson
2020-09-30  7:38         ` Greg Kroah-Hartman
2020-09-30  7:40           ` Greg Kroah-Hartman
2020-09-30 14:35           ` Tony Asleson
2020-10-01 11:48             ` Greg Kroah-Hartman [this message]
2020-10-07 20:10               ` Tony Asleson
2020-10-08  4:48                 ` Greg Kroah-Hartman
2020-10-08 20:49                   ` Martin K. Petersen
2020-10-08  5:54                 ` Hannes Reinecke
2020-10-08  6:22                 ` Finn Thain
2020-09-25 16:19 ` [v5 02/12] create_syslog_header: Add durable name Tony Asleson
2020-09-25 16:19 ` [v5 03/12] dev_vprintk_emit: Increase hdr size Tony Asleson
2020-09-25 16:19 ` [v5 04/12] scsi: Add durable_name for dev_printk Tony Asleson
2020-09-25 16:19 ` [v5 05/12] nvme: Add durable name " Tony Asleson
2020-09-25 16:19 ` [v5 06/12] libata: Add ata_scsi_durable_name Tony Asleson
2020-09-25 16:19 ` [v5 07/12] libata: Make ata_scsi_durable_name static Tony Asleson
2020-09-26  8:40   ` Sergei Shtylyov
2020-09-26 14:17     ` Tony Asleson
2020-09-26 15:57       ` James Bottomley
2020-09-28 20:35     ` Tony Asleson
2020-09-25 16:19 ` [v5 08/12] Add durable_name_printk Tony Asleson
2020-09-26 23:53   ` Randy Dunlap
2020-09-28 15:52     ` Tony Asleson
2020-09-28 17:32       ` Randy Dunlap
2020-09-25 16:19 ` [v5 09/12] libata: use durable_name_printk Tony Asleson
2020-09-25 16:19 ` [v5 10/12] Add durable_name_printk_ratelimited Tony Asleson
2020-09-25 16:19 ` [v5 11/12] print_req_error: Use durable_name_printk_ratelimited Tony Asleson
2020-09-25 16:19 ` [v5 12/12] buffer_io_error: " Tony Asleson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201001114832.GC2368232@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=hare@suse.de \
    --cc=hch@infradead.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=tasleson@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).