* Re: [PATCH] Improve error stats message
2018-03-07 17:37 [PATCH] Improve error stats message Diego
@ 2018-03-07 18:24 ` Hugo Mills
2018-03-07 19:02 ` Diego
2018-03-08 7:28 ` Nikolay Borisov
2018-03-08 10:04 ` Anand Jain
2 siblings, 1 reply; 6+ messages in thread
From: Hugo Mills @ 2018-03-07 18:24 UTC (permalink / raw)
To: Diego; +Cc: linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]
On Wed, Mar 07, 2018 at 06:37:29PM +0100, Diego wrote:
> A typical notification of filesystem errors looks like this:
>
> BTRFS error (device sda2): bdev /dev/sda2 errs: wr 0, rd 1, flush 0, corrupt 0, gen 0
>
> The device name is being printed twice.
For good reason -- the first part ("device sda2") indicates the
filesystem, and is the arbitrarily-selected device used by the kernel
to represent the FS. The second part ("bdev /dev/sda2") indicates the
_actual_ device for which the errors are being reported.
On multi-device filesystems, the two are not necessarily the same.
Hugo.
> Also, these abbreviatures
> feel unnecesary. Make the message look like this instead:
>
> BTRFS error (device sda2): errors: write 0, read 1, flush 0, corrupt 0, generation 0
>
>
> Signed-off-by: Diego Calleja <diegocg@gmail.com>
> ---
> fs/btrfs/volumes.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2ceb924ca0d6..52fee5bb056f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7238,9 +7238,8 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
> {
> if (!dev->dev_stats_valid)
> return;
> - btrfs_err_rl_in_rcu(dev->fs_info,
> - "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
> - rcu_str_deref(dev->name),
> + btrfs_err_rl(dev->fs_info,
> + "errors: write %u, read %u, flush %u, corrupt %u, generation %u",
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
--
Hugo Mills | Would you like an ocelot with that non-sequitur?
hugo@... carfax.org.uk |
http://carfax.org.uk/ |
PGP: E2AB1DE4 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Improve error stats message
2018-03-07 18:24 ` Hugo Mills
@ 2018-03-07 19:02 ` Diego
2018-03-07 20:35 ` Hugo Mills
0 siblings, 1 reply; 6+ messages in thread
From: Diego @ 2018-03-07 19:02 UTC (permalink / raw)
To: Hugo Mills; +Cc: linux-btrfs
El miércoles, 7 de marzo de 2018 19:24:53 (CET) Hugo Mills escribió:
> On multi-device filesystems, the two are not necessarily the same.
Ouch. FWIW, I was moved to do this because I saw this conversation on
IRC which made me think that people aren't understanding what the
message means:
<nick1> hi! I noticed bdev rd 13 as a kernel message
<nick1> what does it mean
<nick2> Well, that's not the whole message.
<nick2> Can you paste the whole line in here? (Just one line)
<nick1> [ 3.404959] BTRFS info (device sda4): bdev /dev/sda4 errs: wr 0, rd 13, flush 0, corrupt 0, gen 0
Maybe something like this would be better:
BTRFS info (device sda4): disk /dev/sda4 errors: write 0, read 13, flush 0, corrupt 0, generation 0
---
fs/btrfs/volumes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2ceb924ca0d6..cfa029468585 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7239,7 +7239,7 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
if (!dev->dev_stats_valid)
return;
btrfs_err_rl_in_rcu(dev->fs_info,
- "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
+ "disk %s errors: write %u, read %u, flush %u, corrupt %u, generation %u",
rcu_str_deref(dev->name),
btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
--
2.16.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Improve error stats message
2018-03-07 19:02 ` Diego
@ 2018-03-07 20:35 ` Hugo Mills
0 siblings, 0 replies; 6+ messages in thread
From: Hugo Mills @ 2018-03-07 20:35 UTC (permalink / raw)
To: Diego; +Cc: linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]
On Wed, Mar 07, 2018 at 08:02:51PM +0100, Diego wrote:
> El miércoles, 7 de marzo de 2018 19:24:53 (CET) Hugo Mills escribió:
> > On multi-device filesystems, the two are not necessarily the same.
>
> Ouch. FWIW, I was moved to do this because I saw this conversation on
> IRC which made me think that people aren't understanding what the
> message means:
>
> <nick1> hi! I noticed bdev rd 13 as a kernel message
> <nick1> what does it mean
> <nick2> Well, that's not the whole message.
> <nick2> Can you paste the whole line in here? (Just one line)
^^ nick2... that would be me. :)
> <nick1> [ 3.404959] BTRFS info (device sda4): bdev /dev/sda4 errs: wr 0, rd 13, flush 0, corrupt 0, gen 0
>
>
> Maybe something like this would be better:
>
> BTRFS info (device sda4): disk /dev/sda4 errors: write 0, read 13, flush 0, corrupt 0, generation 0
I think the single most helpful modification here would be to
change "device" to "fs on", to show that it's only an indicator of the
filesystem ID, rather than actually the device on which the errors
occurred. The others I'm not really bothered about, personally.
Hugo.
> ---
> fs/btrfs/volumes.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2ceb924ca0d6..cfa029468585 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7239,7 +7239,7 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
> if (!dev->dev_stats_valid)
> return;
> btrfs_err_rl_in_rcu(dev->fs_info,
> - "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
> + "disk %s errors: write %u, read %u, flush %u, corrupt %u, generation %u",
> rcu_str_deref(dev->name),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
--
Hugo Mills | Q: What goes, "Pieces of seven! Pieces of seven!"?
hugo@... carfax.org.uk | A: A parroty error.
http://carfax.org.uk/ |
PGP: E2AB1DE4 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Improve error stats message
2018-03-07 17:37 [PATCH] Improve error stats message Diego
2018-03-07 18:24 ` Hugo Mills
@ 2018-03-08 7:28 ` Nikolay Borisov
2018-03-08 10:04 ` Anand Jain
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-03-08 7:28 UTC (permalink / raw)
To: Diego, linux-btrfs
On 7.03.2018 19:37, Diego wrote:
> A typical notification of filesystem errors looks like this:
>
> BTRFS error (device sda2): bdev /dev/sda2 errs: wr 0, rd 1, flush 0, corrupt 0, gen 0
>
> The device name is being printed twice. Also, these abbreviatures
> feel unnecesary. Make the message look like this instead:
>
> BTRFS error (device sda2): errors: write 0, read 1, flush 0, corrupt 0, generation 0
>
>
> Signed-off-by: Diego Calleja <diegocg@gmail.com>
> ---
> fs/btrfs/volumes.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2ceb924ca0d6..52fee5bb056f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7238,9 +7238,8 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
> {
> if (!dev->dev_stats_valid)
> return;
> - btrfs_err_rl_in_rcu(dev->fs_info,
> - "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
> - rcu_str_deref(dev->name),
> + btrfs_err_rl(dev->fs_info,
> + "errors: write %u, read %u, flush %u, corrupt %u, generation %u",
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
I think what would be better is to expose the btrfs_dev_name functino in
a header file and instead of open-coding rcu_str_deref use that function
instead. Also I agree that write/read/ are better than wr/rd.
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Improve error stats message
2018-03-07 17:37 [PATCH] Improve error stats message Diego
2018-03-07 18:24 ` Hugo Mills
2018-03-08 7:28 ` Nikolay Borisov
@ 2018-03-08 10:04 ` Anand Jain
2 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2018-03-08 10:04 UTC (permalink / raw)
To: Diego, linux-btrfs
On 03/08/2018 01:37 AM, Diego wrote:
> A typical notification of filesystem errors looks like this:
>
> BTRFS error (device sda2): bdev /dev/sda2 errs: wr 0, rd 1, flush 0, corrupt 0, gen 0
>
> The device name is being printed twice. Also, these abbreviatures
> feel unnecesary. Make the message look like this instead:
>
> BTRFS error (device sda2): errors: write 0, read 1, flush 0, corrupt 0, generation 0
>
>
> Signed-off-by: Diego Calleja <diegocg@gmail.com>
> ---
> fs/btrfs/volumes.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2ceb924ca0d6..52fee5bb056f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7238,9 +7238,8 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
> {
> if (!dev->dev_stats_valid)
> return;
> - btrfs_err_rl_in_rcu(dev->fs_info,
> - "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
> - rcu_str_deref(dev->name),
> + btrfs_err_rl(dev->fs_info,
> + "errors: write %u, read %u, flush %u, corrupt %u, generation %u",
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
>
As btrfs_err_rl()->btrfs_printk() does its magic to print the
latest_bdev, this will print the wrong device for the error
reporting in case of multiple device FS.
There is this RFC in the ML, to use FSID insted. Comments are
welcome.
[RFC PATCH] Btrfs: fix fs logging for multi device
Thanks, Anand
^ permalink raw reply [flat|nested] 6+ messages in thread