Improve error stats message
PATCH 1/1 Improve error stats message
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
--- fs/btrfs/volumes.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
fs/btrfs/volumes.c +2 -3
@@ -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),
Signature · 1 line
2.16.2
Quoted 5 lines · Diego
A typical notification of filesystem errors looks like this:
BTRFS error (device sda2): bdev
/dev/sda2errs: wr 0, rd 1, flush 0, corrupt 0, gen 0The 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.
Quoted 27 lines
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 <[email protected]>--- fs/btrfs/volumes.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)fs/btrfs/volumes.c +2
-3@@ -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),
Signature · 4 lines
Hugo Mills | Would you like an ocelot with that non-sequitur? hugo@... carfax.org.uk | http://carfax.org.uk/ | PGP: E2AB1DE4 |
Quoted 1 line · 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(-)
fs/btrfs/volumes.c +1 -1
@@ -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),
Signature · 1 line
2.16.2
Quoted 11 lines · Diego
Quoted 1 line · 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. :)
Quoted 6 lines
<nick1> [ 3.404959] BTRFS info (device sda4): bdev
/dev/sda4errs: wr 0, rd 13, flush 0, corrupt 0, gen 0Maybe something like this would be better:
BTRFS info (device sda4): disk
/dev/sda4errors: 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.
Quoted 17 lines
--- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/volumes.c +1
-1@@ -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),
Signature · 4 lines
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 |
Quoted 21 lines · Diego
A typical notification of filesystem errors looks like this:
BTRFS error (device sda2): bdev
/dev/sda2errs: wr 0, rd 1, flush 0, corrupt 0, gen 0The 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 <[email protected]>--- fs/btrfs/volumes.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)fs/btrfs/volumes.c +0
-0@@ -7238,9 +7238,8 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev) {
fs/btrfs/volumes.c static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev) · line 7245
Quoted 10 lines
fs/btrfs/volumes.c +2
-3if (!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.
Quoted 1 line
Quoted 22 lines · Diego
A typical notification of filesystem errors looks like this:
BTRFS error (device sda2): bdev
/dev/sda2errs: wr 0, rd 1, flush 0, corrupt 0, gen 0The 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 <[email protected]>--- fs/btrfs/volumes.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)fs/btrfs/volumes.c +0
-0@@ -7238,9 +7238,8 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev) {
fs/btrfs/volumes.c static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev) · line 7245
Quoted 10 lines
fs/btrfs/volumes.c +2
-3if (!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