* [v4 01/11] struct device: Add function callback durable_name
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
@ 2020-07-24 17:16 ` Tony Asleson
2020-07-24 17:16 ` [v4 02/11] create_syslog_header: Add durable name Tony Asleson
` (10 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:16 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Function callback and function to be used to write a persistent
durable name to the supplied character buffer. This will be used to add
structured key-value data to log messages for hardware related errors
which allows end users to correlate message and specific hardware.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/base/core.c | 24 ++++++++++++++++++++++++
include/linux/device.h | 4 ++++
2 files changed, 28 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0cad34f1eede..511b7d2fc916 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2304,6 +2304,30 @@ int dev_set_name(struct device *dev, const char *fmt, ...)
}
EXPORT_SYMBOL_GPL(dev_set_name);
+/**
+ * dev_durable_name - Write "DURABLE_NAME"=<durable name> in buffer
+ * @dev: device
+ * @buffer: character buffer to write results
+ * @len: length of buffer
+ * @return: Number of bytes written to buffer
+ */
+int dev_durable_name(const struct device *dev, char *buffer, size_t len)
+{
+ int tmp, dlen;
+
+ if (dev && dev->durable_name) {
+ tmp = snprintf(buffer, len, "DURABLE_NAME=");
+ if (tmp < len) {
+ dlen = dev->durable_name(dev, buffer + tmp,
+ len - tmp);
+ if (dlen > 0 && ((dlen + tmp) < len))
+ return dlen + tmp;
+ }
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dev_durable_name);
+
/**
* device_to_dev_kobj - select a /sys/dev/ directory for the device
* @dev: device
diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..281755404c21 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -613,6 +613,8 @@ struct device {
struct iommu_group *iommu_group;
struct dev_iommu *iommu;
+ int (*durable_name)(const struct device *dev, char *buff, size_t len);
+
bool offline_disabled:1;
bool offline:1;
bool of_node_reused:1;
@@ -654,6 +656,8 @@ static inline const char *dev_name(const struct device *dev)
extern __printf(2, 3)
int dev_set_name(struct device *dev, const char *name, ...);
+int dev_durable_name(const struct device *d, char *buffer, size_t len);
+
#ifdef CONFIG_NUMA
static inline int dev_to_node(struct device *dev)
{
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4 02/11] create_syslog_header: Add durable name
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
2020-07-24 17:16 ` [v4 01/11] struct device: Add function callback durable_name Tony Asleson
@ 2020-07-24 17:16 ` Tony Asleson
2020-07-24 17:16 ` [v4 03/11] dev_vprintk_emit: Increase hdr size Tony Asleson
` (9 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:16 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
This gets us a persistent durable name for code that logs messages in the
block layer that have the appropriate callbacks setup for durable name.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/base/core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 511b7d2fc916..964690572a89 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3754,6 +3754,7 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
{
const char *subsys;
size_t pos = 0;
+ int dlen;
if (dev->class)
subsys = dev->class->name;
@@ -3796,6 +3797,10 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
"DEVICE=+%s:%s", subsys, dev_name(dev));
}
+ dlen = dev_durable_name(dev, hdr + (pos + 1), hdrlen - (pos + 1));
+ if (dlen)
+ pos += dlen + 1;
+
if (pos >= hdrlen)
goto overflow;
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4 03/11] dev_vprintk_emit: Increase hdr size
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
2020-07-24 17:16 ` [v4 01/11] struct device: Add function callback durable_name Tony Asleson
2020-07-24 17:16 ` [v4 02/11] create_syslog_header: Add durable name Tony Asleson
@ 2020-07-24 17:16 ` Tony Asleson
2020-07-25 10:05 ` Andy Shevchenko
2020-07-24 17:16 ` [v4 04/11] scsi: Add durable_name for dev_printk Tony Asleson
` (8 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:16 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
With the addition of the device persistent id we have the possibility
of adding 154 more bytes to the hdr. Thus if we assume the previous
size of 128 was sufficent we can simply add the 2 amounts and round
up.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/base/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 964690572a89..c2439d12608d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3814,7 +3814,7 @@ create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
int dev_vprintk_emit(int level, const struct device *dev,
const char *fmt, va_list args)
{
- char hdr[128];
+ char hdr[288];
size_t hdrlen;
hdrlen = create_syslog_header(dev, hdr, sizeof(hdr));
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 03/11] dev_vprintk_emit: Increase hdr size
2020-07-24 17:16 ` [v4 03/11] dev_vprintk_emit: Increase hdr size Tony Asleson
@ 2020-07-25 10:05 ` Andy Shevchenko
2020-08-26 18:26 ` Tony Asleson
0 siblings, 1 reply; 30+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:05 UTC (permalink / raw)
To: Tony Asleson
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> With the addition of the device persistent id we have the possibility
> of adding 154 more bytes to the hdr. Thus if we assume the previous
> size of 128 was sufficent we can simply add the 2 amounts and round
sufficient
> up.
...
> - char hdr[128];
> + char hdr[288];
This is quite a drastic change for the stack.
Can you refactor to avoid this?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4 03/11] dev_vprintk_emit: Increase hdr size
2020-07-25 10:05 ` Andy Shevchenko
@ 2020-08-26 18:26 ` Tony Asleson
0 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-08-26 18:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On 7/25/20 5:05 AM, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
>> - char hdr[128];
>> + char hdr[288];
>
> This is quite a drastic change for the stack.
> Can you refactor to avoid this?
The only thing I can think of is using a hash of the identifier instead
of the value itself. This could drastically reduce the stack usage and
data stored in journal, but it makes it kind of clumsy for using.
Would placing this on the heap be acceptable?
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4 04/11] scsi: Add durable_name for dev_printk
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (2 preceding siblings ...)
2020-07-24 17:16 ` [v4 03/11] dev_vprintk_emit: Increase hdr size Tony Asleson
@ 2020-07-24 17:16 ` Tony Asleson
2020-07-25 10:20 ` Andy Shevchenko
2020-07-24 17:17 ` [v4 05/11] nvme: Add durable name " Tony Asleson
` (7 subsequent siblings)
11 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:16 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Add the needed functions to fill out the durable_name function
call back for scsi based storage devices. This allows calls
into dev_printk for scsi devices to have a persistent id
associated with them.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/scsi/scsi_lib.c | 14 ++++++++++++++
drivers/scsi/scsi_sysfs.c | 23 +++++++++++++++++++++++
drivers/scsi/sd.c | 2 ++
include/scsi/scsi_device.h | 3 +++
4 files changed, 42 insertions(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 06c260f6cdae..9f6c41162c55 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3142,3 +3142,17 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
return group_id;
}
EXPORT_SYMBOL(scsi_vpd_tpg_id);
+
+int scsi_durable_name(struct scsi_device *sdev, char *buf, size_t len)
+{
+ int vpd_len = 0;
+
+ vpd_len = scsi_vpd_lun_id(sdev, buf, len);
+ if (vpd_len > 0 && vpd_len < len)
+ vpd_len++;
+ else
+ vpd_len = 0;
+
+ return vpd_len;
+}
+EXPORT_SYMBOL(scsi_durable_name);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..f719b63f4b63 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1582,6 +1582,28 @@ static struct device_type scsi_dev_type = {
.groups = scsi_sdev_attr_groups,
};
+
+int dev_to_scsi_durable_name(const struct device *dev, char *buf, size_t len)
+{
+ struct scsi_device *sd_dev = NULL;
+
+ // When we go through dev_printk in the scsi layer, dev is embedded
+ // in a struct scsi_device. When we go through the block layer,
+ // dev is embedded in struct genhd, thus we need different paths to
+ // retrieve the struct scsi_device to call scsi_durable_name.
+ if (dev->type == &scsi_dev_type) {
+ sd_dev = to_scsi_device(dev);
+ } else if (dev->parent && dev->parent->type == &scsi_dev_type) {
+ sd_dev = to_scsi_device(dev->parent);
+ } else {
+ // We have a pointer to something else, bail
+ return 0;
+ }
+
+ return scsi_durable_name(sd_dev, buf, len);
+}
+EXPORT_SYMBOL(dev_to_scsi_durable_name);
+
void scsi_sysfs_device_initialize(struct scsi_device *sdev)
{
unsigned long flags;
@@ -1591,6 +1613,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
device_initialize(&sdev->sdev_gendev);
sdev->sdev_gendev.bus = &scsi_bus_type;
sdev->sdev_gendev.type = &scsi_dev_type;
+ sdev->sdev_gendev.durable_name = dev_to_scsi_durable_name;
dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a793cb08d025..f40e4cb4a5f6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3360,6 +3360,8 @@ static int sd_probe(struct device *dev)
gd->private_data = &sdkp->driver;
gd->queue = sdkp->device->request_queue;
+ disk_to_dev(gd)->durable_name = dev_to_scsi_durable_name;
+
/* defaults, until the device tells us otherwise */
sdp->sector_size = 512;
sdkp->capacity = 0;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..7be5861565f7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -461,6 +461,9 @@ extern void sdev_disable_disk_events(struct scsi_device *sdev);
extern void sdev_enable_disk_events(struct scsi_device *sdev);
extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
extern int scsi_vpd_tpg_id(struct scsi_device *, int *);
+extern int dev_to_scsi_durable_name(const struct device *dev, char *buf,
+ size_t len);
+extern int scsi_durable_name(struct scsi_device *sdev, char *buf, size_t len);
#ifdef CONFIG_PM
extern int scsi_autopm_get_device(struct scsi_device *);
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 04/11] scsi: Add durable_name for dev_printk
2020-07-24 17:16 ` [v4 04/11] scsi: Add durable_name for dev_printk Tony Asleson
@ 2020-07-25 10:20 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:20 UTC (permalink / raw)
To: Tony Asleson
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> Add the needed functions to fill out the durable_name function
> call back for scsi based storage devices. This allows calls
> into dev_printk for scsi devices to have a persistent id
> associated with them.
...
> +int scsi_durable_name(struct scsi_device *sdev, char *buf, size_t len)
> +{
> + int vpd_len = 0;
What is the purpose of the assignment?
> + vpd_len = scsi_vpd_lun_id(sdev, buf, len);
> + if (vpd_len > 0 && vpd_len < len)
> + vpd_len++;
> + else
> + vpd_len = 0;
> +
> + return vpd_len;
int vpd_len;
vpd_len = ...(...);
if (vpd_len ...)
return vpd_len + 1;
return 0;
> +}
...
> };
>
> +
One blank line is enough.
> +int dev_to_scsi_durable_name(const struct device *dev, char *buf, size_t len)
> +{
> + struct scsi_device *sd_dev = NULL;
Redundant assignment.
> + // When we go through dev_printk in the scsi layer, dev is embedded
> + // in a struct scsi_device. When we go through the block layer,
> + // dev is embedded in struct genhd, thus we need different paths to
> + // retrieve the struct scsi_device to call scsi_durable_name.
> + if (dev->type == &scsi_dev_type) {
> + sd_dev = to_scsi_device(dev);
This...
> + } else if (dev->parent && dev->parent->type == &scsi_dev_type) {
> + sd_dev = to_scsi_device(dev->parent);
...and this along with pieces in scsi_bus_uevent(), scsi_bus_match()
are candidates for a good helper (perhaps with utilization of
scsi_is_sdev_device(), but maybe not necessary).
static struct scsi_device *dev_to_scsi_device(struct device *dev)
{
return dev->type == &scsi_dev_type ? to_scsi_device(dev) : NULL;
}
sdev = dev_to_scsi_device(dev);
if (!sdev)
sdev = dev_to_scsi_device(dev->parent);
if (!sdev)
return 0;
And when introducing a helper change users at the same time.
> + } else {
> + // We have a pointer to something else, bail
> + return 0;
> + }
> +
> + return scsi_durable_name(sd_dev, buf, len);
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4 05/11] nvme: Add durable name for dev_printk
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (3 preceding siblings ...)
2020-07-24 17:16 ` [v4 04/11] scsi: Add durable_name for dev_printk Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-25 9:05 ` Sergei Shtylyov
2020-07-25 10:23 ` Andy Shevchenko
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
` (6 subsequent siblings)
11 siblings, 2 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Corrections from Keith Busch review comments.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/nvme/host/core.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f3c037f5a9ba..f2e5b91668a1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2667,6 +2667,22 @@ static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
return true;
}
+static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
+ char *buf);
+
+static int dev_to_nvme_durable_name(const struct device *dev, char *buf, size_t len)
+{
+ char serial[144]; /* Max 141 for wwid_show */
+ ssize_t serial_len = wwid_show((struct device *)dev, NULL, serial);
+
+ if (serial_len > 0 && serial_len < len) {
+ serial_len -= 1; /* Remove the '\n' from the string */
+ strncpy(buf, serial, serial_len);
+ return serial_len;
+ }
+ return 0;
+}
+
static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
{
struct nvme_subsystem *subsys, *found;
@@ -3616,6 +3632,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
disk->queue = ns->queue;
disk->flags = flags;
memcpy(disk->disk_name, disk_name, DISK_NAME_LEN);
+ disk_to_dev(disk)->durable_name = dev_to_nvme_durable_name;
+
ns->disk = disk;
__nvme_revalidate_disk(disk, id);
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 05/11] nvme: Add durable name for dev_printk
2020-07-24 17:17 ` [v4 05/11] nvme: Add durable name " Tony Asleson
@ 2020-07-25 9:05 ` Sergei Shtylyov
2020-07-25 10:23 ` Andy Shevchenko
1 sibling, 0 replies; 30+ messages in thread
From: Sergei Shtylyov @ 2020-07-25 9:05 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Hello!
On 24.07.2020 20:17, Tony Asleson wrote:
> Corrections from Keith Busch review comments.
>
> Signed-off-by: Tony Asleson <tasleson@redhat.com>
> ---
> drivers/nvme/host/core.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f3c037f5a9ba..f2e5b91668a1 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2667,6 +2667,22 @@ static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
> return true;
> }
>
> +static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +
> +static int dev_to_nvme_durable_name(const struct device *dev, char *buf, size_t len)
> +{
> + char serial[144]; /* Max 141 for wwid_show */
> + ssize_t serial_len = wwid_show((struct device *)dev, NULL, serial);
> +
> + if (serial_len > 0 && serial_len < len) {
> + serial_len -= 1; /* Remove the '\n' from the string */
serial_len-- instead?
> + strncpy(buf, serial, serial_len);
> + return serial_len;
> + }
> + return 0;
> +}
> +
> static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> {
> struct nvme_sub
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4 05/11] nvme: Add durable name for dev_printk
2020-07-24 17:17 ` [v4 05/11] nvme: Add durable name " Tony Asleson
2020-07-25 9:05 ` Sergei Shtylyov
@ 2020-07-25 10:23 ` Andy Shevchenko
1 sibling, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:23 UTC (permalink / raw)
To: Tony Asleson
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> Corrections from Keith Busch review comments.
Good! And where is the commit message?
> +static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +
> +static int dev_to_nvme_durable_name(const struct device *dev, char *buf, size_t len)
> +{
> + char serial[144]; /* Max 141 for wwid_show */
> + ssize_t serial_len = wwid_show((struct device *)dev, NULL, serial);
> +
> + if (serial_len > 0 && serial_len < len) {
> + serial_len -= 1; /* Remove the '\n' from the string */
> + strncpy(buf, serial, serial_len);
> + return serial_len;
> + }
> + return 0;
> +}
Sorry, but this is ugly. Can we rather get some common code from
wwid_show() and reuse it there and here w/o above dances with \n?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4 06/11] libata: Add ata_scsi_durable_name
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (4 preceding siblings ...)
2020-07-24 17:17 ` [v4 05/11] nvme: Add durable name " Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-25 0:48 ` kernel test robot
` (4 more replies)
2020-07-24 17:17 ` [v4 07/11] Add durable_name_printk Tony Asleson
` (5 subsequent siblings)
11 siblings, 5 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Function used to create the durable name for ata scsi.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/ata/libata-scsi.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 36e588d88b95..ec1f6e406ceb 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1091,6 +1091,14 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
return 0;
}
+int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
+{
+ struct ata_device *ata_dev = container_of(dev, struct ata_device, tdev);
+
+ return scsi_durable_name(ata_dev->sdev, buf, len);
+}
+
+
/**
* ata_scsi_slave_config - Set SCSI device attributes
* @sdev: SCSI device to examine
@@ -1111,8 +1119,11 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
ata_scsi_sdev_config(sdev);
- if (dev)
+ if (dev) {
rc = ata_scsi_dev_config(sdev, dev);
+ if (!rc)
+ dev->tdev.durable_name = ata_scsi_durable_name;
+ }
return rc;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 06/11] libata: Add ata_scsi_durable_name
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
@ 2020-07-25 0:48 ` kernel test robot
2020-07-25 1:07 ` kernel test robot
` (3 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-07-25 0:48 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]
Hi Tony,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162]
url: https://github.com/0day-ci/linux/commits/Tony-Asleson/Add-persistent-durable-identifier-to-storage-log-messages/20200725-011936
base: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/ata/libata-scsi.c:1094:5: warning: no previous prototype for 'ata_scsi_durable_name' [-Wmissing-prototypes]
1094 | int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
| ^~~~~~~~~~~~~~~~~~~~~
vim +/ata_scsi_durable_name +1094 drivers/ata/libata-scsi.c
1093
> 1094 int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
1095 {
1096 struct ata_device *ata_dev = container_of(dev, struct ata_device, tdev);
1097
1098 return scsi_durable_name(ata_dev->sdev, buf, len);
1099 }
1100
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72863 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4 06/11] libata: Add ata_scsi_durable_name
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
2020-07-25 0:48 ` kernel test robot
@ 2020-07-25 1:07 ` kernel test robot
2020-07-25 1:07 ` [RFC PATCH] libata: ata_scsi_durable_name() can be static kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-07-25 1:07 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
Hi Tony,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162]
url: https://github.com/0day-ci/linux/commits/Tony-Asleson/Add-persistent-durable-identifier-to-storage-log-messages/20200725-011936
base: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
config: x86_64-randconfig-s022-20200724 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-93-g4c6cbe55-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/ata/libata-scsi.c:1094:5: sparse: sparse: symbol 'ata_scsi_durable_name' was not declared. Should it be static?
drivers/ata/libata-scsi.c:1803:13: sparse: sparse: context imbalance in 'ata_scsi_rbuf_get' - wrong count at exit
drivers/ata/libata-scsi.c:1833:31: sparse: sparse: context imbalance in 'ata_scsi_rbuf_fill' - unexpected unlock
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32757 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* [RFC PATCH] libata: ata_scsi_durable_name() can be static
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
2020-07-25 0:48 ` kernel test robot
2020-07-25 1:07 ` kernel test robot
@ 2020-07-25 1:07 ` kernel test robot
2020-07-25 1:18 ` [v4 06/11] libata: Add ata_scsi_durable_name kernel test robot
2020-07-25 10:26 ` Andy Shevchenko
4 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-07-25 1:07 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Cc: kbuild-all
Signed-off-by: kernel test robot <lkp@intel.com>
---
libata-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ec1f6e406ceb2..32bd2426ca4f5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1091,7 +1091,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
return 0;
}
-int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
+static int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
{
struct ata_device *ata_dev = container_of(dev, struct ata_device, tdev);
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 06/11] libata: Add ata_scsi_durable_name
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
` (2 preceding siblings ...)
2020-07-25 1:07 ` [RFC PATCH] libata: ata_scsi_durable_name() can be static kernel test robot
@ 2020-07-25 1:18 ` kernel test robot
2020-07-25 10:26 ` Andy Shevchenko
4 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2020-07-25 1:18 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Cc: kbuild-all, clang-built-linux
[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]
Hi Tony,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162]
url: https://github.com/0day-ci/linux/commits/Tony-Asleson/Add-persistent-durable-identifier-to-storage-log-messages/20200725-011936
base: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
config: x86_64-randconfig-a005-20200724 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 1d09ecf36175f7910ffedd6d497c07b5c74c22fb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/ata/libata-scsi.c:1094:5: warning: no previous prototype for function 'ata_scsi_durable_name' [-Wmissing-prototypes]
int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
^
drivers/ata/libata-scsi.c:1094:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
^
static
1 warning generated.
vim +/ata_scsi_durable_name +1094 drivers/ata/libata-scsi.c
1093
> 1094 int ata_scsi_durable_name(const struct device *dev, char *buf, size_t len)
1095 {
1096 struct ata_device *ata_dev = container_of(dev, struct ata_device, tdev);
1097
1098 return scsi_durable_name(ata_dev->sdev, buf, len);
1099 }
1100
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26996 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4 06/11] libata: Add ata_scsi_durable_name
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
` (3 preceding siblings ...)
2020-07-25 1:18 ` [v4 06/11] libata: Add ata_scsi_durable_name kernel test robot
@ 2020-07-25 10:26 ` Andy Shevchenko
4 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:26 UTC (permalink / raw)
To: Tony Asleson
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> Function used to create the durable name for ata scsi.
https://chris.beams.io/posts/git-commit/
> Signed-off-by: Tony Asleson <tasleson@redhat.com>
...
> +}
> +
> +
One is enough, really!
> /**
...
> - if (dev)
> + if (dev) {
> rc = ata_scsi_dev_config(sdev, dev);
> + if (!rc)
> + dev->tdev.durable_name = ata_scsi_durable_name;
> + }
Can we stick to our usual pattern?
rc = ...
if (rc)
return rc;
...
> return rc;
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4 07/11] Add durable_name_printk
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (5 preceding siblings ...)
2020-07-24 17:17 ` [v4 06/11] libata: Add ata_scsi_durable_name Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-24 17:17 ` [v4 08/11] libata: use durable_name_printk Tony Asleson
` (4 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Ideally block related code would standardize on using dev_printk,
but dev_printk does change the user visible messages which is
questionable. Adding this function which adds the structured
key/value durable name to the log entry. It has the
same signature as dev_printk. In the future, code that
is using this could easily transition to dev_printk when that
becomes workable.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/base/core.c | 15 +++++++++++++++
include/linux/dev_printk.h | 5 +++++
2 files changed, 20 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index c2439d12608d..aaf7b8256712 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3865,6 +3865,21 @@ void dev_printk(const char *level, const struct device *dev,
}
EXPORT_SYMBOL(dev_printk);
+void durable_name_printk(const char *level, const struct device *dev,
+ const char *fmt, ...)
+{
+ size_t dictlen;
+ va_list args;
+ char dict[288];
+
+ dictlen = dev_durable_name(dev, dict, sizeof(dict));
+
+ va_start(args, fmt);
+ vprintk_emit(0, level[1] - '0', dict, dictlen, fmt, args);
+ va_end(args);
+}
+EXPORT_SYMBOL(durable_name_printk);
+
#define define_dev_printk_level(func, kern_level) \
void func(const struct device *dev, const char *fmt, ...) \
{ \
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 5aad06b4ca7b..502cf9fd7fe7 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -32,6 +32,11 @@ int dev_printk_emit(int level, const struct device *dev, const char *fmt, ...);
__printf(3, 4) __cold
void dev_printk(const char *level, const struct device *dev,
const char *fmt, ...);
+
+__printf(3, 4) __cold
+void durable_name_printk(const char *level, const struct device *dev,
+ const char *fmt, ...);
+
__printf(2, 3) __cold
void _dev_emerg(const struct device *dev, const char *fmt, ...);
__printf(2, 3) __cold
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4 08/11] libata: use durable_name_printk
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (6 preceding siblings ...)
2020-07-24 17:17 ` [v4 07/11] Add durable_name_printk Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-24 17:17 ` [v4 09/11] Add durable_name_printk_ratelimited Tony Asleson
` (3 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Utilize durable_name_printk to associate the durable name
with the log message via structured data. The user visible
portion of the log message is unchanged.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
drivers/ata/libata-core.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index beca5f91bb4c..468aa3f7eaad 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6444,7 +6444,8 @@ void ata_port_printk(const struct ata_port *ap, const char *level,
vaf.fmt = fmt;
vaf.va = &args;
- printk("%sata%u: %pV", level, ap->print_id, &vaf);
+ durable_name_printk(level, &ap->tdev, "ata%u: %pV",
+ ap->print_id, &vaf);
va_end(args);
}
@@ -6462,11 +6463,11 @@ void ata_link_printk(const struct ata_link *link, const char *level,
vaf.va = &args;
if (sata_pmp_attached(link->ap) || link->ap->slave_link)
- printk("%sata%u.%02u: %pV",
- level, link->ap->print_id, link->pmp, &vaf);
+ durable_name_printk(level, &link->tdev, "ata%u.%02u: %pV",
+ link->ap->print_id, link->pmp, &vaf);
else
- printk("%sata%u: %pV",
- level, link->ap->print_id, &vaf);
+ durable_name_printk(level, &link->tdev, "ata%u: %pV",
+ link->ap->print_id, &vaf);
va_end(args);
}
@@ -6483,9 +6484,9 @@ void ata_dev_printk(const struct ata_device *dev, const char *level,
vaf.fmt = fmt;
vaf.va = &args;
- printk("%sata%u.%02u: %pV",
- level, dev->link->ap->print_id, dev->link->pmp + dev->devno,
- &vaf);
+ durable_name_printk(level, &dev->tdev, "ata%u.%02u: %pV",
+ dev->link->ap->print_id, dev->link->pmp + dev->devno,
+ &vaf);
va_end(args);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4 09/11] Add durable_name_printk_ratelimited
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (7 preceding siblings ...)
2020-07-24 17:17 ` [v4 08/11] libata: use durable_name_printk Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-24 17:17 ` [v4 10/11] print_req_error: Use durable_name_printk_ratelimited Tony Asleson
` (2 subsequent siblings)
11 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Create a rate limited macro for durable_name_printk so that
we can use this for printk_ratelimited usage in the block
layers and add the durable name key/value to the log
message.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
include/linux/dev_printk.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 502cf9fd7fe7..931977f5c85e 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -37,6 +37,15 @@ __printf(3, 4) __cold
void durable_name_printk(const char *level, const struct device *dev,
const char *fmt, ...);
+#define durable_name_printk_ratelimited(level, dev, fmt, ...) \
+do { \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ if (__ratelimit(&_rs)) \
+ durable_name_printk(level, dev, fmt, ##__VA_ARGS__); \
+} while (0)
+
__printf(2, 3) __cold
void _dev_emerg(const struct device *dev, const char *fmt, ...);
__printf(2, 3) __cold
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [v4 10/11] print_req_error: Use durable_name_printk_ratelimited
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (8 preceding siblings ...)
2020-07-24 17:17 ` [v4 09/11] Add durable_name_printk_ratelimited Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-25 9:15 ` Sergei Shtylyov
2020-07-24 17:17 ` [v4 11/11] buffer_io_error: " Tony Asleson
2020-07-26 15:10 ` [v4 00/11] Add persistent durable identifier to storage log messages Christoph Hellwig
11 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Replace printk_ratelimited with one that adds the key/value
durable name to log entry.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
block/blk-core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 9bfaee050c82..a1f35e3e21d8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -213,12 +213,15 @@ EXPORT_SYMBOL_GPL(blk_status_to_errno);
static void print_req_error(struct request *req, blk_status_t status,
const char *caller)
{
+ struct device *dev;
int idx = (__force int)status;
if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors)))
return;
- printk_ratelimited(KERN_ERR
+ dev = (req->rq_disk) ? disk_to_dev(req->rq_disk) : NULL;
+
+ durable_name_printk_ratelimited(KERN_ERR, dev,
"%s: %s error, dev %s, sector %llu op 0x%x:(%s) flags 0x%x "
"phys_seg %u prio class %u\n",
caller, blk_errors[idx].name,
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 10/11] print_req_error: Use durable_name_printk_ratelimited
2020-07-24 17:17 ` [v4 10/11] print_req_error: Use durable_name_printk_ratelimited Tony Asleson
@ 2020-07-25 9:15 ` Sergei Shtylyov
0 siblings, 0 replies; 30+ messages in thread
From: Sergei Shtylyov @ 2020-07-25 9:15 UTC (permalink / raw)
To: Tony Asleson, linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 24.07.2020 20:17, Tony Asleson wrote:
> Replace printk_ratelimited with one that adds the key/value
> durable name to log entry.
>
> Signed-off-by: Tony Asleson <tasleson@redhat.com>
> ---
> block/blk-core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9bfaee050c82..a1f35e3e21d8 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -213,12 +213,15 @@ EXPORT_SYMBOL_GPL(blk_status_to_errno);
> static void print_req_error(struct request *req, blk_status_t status,
> const char *caller)
> {
> + struct device *dev;
> int idx = (__force int)status;
>
> if (WARN_ON_ONCE(idx >= ARRAY_SIZE(blk_errors)))
> return;
>
> - printk_ratelimited(KERN_ERR
> + dev = (req->rq_disk) ? disk_to_dev(req->rq_disk) : NULL;
The 1st () not necessary, here iand in the next patch...
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 30+ messages in thread
* [v4 11/11] buffer_io_error: Use durable_name_printk_ratelimited
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (9 preceding siblings ...)
2020-07-24 17:17 ` [v4 10/11] print_req_error: Use durable_name_printk_ratelimited Tony Asleson
@ 2020-07-24 17:17 ` Tony Asleson
2020-07-25 10:29 ` Andy Shevchenko
2020-07-26 15:10 ` [v4 00/11] Add persistent durable identifier to storage log messages Christoph Hellwig
11 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-24 17:17 UTC (permalink / raw)
To: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
Replace printk_ratelimited with one that adds the key/value
durable name to log entry.
Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
fs/buffer.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index a60f60396cfa..f35eaaafce0e 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -133,10 +133,16 @@ __clear_page_buffers(struct page *page)
static void buffer_io_error(struct buffer_head *bh, char *msg)
{
- if (!test_bit(BH_Quiet, &bh->b_state))
- printk_ratelimited(KERN_ERR
+ if (!test_bit(BH_Quiet, &bh->b_state)) {
+ struct device *gendev;
+
+ gendev = (bh->b_bdev->bd_disk) ?
+ disk_to_dev(bh->b_bdev->bd_disk) : NULL;
+
+ durable_name_printk_ratelimited(KERN_ERR, gendev,
"Buffer I/O error on dev %pg, logical block %llu%s\n",
bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
+ }
}
/*
--
2.26.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [v4 11/11] buffer_io_error: Use durable_name_printk_ratelimited
2020-07-24 17:17 ` [v4 11/11] buffer_io_error: " Tony Asleson
@ 2020-07-25 10:29 ` Andy Shevchenko
0 siblings, 0 replies; 30+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:29 UTC (permalink / raw)
To: Tony Asleson
Cc: linux-block, linux-ide, linux-scsi, Bartlomiej Zolnierkiewicz,
Jens Axboe
On Fri, Jul 24, 2020 at 8:19 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> Replace printk_ratelimited with one that adds the key/value
> durable name to log entry.
> static void buffer_io_error(struct buffer_head *bh, char *msg)
> {
> - if (!test_bit(BH_Quiet, &bh->b_state))
> - printk_ratelimited(KERN_ERR
> + if (!test_bit(BH_Quiet, &bh->b_state)) {
> + struct device *gendev;
> +
> + gendev = (bh->b_bdev->bd_disk) ?
> + disk_to_dev(bh->b_bdev->bd_disk) : NULL;
Besides unneeded parentheses as Sergey noticed...
> +
> + durable_name_printk_ratelimited(KERN_ERR, gendev,
> "Buffer I/O error on dev %pg, logical block %llu%s\n",
> bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
> + }
...can we drop indentation level?
if (test_bit(...))
return;
...
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-24 17:16 [v4 00/11] Add persistent durable identifier to storage log messages Tony Asleson
` (10 preceding siblings ...)
2020-07-24 17:17 ` [v4 11/11] buffer_io_error: " Tony Asleson
@ 2020-07-26 15:10 ` Christoph Hellwig
2020-07-27 15:45 ` Tony Asleson
11 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-07-26 15:10 UTC (permalink / raw)
To: Tony Asleson; +Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
FYI, I think these identifiers are absolutely horrible and have no
business in dmesg:
Nacked-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-26 15:10 ` [v4 00/11] Add persistent durable identifier to storage log messages Christoph Hellwig
@ 2020-07-27 15:45 ` Tony Asleson
2020-07-27 16:46 ` Hannes Reinecke
0 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-27 15:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 7/26/20 10:10 AM, Christoph Hellwig wrote:
> FYI, I think these identifiers are absolutely horrible and have no
> business in dmesg:
The identifiers are structured data, they're not visible unless you go
looking for them.
I'm open to other suggestions on how we can positively identify storage
devices over time, across reboots, replacement, and dynamic reconfiguration.
My home system has 4 disks, 2 are identical except for serial number.
Even with this simple configuration, it's not trivial to identify which
message goes with which disk across reboots.
-Tony
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-27 15:45 ` Tony Asleson
@ 2020-07-27 16:46 ` Hannes Reinecke
2020-07-27 17:42 ` Tony Asleson
0 siblings, 1 reply; 30+ messages in thread
From: Hannes Reinecke @ 2020-07-27 16:46 UTC (permalink / raw)
To: tasleson, Christoph Hellwig
Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 7/27/20 5:45 PM, Tony Asleson wrote:
> On 7/26/20 10:10 AM, Christoph Hellwig wrote:
>> FYI, I think these identifiers are absolutely horrible and have no
>> business in dmesg:
>
> The identifiers are structured data, they're not visible unless you go
> looking for them.
>
> I'm open to other suggestions on how we can positively identify storage
> devices over time, across reboots, replacement, and dynamic reconfiguration.
>
> My home system has 4 disks, 2 are identical except for serial number.
> Even with this simple configuration, it's not trivial to identify which
> message goes with which disk across reboots.
>
Well; the more important bits would be to identify the physical location
where these disks reside.
If one goes bad it doesn't really help you if have a persistent
identification in the OS; what you really need is a physical indicator
or a physical location allowing you to identify which disk to pull.
Which isn't addressed at all with this patchset (nor should it; the
physical location it typically found via other means).
And for the other use-cases: We do have persistent device links, do we
not? They provide even more information that you'd extract with this
this patchset, and don't require any kernel changes whatsoever.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-27 16:46 ` Hannes Reinecke
@ 2020-07-27 17:42 ` Tony Asleson
2020-07-27 19:17 ` Douglas Gilbert
0 siblings, 1 reply; 30+ messages in thread
From: Tony Asleson @ 2020-07-27 17:42 UTC (permalink / raw)
To: Hannes Reinecke, Christoph Hellwig
Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 7/27/20 11:46 AM, Hannes Reinecke wrote:
> On 7/27/20 5:45 PM, Tony Asleson wrote:
>> On 7/26/20 10:10 AM, Christoph Hellwig wrote:
>>> FYI, I think these identifiers are absolutely horrible and have no
>>> business in dmesg:
>>
>> The identifiers are structured data, they're not visible unless you go
>> looking for them.
>>
>> I'm open to other suggestions on how we can positively identify storage
>> devices over time, across reboots, replacement, and dynamic
>> reconfiguration.
>>
>> My home system has 4 disks, 2 are identical except for serial number.
>> Even with this simple configuration, it's not trivial to identify which
>> message goes with which disk across reboots.
>>
> Well; the more important bits would be to identify the physical location
> where these disks reside.
> If one goes bad it doesn't really help you if have a persistent
> identification in the OS; what you really need is a physical indicator
> or a physical location allowing you to identify which disk to pull.
In my use case I have no slot information. I have no SCSI enclosure
services to toggle identification LEDs or fault LEDs for the drive sled.
For some users the device might be a virtual one in a storage server,
vpd helps.
In my case the in kernel vpd (WWN) data can be used to correlate with
the sticker on the disk as the disks have the WWN printed on them. I
would think this is true for most disks/storage devices, but obviously I
can't make that statement with 100% certainty as I have a small sample size.
> Which isn't addressed at all with this patchset (nor should it; the
> physical location it typically found via other means).
>
> And for the other use-cases: We do have persistent device links, do we
> not?
How does /dev/disk/by-* help when you are looking at the journal from 1
or more reboots ago and the only thing you have in your journal is
something like:
blk_update_request: critical medium error, dev sde, sector 43578 op
0x0:(READ) flags 0x0 phys_seg 1 prio class 0
The links are only valid for right now.
-Tony
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-27 17:42 ` Tony Asleson
@ 2020-07-27 19:17 ` Douglas Gilbert
2020-07-27 20:27 ` Tony Asleson
0 siblings, 1 reply; 30+ messages in thread
From: Douglas Gilbert @ 2020-07-27 19:17 UTC (permalink / raw)
To: tasleson, Hannes Reinecke, Christoph Hellwig
Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 2020-07-27 1:42 p.m., Tony Asleson wrote:
> On 7/27/20 11:46 AM, Hannes Reinecke wrote:
>> On 7/27/20 5:45 PM, Tony Asleson wrote:
>>> On 7/26/20 10:10 AM, Christoph Hellwig wrote:
>>>> FYI, I think these identifiers are absolutely horrible and have no
>>>> business in dmesg:
>>>
>>> The identifiers are structured data, they're not visible unless you go
>>> looking for them.
>>>
>>> I'm open to other suggestions on how we can positively identify storage
>>> devices over time, across reboots, replacement, and dynamic
>>> reconfiguration.
>>>
>>> My home system has 4 disks, 2 are identical except for serial number.
>>> Even with this simple configuration, it's not trivial to identify which
>>> message goes with which disk across reboots.
>>>
>> Well; the more important bits would be to identify the physical location
>> where these disks reside.
>> If one goes bad it doesn't really help you if have a persistent
>> identification in the OS; what you really need is a physical indicator
>> or a physical location allowing you to identify which disk to pull.
>
> In my use case I have no slot information. I have no SCSI enclosure
> services to toggle identification LEDs or fault LEDs for the drive sled.
> For some users the device might be a virtual one in a storage server,
> vpd helps.
>
> In my case the in kernel vpd (WWN) data can be used to correlate with
> the sticker on the disk as the disks have the WWN printed on them. I
> would think this is true for most disks/storage devices, but obviously I
> can't make that statement with 100% certainty as I have a small sample size.
>
>> Which isn't addressed at all with this patchset (nor should it; the
>> physical location it typically found via other means).
>>
>> And for the other use-cases: We do have persistent device links, do we
>> not?
>
> How does /dev/disk/by-* help when you are looking at the journal from 1
> or more reboots ago and the only thing you have in your journal is
> something like:
>
> blk_update_request: critical medium error, dev sde, sector 43578 op
> 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>
> The links are only valid for right now.
Does:
lsscsi -U
or
lsscsi -UU
solve your problem, or come close?
Example:
# lsscsi -UU
[1:0:0:0] disk naa.5000cca02b38d0b8 /dev/sda
[1:0:1:0] disk naa.5000c5003011cb2b /dev/sdb
[1:0:2:0] enclosu naa.5001b4d516ecc03f -
[N:0:1:1] disk eui.e8238fa6bf530001001b448b46bd5525 /dev/nvme0n1
The first two (SAS SSDs) NAAs are printed on the disk labels. I don't
think either that enclosure or the M2 NVMe SSD have their numbers
visible (i.e. the last two lines of output).
If it is what you want, then perhaps you could arrange for its output
to be sent to the log when the system has stabilized after a reboot. That
would only leave disk hotplug events exposed.
Faced with the above medium error I would try:
dd if=<all_possibles> bs=512 skip=43578 iflag=direct of=/dev/null count=1
and look for noise in the logs. Change 'bs=512' up to 4096 if that is
the logical block size. For <all_possibles> use /dev/sde (and /dev/sdf and
/dev/dev/sdg or whatever) IOWs the _whole_ disk device name.
Doug Gilbert
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [v4 00/11] Add persistent durable identifier to storage log messages
2020-07-27 19:17 ` Douglas Gilbert
@ 2020-07-27 20:27 ` Tony Asleson
0 siblings, 0 replies; 30+ messages in thread
From: Tony Asleson @ 2020-07-27 20:27 UTC (permalink / raw)
To: dgilbert, Hannes Reinecke, Christoph Hellwig
Cc: linux-block, linux-ide, linux-scsi, b.zolnierkie, axboe
On 7/27/20 2:17 PM, Douglas Gilbert wrote:
> On 2020-07-27 1:42 p.m., Tony Asleson wrote:
>> On 7/27/20 11:46 AM, Hannes Reinecke wrote:
>>> On 7/27/20 5:45 PM, Tony Asleson wrote:
>>>> On 7/26/20 10:10 AM, Christoph Hellwig wrote:
>>>>> FYI, I think these identifiers are absolutely horrible and have no
>>>>> business in dmesg:
>>>>
>>>> The identifiers are structured data, they're not visible unless you go
>>>> looking for them.
>>>>
>>>> I'm open to other suggestions on how we can positively identify storage
>>>> devices over time, across reboots, replacement, and dynamic
>>>> reconfiguration.
>>>>
>>>> My home system has 4 disks, 2 are identical except for serial number.
>>>> Even with this simple configuration, it's not trivial to identify which
>>>> message goes with which disk across reboots.
>>>>
>>> Well; the more important bits would be to identify the physical location
>>> where these disks reside.
>>> If one goes bad it doesn't really help you if have a persistent
>>> identification in the OS; what you really need is a physical indicator
>>> or a physical location allowing you to identify which disk to pull.
>>
>> In my use case I have no slot information. I have no SCSI enclosure
>> services to toggle identification LEDs or fault LEDs for the drive sled.
>> For some users the device might be a virtual one in a storage server,
>> vpd helps.
>>
>> In my case the in kernel vpd (WWN) data can be used to correlate with
>> the sticker on the disk as the disks have the WWN printed on them. I
>> would think this is true for most disks/storage devices, but obviously I
>> can't make that statement with 100% certainty as I have a small sample
>> size.
>>
>>> Which isn't addressed at all with this patchset (nor should it; the
>>> physical location it typically found via other means).
>>>
>>> And for the other use-cases: We do have persistent device links, do we
>>> not?
>>
>> How does /dev/disk/by-* help when you are looking at the journal from 1
>> or more reboots ago and the only thing you have in your journal is
>> something like:
>>
>> blk_update_request: critical medium error, dev sde, sector 43578 op
>> 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>>
>> The links are only valid for right now.
>
> Does:
> lsscsi -U
> or
> lsscsi -UU
>
> solve your problem, or come close?
>
> Example:
> # lsscsi -UU
> [1:0:0:0] disk naa.5000cca02b38d0b8 /dev/sda
> [1:0:1:0] disk naa.5000c5003011cb2b /dev/sdb
> [1:0:2:0] enclosu naa.5001b4d516ecc03f -
> [N:0:1:1] disk eui.e8238fa6bf530001001b448b46bd5525 /dev/nvme0n1
>
> The first two (SAS SSDs) NAAs are printed on the disk labels. I don't
> think either that enclosure or the M2 NVMe SSD have their numbers
> visible (i.e. the last two lines of output).
>
> If it is what you want, then perhaps you could arrange for its output
> to be sent to the log when the system has stabilized after a reboot. That
> would only leave disk hotplug events exposed.
Yes, if we write a new udev rule or script we could place bread crumbs
in the journal so we can correlate
sda == naa.5000cca02b38d0b8 at the time of the error. However, none of
the existing tooling will allow you to see all the log messages that
pertain to the device easily. The user is still left with a bunch of
log messages that have different ways to refer to the same device
attachment eg. sda, ata1.00, sd 0:0:0:0. For them to understand which
messages go with which device is not trivial. Even if someone writes a
tool to parse the messages, looking for the string that contains the ID
and has the needed decoder information to associate it with the correct
piece of hardware, it's only good until the message changes in the kernel.
If we stuff the defacto ID into the message itself when it occurs, the
ambiguity of what device is associated with a message is removed.
I would like to know, why this is so horrible? Is it processing time in
an error path? Stack usage holding the data in flight, wasted disk
space on disk? Unique identifiers are just too long and terse?
The only valid reason I can think of is someone working with very
sensitive data and not wanting the unique ID of a removable or network
storage device to be in their logs. Of course we could add a disable
boot time option for that or make the default off for those that don't
want/care.
> Faced with the above medium error I would try:
> dd if=<all_possibles> bs=512 skip=43578 iflag=direct of=/dev/null
> count=1
> and look for noise in the logs. Change 'bs=512' up to 4096 if that is
> the logical block size. For <all_possibles> use /dev/sde (and /dev/sdf and
> /dev/dev/sdg or whatever) IOWs the _whole_ disk device name.
Assuming the error reproduces, this would work. However, I think this
solution speaks volumes for how difficult it is to simply identify what
device an error originated from.
-Tony
^ permalink raw reply [flat|nested] 30+ messages in thread