Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] buffer_io_error: Use dev_err_ratelimited
@ 2020-10-26 15:57 Tony Asleson
  2020-10-26 22:07 ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Asleson @ 2020-10-26 15:57 UTC (permalink / raw)
  To: viro, linux-fsdevel, linux-kernel

Replace printk_ratelimited with dev_err_ratelimited which
adds dev_printk meta data. This is used by journald to
add disk ID information to the journal entry.

This re-worked change is from a different patch series
and utilizes the following suggestions.

- Reduce indentation level (Andy Shevchenko)
- Remove unneeded () for conditional operator (Sergei Shtylyov)

Signed-off-by: Tony Asleson <tasleson@redhat.com>
---
 fs/buffer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 50bbc99e3d96..18175fbb1101 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -125,10 +125,17 @@ EXPORT_SYMBOL(__wait_on_buffer);
 
 static void buffer_io_error(struct buffer_head *bh, char *msg)
 {
-	if (!test_bit(BH_Quiet, &bh->b_state))
-		printk_ratelimited(KERN_ERR
-			"Buffer I/O error on dev %pg, logical block %llu%s\n",
-			bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
+	struct device *gendev;
+
+	if (test_bit(BH_Quiet, &bh->b_state))
+		return;
+
+	gendev = bh->b_bdev->bd_disk ?
+		disk_to_dev(bh->b_bdev->bd_disk) : NULL;
+
+	dev_err_ratelimited(gendev,
+		"Buffer I/O error, logical block %llu%s\n",
+		(unsigned long long)bh->b_blocknr, msg);
 }
 
 /*

base-commit: bbf5c979011a099af5dc76498918ed7df445635b
-- 
2.26.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] buffer_io_error: Use dev_err_ratelimited
  2020-10-26 15:57 [PATCH] buffer_io_error: Use dev_err_ratelimited Tony Asleson
@ 2020-10-26 22:07 ` Andy Shevchenko
  2020-10-28 20:45   ` Tony Asleson
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-26 22:07 UTC (permalink / raw)
  To: Tony Asleson; +Cc: Alexander Viro, Linux FS Devel, Linux Kernel Mailing List

On Mon, Oct 26, 2020 at 10:59 PM Tony Asleson <tasleson@redhat.com> wrote:
>
> Replace printk_ratelimited with dev_err_ratelimited which
> adds dev_printk meta data. This is used by journald to
> add disk ID information to the journal entry.


> This re-worked change is from a different patch series
> and utilizes the following suggestions.
>
> - Reduce indentation level (Andy Shevchenko)
> - Remove unneeded () for conditional operator (Sergei Shtylyov)

This should go as a changelog after the cutter '---' line...

> Signed-off-by: Tony Asleson <tasleson@redhat.com>
> ---

...somewhere here.

...

> -       if (!test_bit(BH_Quiet, &bh->b_state))
> -               printk_ratelimited(KERN_ERR
> -                       "Buffer I/O error on dev %pg, logical block %llu%s\n",
> -                       bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
> +       struct device *gendev;
> +
> +       if (test_bit(BH_Quiet, &bh->b_state))
> +               return;
> +

> +       gendev = bh->b_bdev->bd_disk ?
> +               disk_to_dev(bh->b_bdev->bd_disk) : NULL;

I'm not sure it's a good idea to print '(null)'.

Perhaps

if (bh->b_bdev->bd_disk)
  dev_err_ratelimit(disk_to_dev(bh->b_bdev->bd_disk), ...);
else
  pr_err_ratelimit(...);

?

> +       dev_err_ratelimited(gendev,
> +               "Buffer I/O error, logical block %llu%s\n",

> +               (unsigned long long)bh->b_blocknr, msg);

It's a u64 always (via sector_t), do we really need a casting?

>  }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] buffer_io_error: Use dev_err_ratelimited
  2020-10-26 22:07 ` Andy Shevchenko
@ 2020-10-28 20:45   ` Tony Asleson
  2020-10-28 21:05     ` Tony Asleson
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Asleson @ 2020-10-28 20:45 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Alexander Viro, Linux FS Devel, Linux Kernel Mailing List

On 10/26/20 5:07 PM, Andy Shevchenko wrote:
> On Mon, Oct 26, 2020 at 10:59 PM Tony Asleson <tasleson@redhat.com> wrote:
>>
>> Replace printk_ratelimited with dev_err_ratelimited which
>> adds dev_printk meta data. This is used by journald to
>> add disk ID information to the journal entry.
> 
> 
>> This re-worked change is from a different patch series
>> and utilizes the following suggestions.
>>
>> - Reduce indentation level (Andy Shevchenko)
>> - Remove unneeded () for conditional operator (Sergei Shtylyov)
> 
> This should go as a changelog after the cutter '---' line...

Thanks, I'll correct this.


>> Signed-off-by: Tony Asleson <tasleson@redhat.com>
>> ---
> 
> ...somewhere here.
> 
> ...
> 
>> -       if (!test_bit(BH_Quiet, &bh->b_state))
>> -               printk_ratelimited(KERN_ERR
>> -                       "Buffer I/O error on dev %pg, logical block %llu%s\n",
>> -                       bh->b_bdev, (unsigned long long)bh->b_blocknr, msg);
>> +       struct device *gendev;
>> +
>> +       if (test_bit(BH_Quiet, &bh->b_state))
>> +               return;
>> +
> 
>> +       gendev = bh->b_bdev->bd_disk ?
>> +               disk_to_dev(bh->b_bdev->bd_disk) : NULL;
> 
> I'm not sure it's a good idea to print '(null)'.

I've not seen any cases where we end up with null here, but I've only
tested ATA, SCSI, and NVMe subsystems.

However, I would think that if this does occur it would be more obvious
that it's an issue that needs to be corrected if the null ends up in the
logs instead of having the same output that we have today?

> Perhaps
> 
> if (bh->b_bdev->bd_disk)
>   dev_err_ratelimit(disk_to_dev(bh->b_bdev->bd_disk), ...);
> else
>   pr_err_ratelimit(...);
> 
> ?
> 
>> +       dev_err_ratelimited(gendev,
>> +               "Buffer I/O error, logical block %llu%s\n",
> 
>> +               (unsigned long long)bh->b_blocknr, msg);
> 
> It's a u64 always (via sector_t), do we really need a casting?

That's a good question, grepping around shows *many* instances of this
being done.  I do agree that this doesn't seem to be needed, but maybe
there is a reason why it's done?

-Tony


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] buffer_io_error: Use dev_err_ratelimited
  2020-10-28 20:45   ` Tony Asleson
@ 2020-10-28 21:05     ` Tony Asleson
  2020-10-28 23:22       ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Tony Asleson @ 2020-10-28 21:05 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Alexander Viro, Linux FS Devel, Linux Kernel Mailing List

On 10/28/20 3:45 PM, Tony Asleson wrote:
> On 10/26/20 5:07 PM, Andy Shevchenko wrote:

>>> +       dev_err_ratelimited(gendev,
>>> +               "Buffer I/O error, logical block %llu%s\n",
>>
>>> +               (unsigned long long)bh->b_blocknr, msg);
>>
>> It's a u64 always (via sector_t), do we really need a casting?
> 
> That's a good question, grepping around shows *many* instances of this
> being done.  I do agree that this doesn't seem to be needed, but maybe
> there is a reason why it's done?

According to this:

https://www.kernel.org/doc/html/v5.9/core-api/printk-formats.html

This should be left as it is, because 'sector_t' is dependent on a
config option.

-Tony


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] buffer_io_error: Use dev_err_ratelimited
  2020-10-28 21:05     ` Tony Asleson
@ 2020-10-28 23:22       ` Andy Shevchenko
  2020-11-17 20:38         ` Tony Asleson
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-28 23:22 UTC (permalink / raw)
  To: Tony Asleson, Christoph Hellwig, Jonathan Corbet
  Cc: Alexander Viro, Linux FS Devel, Linux Kernel Mailing List

On Wed, Oct 28, 2020 at 11:05 PM Tony Asleson <tasleson@redhat.com> wrote:
> On 10/28/20 3:45 PM, Tony Asleson wrote:
> > On 10/26/20 5:07 PM, Andy Shevchenko wrote:
>
> >>> +       dev_err_ratelimited(gendev,
> >>> +               "Buffer I/O error, logical block %llu%s\n",
> >>
> >>> +               (unsigned long long)bh->b_blocknr, msg);
> >>
> >> It's a u64 always (via sector_t), do we really need a casting?
> >
> > That's a good question, grepping around shows *many* instances of this
> > being done.  I do agree that this doesn't seem to be needed, but maybe
> > there is a reason why it's done?
>
> According to this:
>
> https://www.kernel.org/doc/html/v5.9/core-api/printk-formats.html
>
> This should be left as it is, because 'sector_t' is dependent on a
> config option.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/types.h?id=72deb455b5ec619ff043c30bc90025aa3de3cdda

Staled documentation. You may send a patch to fix it (I Cc'ed
Christoph and Jonathan).
It means that it doesn't go under this category and the example should
be changed to something else.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] buffer_io_error: Use dev_err_ratelimited
  2020-10-28 23:22       ` Andy Shevchenko
@ 2020-11-17 20:38         ` Tony Asleson
  0 siblings, 0 replies; 6+ messages in thread
From: Tony Asleson @ 2020-11-17 20:38 UTC (permalink / raw)
  To: Andy Shevchenko, Christoph Hellwig, Jonathan Corbet
  Cc: Alexander Viro, Linux FS Devel, Linux Kernel Mailing List

On 10/28/20 6:22 PM, Andy Shevchenko wrote:
> Staled documentation. You may send a patch to fix it (I Cc'ed
> Christoph and Jonathan).
> It means that it doesn't go under this category and the example should
> be changed to something else.

I'm looking into a suitable replacement example.  Will post
documentation patch when I find one.

Thanks,
Tony


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 15:57 [PATCH] buffer_io_error: Use dev_err_ratelimited Tony Asleson
2020-10-26 22:07 ` Andy Shevchenko
2020-10-28 20:45   ` Tony Asleson
2020-10-28 21:05     ` Tony Asleson
2020-10-28 23:22       ` Andy Shevchenko
2020-11-17 20:38         ` Tony Asleson

Linux-Fsdevel Archive on lore.kernel.org

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

Example config snippet for mirrors

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


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