Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Tony Asleson <tasleson@redhat.com>
To: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	linux-ide@vger.kernel.org
Subject: Re: [RFC PATCH v3 5/8] ata_dev_printk: Use dev_printk
Date: Wed, 24 Jun 2020 10:15:44 -0500
Message-ID: <33577b4f-6ee1-f054-8853-b61ca800e10a@redhat.com> (raw)
In-Reply-To: <d817c9dd-6852-9233-5f61-1c0bc0f65ca4@samsung.com>

On 6/24/20 5:35 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added linux-ide ML to Cc: ]
> 
> Hi,

Hello,

> 
> On 6/23/20 9:17 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 beca5f91bb4c..44c874367fe3 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -6475,6 +6475,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;
>>  
>> @@ -6483,9 +6484,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,
> 
> This duplicates the device information and breaks integrity of
> libata logging functionality (ata_{dev,link,port}_printk() should
> be all converted to use dev_printk() at the same time).
> 
> The root source of problem is that libata transport uses different
> naming scheme for ->tdev devices (please see dev_set_name() in
> ata_t{dev,link,port}_add()) than libata core for its logging
> functionality (ata_{dev,link,port}_printk()).
> 
> Since libata transport is part of sysfs ABI we should be careful
> to not break it so one idea for solving the issue is to convert
> ata_t{dev,link,port}_add() to use libata logging naming scheme and
> at the same time add sysfs symlinks for the old libata transport
> naming scheme.
> 
> dev->sdev usage is not required for dev_printk() conversion and
> should be considered as a separate change (since it also breaks
> integrity of libata logging and can be considered as a mild
> "layering violation" I don't think that it should be applied).

The point of this patch series is to attach a device unique identifier
to the storage device log messages as structured data.  Originally I
resurrected and used printk_emit, but it was suggested I leverage
dev_printk.  dev_printk does change the output of the log message to
include duplicate information if the message isn't changed. You are not
the first person to raise that concern.  I listed this as an open
question in the cover letter.  I've wanted to preserve the original log
message, so as to not break user space mining tools and I've been
concerned that dev_printk prefixing with an id may already do that.
Adding structured data is invisible to them, or at the least shouldn't
break them, eg. adding a new key-value pair.

I can understand the desire to make all the ata logging functions
consistent, and use dev_printk if we go this way.  However, for this
change it wasn't really the goal to refactor all the logging everywhere
to use dev_printk, although that may be a good change in general.  This
is especially true that if at the end of the refactor to use dev_printk
we consider it a layering violation to call into the existing
functionality to return the vpd ID for the device and reject that part
of the change.

What I am hoping is that we can all agree that having a persistent
identifier associated to storage related log messages is indeed useful.
If we can agree on that, then I would like to have the discussion on
what's the best way to achieve that.

Thanks,
Tony


  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 19:17 [RFC PATCH v3 0/8] Add persistent durable identifier to storage log messages Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 1/8] struct device: Add function callback durable_name Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 2/8] create_syslog_header: Add durable name Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 3/8] print_req_error: Use dev_printk Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 4/8] buffer_io_error: " Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 5/8] ata_dev_printk: " Tony Asleson
     [not found]   ` <CGME20200624103532eucas1p2c0988207e4dfc2f992d309b75deac3ee@eucas1p2.samsung.com>
2020-06-24 10:35     ` Bartlomiej Zolnierkiewicz
2020-06-24 15:15       ` Tony Asleson [this message]
2020-06-26 12:45         ` Bartlomiej Zolnierkiewicz
2020-06-26 13:54           ` Tony Asleson
     [not found]       ` <7ed08b94-755f-baab-0555-b4e454405729@redhat.com>
2020-07-14  8:06         ` Bartlomiej Zolnierkiewicz
2020-07-14  8:17           ` Greg Kroah-Hartman
2020-07-14  8:50             ` Bartlomiej Zolnierkiewicz
2020-07-17 10:06               ` Greg Kroah-Hartman
2020-07-17 10:17                 ` Hannes Reinecke
2020-07-17 10:27                 ` Bartlomiej Zolnierkiewicz
2020-07-17 19:47                   ` Tony Asleson
2020-07-24  8:50                     ` Bartlomiej Zolnierkiewicz
2020-06-23 19:17 ` [RFC PATCH v3 6/8] scsi: Add durable_name for dev_printk Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 7/8] nvme: Add durable name " Tony Asleson
2020-06-23 20:04   ` Chaitanya Kulkarni
2020-06-23 20:32     ` Tony Asleson
2020-06-23 19:17 ` [RFC PATCH v3 8/8] dev_vprintk_emit: Increase hdr size 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=33577b4f-6ee1-f054-8853-b61ca800e10a@redhat.com \
    --to=tasleson@redhat.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-ide@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-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/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-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

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


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