diff mbox

Improve error stats message

Message ID 1588327.cr4xlG2VNd@archlinux (mailing list archive)
State New, archived
Headers show

Commit Message

Diego Calleja March 7, 2018, 5:37 p.m. UTC
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(-)

Comments

Hugo Mills March 7, 2018, 6:24 p.m. UTC | #1
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),
Nikolay Borisov March 8, 2018, 7:28 a.m. UTC | #2
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.

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anand Jain March 8, 2018, 10:04 a.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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),