[03/11] xfs_scrub: better reporting of metadata media errors
diff mbox series

Message ID 156944738604.300131.8278382614496808681.stgit@magnolia
State Superseded
Headers show
Series
  • xfs_scrub: fix IO error reporting
Related show

Commit Message

Darrick J. Wong Sept. 25, 2019, 9:36 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

When we report bad metadata, we inexplicably report the physical address
in units of sectors, whereas for file data we report file offsets in
units of bytes.  Fix the metadata reporting units to match the file data
units (i.e. bytes) and skip the printf for all other cases.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/phase6.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Eric Sandeen Oct. 21, 2019, 4:46 p.m. UTC | #1
On 9/25/19 4:36 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we report bad metadata, we inexplicably report the physical address
> in units of sectors, whereas for file data we report file offsets in
> units of bytes.  Fix the metadata reporting units to match the file data
> units (i.e. bytes) and skip the printf for all other cases.

kernelspace has been plagued with stuff like this - sectors vs bytes vs
blocks, hex vs decimal, leading 0x or not ... A doc in xfs_scrub about
how everything should be reported seems like it might be a good idea?

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  scrub/phase6.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index a16ad114..310ab36c 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -368,7 +368,7 @@ xfs_check_rmap_error_report(
>  	void			*arg)
>  {
>  	const char		*type;
> -	char			buf[32];
> +	char			buf[DESCR_BUFSZ];
>  	uint64_t		err_physical = *(uint64_t *)arg;
>  	uint64_t		err_off;
>  
> @@ -377,14 +377,12 @@ xfs_check_rmap_error_report(
>  	else
>  		err_off = 0;
>  
> -	snprintf(buf, 32, _("disk offset %"PRIu64),
> -			(uint64_t)BTOBB(map->fmr_physical + err_off));
> -

so did this double-report if the error was associated with a file?

> +	/* Report special owners */
>  	if (map->fmr_flags & FMR_OF_SPECIAL_OWNER) {
> +		snprintf(buf, DESCR_BUFSZ, _("disk offset %"PRIu64),
> +				(uint64_t)map->fmr_physical + err_off);
>  		type = xfs_decode_special_owner(map->fmr_owner);
> -		str_error(ctx, buf,
> -_("%s failed read verification."),
> -				type);
> +		str_error(ctx, buf, _("media error in %s."), type);
>  	}

 
>  	/*
>
Darrick J. Wong Oct. 21, 2019, 6:10 p.m. UTC | #2
On Mon, Oct 21, 2019 at 11:46:23AM -0500, Eric Sandeen wrote:
> On 9/25/19 4:36 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we report bad metadata, we inexplicably report the physical address
> > in units of sectors, whereas for file data we report file offsets in
> > units of bytes.  Fix the metadata reporting units to match the file data
> > units (i.e. bytes) and skip the printf for all other cases.
> 
> kernelspace has been plagued with stuff like this - sectors vs bytes vs
> blocks, hex vs decimal, leading 0x or not ... A doc in xfs_scrub about
> how everything should be reported seems like it might be a good idea?

I'll see what I can come up with...

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  scrub/phase6.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > 
> > diff --git a/scrub/phase6.c b/scrub/phase6.c
> > index a16ad114..310ab36c 100644
> > --- a/scrub/phase6.c
> > +++ b/scrub/phase6.c
> > @@ -368,7 +368,7 @@ xfs_check_rmap_error_report(
> >  	void			*arg)
> >  {
> >  	const char		*type;
> > -	char			buf[32];
> > +	char			buf[DESCR_BUFSZ];
> >  	uint64_t		err_physical = *(uint64_t *)arg;
> >  	uint64_t		err_off;
> >  
> > @@ -377,14 +377,12 @@ xfs_check_rmap_error_report(
> >  	else
> >  		err_off = 0;
> >  
> > -	snprintf(buf, 32, _("disk offset %"PRIu64),
> > -			(uint64_t)BTOBB(map->fmr_physical + err_off));
> > -
> 
> so did this double-report if the error was associated with a file?

This snprintf call formats the error message prefix for the case where
getfsmap tells us that a bad range intersects with some metadata...

> > +	/* Report special owners */
> >  	if (map->fmr_flags & FMR_OF_SPECIAL_OWNER) {
> > +		snprintf(buf, DESCR_BUFSZ, _("disk offset %"PRIu64),
> > +				(uint64_t)map->fmr_physical + err_off);

...so we move it here to avoid wasting time on string formatting for
other badblocks cases such as the one we add in the next patch.

--D

> >  		type = xfs_decode_special_owner(map->fmr_owner);
> > -		str_error(ctx, buf,
> > -_("%s failed read verification."),
> > -				type);
> > +		str_error(ctx, buf, _("media error in %s."), type);
> >  	}
> 
>  
> >  	/*
> >

Patch
diff mbox series

diff --git a/scrub/phase6.c b/scrub/phase6.c
index a16ad114..310ab36c 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -368,7 +368,7 @@  xfs_check_rmap_error_report(
 	void			*arg)
 {
 	const char		*type;
-	char			buf[32];
+	char			buf[DESCR_BUFSZ];
 	uint64_t		err_physical = *(uint64_t *)arg;
 	uint64_t		err_off;
 
@@ -377,14 +377,12 @@  xfs_check_rmap_error_report(
 	else
 		err_off = 0;
 
-	snprintf(buf, 32, _("disk offset %"PRIu64),
-			(uint64_t)BTOBB(map->fmr_physical + err_off));
-
+	/* Report special owners */
 	if (map->fmr_flags & FMR_OF_SPECIAL_OWNER) {
+		snprintf(buf, DESCR_BUFSZ, _("disk offset %"PRIu64),
+				(uint64_t)map->fmr_physical + err_off);
 		type = xfs_decode_special_owner(map->fmr_owner);
-		str_error(ctx, buf,
-_("%s failed read verification."),
-				type);
+		str_error(ctx, buf, _("media error in %s."), type);
 	}
 
 	/*