Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
From: Tony Asleson <tasleson@redhat.com>
To: Hannes Reinecke <hare@suse.de>,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: Re: [RFC PATCH v2 5/7] ata_dev_printk: Use dev_printk
Date: Thu, 14 May 2020 13:03:18 -0500
Message-ID: <e12aeb9e-fe5d-5b5e-d190-401997cecc34@redhat.com> (raw)
In-Reply-To: <82257837-c5a8-6a38-ce13-0f1ce7e245ac@suse.de>

On 5/14/20 12:53 AM, Hannes Reinecke wrote:
> On 5/13/20 11:36 PM, Tony Asleson wrote:
>> Utilize the dev_printk function which will add structured data
>> to the log message.
>>
>> Signed-off-by: Tony Asleson <tasleson@redhat.com>
>> ---
>>   drivers/ata/libata-core.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 42c8728f6117..16978d615a17 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -7301,6 +7301,7 @@ EXPORT_SYMBOL(ata_link_printk);
>>   void ata_dev_printk(const struct ata_device *dev, const char *level,
>>               const char *fmt, ...)
>>   {
>> +    const struct device *gendev;
>>       struct va_format vaf;
>>       va_list args;
>>   @@ -7309,9 +7310,12 @@ 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);
>> +    gendev = (dev->sdev) ? &dev->sdev->sdev_gendev : &dev->tdev;
>> +
>> +    dev_printk(level, gendev, "ata%u.%02u: %pV",
>> +            dev->link->ap->print_id,
>> +            dev->link->pmp + dev->devno,
>> +            &vaf);
>>         va_end(args);
>>   }
>>
> That is wrong.
> dev_printk() will already prefix the logging message with the device
> name, so we'll end up having the name printed twice.

It certainly could be. Early in boot when &dev->sdev->sdev_gendev ==
NULL and &dev->tdev is used we get

dev1.0: ata1.00: configured for UDMA/100

later when &dev->sdev->sdev_gendev != NULL we get

sd 1:0:0:0: [sdb] 209715200 512-byte logical blocks: (107 GB/100 GiB)

to clarify, your point is dev1.0 is redundant as ata1.00 exists in the
message?


In the block layer print_req_error we get:

block sdb: blk_update_request: I/O error, dev sdb, sector 10000 op
0x0:(READ) flags 0x80700 phys_seg 4 prio class 0

Which seems a bit more redundant.

I've been trying to be careful to not change the human readable portion
of the message, so not to disturb all the log scraping tools that exist
mining errors.  Maybe this is the wrong approach?  In my original patch
series I brought back printk_emit so that I could add the structured
data without introducing changes in the message text output.  James
Bottomley suggested using dev_printk which certainly made things
cleaner, but it does add the prefix.


Thanks,
Tony


  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 21:36 [RFC PATCH v2 0/7] Add persistent durable identifier to storage log messages Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 1/7] struct device: Add function callback durable_name Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 2/7] create_syslog_header: Add durable name Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 3/7] print_req_error: Use dev_printk Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 4/7] buffer_io_error: " Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 5/7] ata_dev_printk: " Tony Asleson
2020-05-14  5:53   ` Hannes Reinecke
2020-05-14 18:03     ` Tony Asleson [this message]
     [not found]       ` <CGME20200626135627eucas1p2d68eb5853f90bc636faab69149fbe02c@eucas1p2.samsung.com>
2020-06-26 13:56         ` Bartlomiej Zolnierkiewicz
2020-05-13 21:36 ` [RFC PATCH v2 6/7] scsi: Add durable_name for dev_printk Tony Asleson
2020-05-13 21:36 ` [RFC PATCH v2 7/7] nvme: Add durable name " Tony Asleson
2020-05-13 23:04   ` Keith Busch
2020-05-14 17:13     ` Tony Asleson
2020-05-14 17:20       ` Keith Busch

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=e12aeb9e-fe5d-5b5e-d190-401997cecc34@redhat.com \
    --to=tasleson@redhat.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=hare@suse.de \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    /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

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-scsi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git