Re: [PATCH] Improve error stats message

2018-03-08 Thread Anand Jain 



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

2018-03-07 Thread Nikolay Borisov 


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

2018-03-07 Thread Hugo Mills 
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

2018-03-07 Thread Diego 
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

2018-03-07 Thread Hugo Mills 
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

2018-03-07 Thread Diego 
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