| Message ID | 156944738604.300131.8278382614496808681.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_scrub: fix IO error reporting | expand |
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); > } > /* >
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); > > } > > > > /* > >
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); } /*