Re: [PATCH] Improve error stats message
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve error stats message
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 > --- > 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. > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve error stats message
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: > >hi! I noticed bdev rd 13 as a kernel message >what does it mean >Well, that's not the whole message. >Can you paste the whole line in here? (Just one line) ^^ nick2... that would be me. :) >[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 | signature.asc Description: Digital signature
Re: [PATCH] Improve error stats message
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: hi! I noticed bdev rd 13 as a kernel message what does it mean Well, that's not the whole message. Can you paste the whole line in here? (Just one line) [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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve error stats message
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 > --- > 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 | signature.asc Description: Digital signature
[PATCH] 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 Signed-off-by: Diego Calleja --- 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), -- 2.16.2 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html