* Re: [PATCH] xfs: clarify units in the failed metadata io message
2018-01-08 19:40 [PATCH] xfs: clarify units in the failed metadata io message Darrick J. Wong
@ 2018-01-08 20:19 ` Eric Sandeen
2018-01-08 20:32 ` Darrick J. Wong
2018-01-08 21:32 ` [PATCH v2] " Darrick J. Wong
2018-01-08 22:38 ` [PATCH v3] " Darrick J. Wong
2 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2018-01-08 20:19 UTC (permalink / raw)
To: Darrick J. Wong, xfs
On 1/8/18 1:40 PM, Darrick J. Wong wrote:> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> If a metadata IO error happens, we report the location of the failed IO
> request in units of daddrs. However, the printk message misleads people
> into thinking that the units are fs blocks, so fix the reported units.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_buf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1981ef7..582c64a 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1196,7 +1196,7 @@ xfs_buf_ioerror_alert(
> const char *func)
> {
> xfs_alert(bp->b_target->bt_mount,
> -"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
> +"metadata I/O error: daddr 0x%llx (\"%s\") error %d numblks %d",
> (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
Ok, so I might have nodded in approval on IRC a little early; if "block"
was misleading, I think "numblks" is too. So if you change one to
daddr, I wonder if the other should be changed as well to, um ...
"metadata I/O error: daddr 0x%llx (\"%s\") error %d count %d"
? That's a bit vague but at least not misleadingly containing a form of
"blocks" - whatchathink? Maybe "len?" Even "sectors" is a bit overloaded
now. :(
-Eric
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xfs: clarify units in the failed metadata io message
2018-01-08 20:19 ` Eric Sandeen
@ 2018-01-08 20:32 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2018-01-08 20:32 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Mon, Jan 08, 2018 at 02:19:58PM -0600, Eric Sandeen wrote:
> On 1/8/18 1:40 PM, Darrick J. Wong wrote:> From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > If a metadata IO error happens, we report the location of the failed IO
> > request in units of daddrs. However, the printk message misleads people
> > into thinking that the units are fs blocks, so fix the reported units.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/xfs_buf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 1981ef7..582c64a 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1196,7 +1196,7 @@ xfs_buf_ioerror_alert(
> > const char *func)
> > {
> > xfs_alert(bp->b_target->bt_mount,
> > -"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
> > +"metadata I/O error: daddr 0x%llx (\"%s\") error %d numblks %d",
> > (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
> Ok, so I might have nodded in approval on IRC a little early; if "block"
> was misleading, I think "numblks" is too. So if you change one to
> daddr, I wonder if the other should be changed as well to, um ...
>
>
> "metadata I/O error: daddr 0x%llx (\"%s\") error %d count %d"
"metadata I/O error in ("\%s\") at daddr 0x%llx len %d error %d"
also:
"%s: daddr out of range: block 0x%llx, EOFS 0x%llx ", __func__, b_bn, eofs
"%s: no ops on buf for daddr 0x%llx/0x%x", __func__, b_bn, b_length
"Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!"
So I guess it's also not consistent how we print buffer length either?
I'll work on something to clean all this up...
--D
> ? That's a bit vague but at least not misleadingly containing a form of
> "blocks" - whatchathink? Maybe "len?" Even "sectors" is a bit overloaded
> now. :(
>
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] xfs: clarify units in the failed metadata io message
2018-01-08 19:40 [PATCH] xfs: clarify units in the failed metadata io message Darrick J. Wong
2018-01-08 20:19 ` Eric Sandeen
@ 2018-01-08 21:32 ` Darrick J. Wong
2018-01-08 22:08 ` Dave Chinner
2018-01-08 22:38 ` [PATCH v3] " Darrick J. Wong
2 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2018-01-08 21:32 UTC (permalink / raw)
To: xfs; +Cc: Eric Sandeen
From: Darrick J. Wong <darrick.wong@oracle.com>
If a metadata IO error happens, we report the location of the failed IO
request in units of daddrs. However, the printk message misleads people
into thinking that the units are fs blocks, so fix the reported units.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: fix more messages
---
fs/xfs/xfs_buf.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 1981ef7..641b823 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -585,8 +585,8 @@ _xfs_buf_find(
* returning a specific error on buffer lookup failures.
*/
xfs_alert(btp->bt_mount,
- "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
- __func__, cmap.bm_bn, eofs);
+ "%pS: daddr 0x%llx out of range, EOFS 0x%llx",
+ __return_address, cmap.bm_bn, eofs);
WARN_ON(1);
return NULL;
}
@@ -1196,8 +1196,9 @@ xfs_buf_ioerror_alert(
const char *func)
{
xfs_alert(bp->b_target->bt_mount,
-"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
- (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
+"metadata I/O error in %pS (\"%s\") at daddr 0x%llx len %d error %d",
+ __return_address, func, (uint64_t)XFS_BUF_ADDR(bp),
+ bp->b_length, -bp->b_error);
}
int
@@ -1379,7 +1380,7 @@ _xfs_buf_ioapply(
*/
if (xfs_sb_version_hascrc(&mp->m_sb)) {
xfs_warn(mp,
- "%s: no ops on block 0x%llx/0x%x",
+ "%s: no buf ops on daddr 0x%llx len %d",
__func__, bp->b_bn, bp->b_length);
xfs_hex_dump(bp->b_addr,
XFS_CORRUPTION_DUMP_LEN);
@@ -1673,7 +1674,7 @@ xfs_wait_buftarg(
list_del_init(&bp->b_lru);
if (bp->b_flags & XBF_WRITE_FAIL) {
xfs_alert(btp->bt_mount,
-"Corruption Alert: Buffer at block 0x%llx had permanent write failures!",
+"Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
(long long)bp->b_bn);
xfs_alert(btp->bt_mount,
"Please run xfs_repair to determine the extent of the problem.");
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] xfs: clarify units in the failed metadata io message
2018-01-08 21:32 ` [PATCH v2] " Darrick J. Wong
@ 2018-01-08 22:08 ` Dave Chinner
2018-01-08 22:20 ` Darrick J. Wong
0 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2018-01-08 22:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs, Eric Sandeen
On Mon, Jan 08, 2018 at 01:32:35PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> If a metadata IO error happens, we report the location of the failed IO
> request in units of daddrs. However, the printk message misleads people
> into thinking that the units are fs blocks, so fix the reported units.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: fix more messages
A useful cleanup...
> ---
> fs/xfs/xfs_buf.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1981ef7..641b823 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -585,8 +585,8 @@ _xfs_buf_find(
> * returning a specific error on buffer lookup failures.
> */
> xfs_alert(btp->bt_mount,
> - "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
> - __func__, cmap.bm_bn, eofs);
> + "%pS: daddr 0x%llx out of range, EOFS 0x%llx",
> + __return_address, cmap.bm_bn, eofs);
That return address won't tell us anything useful - it'll almost
always be xfs_buf_get_map(). That's why I used __func__ originally
here, and the code hasn't really changed since I did that....
> WARN_ON(1);
> return NULL;
> }
> @@ -1196,8 +1196,9 @@ xfs_buf_ioerror_alert(
> const char *func)
> {
> xfs_alert(bp->b_target->bt_mount,
> -"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
> - (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
> +"metadata I/O error in %pS (\"%s\") at daddr 0x%llx len %d error %d",
> + __return_address, func, (uint64_t)XFS_BUF_ADDR(bp),
> + bp->b_length, -bp->b_error);
> }
Same here - the return address is basically going to tell us exactly
the same information as the func parameter that is passed in.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] xfs: clarify units in the failed metadata io message
2018-01-08 22:08 ` Dave Chinner
@ 2018-01-08 22:20 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2018-01-08 22:20 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs, Eric Sandeen
On Tue, Jan 09, 2018 at 09:08:17AM +1100, Dave Chinner wrote:
> On Mon, Jan 08, 2018 at 01:32:35PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > If a metadata IO error happens, we report the location of the failed IO
> > request in units of daddrs. However, the printk message misleads people
> > into thinking that the units are fs blocks, so fix the reported units.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: fix more messages
>
> A useful cleanup...
>
> > ---
> > fs/xfs/xfs_buf.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 1981ef7..641b823 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -585,8 +585,8 @@ _xfs_buf_find(
> > * returning a specific error on buffer lookup failures.
> > */
> > xfs_alert(btp->bt_mount,
> > - "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
> > - __func__, cmap.bm_bn, eofs);
> > + "%pS: daddr 0x%llx out of range, EOFS 0x%llx",
> > + __return_address, cmap.bm_bn, eofs);
>
> That return address won't tell us anything useful - it'll almost
> always be xfs_buf_get_map(). That's why I used __func__ originally
> here, and the code hasn't really changed since I did that....
<nod>
> > WARN_ON(1);
> > return NULL;
> > }
> > @@ -1196,8 +1196,9 @@ xfs_buf_ioerror_alert(
> > const char *func)
> > {
> > xfs_alert(bp->b_target->bt_mount,
> > -"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
> > - (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
> > +"metadata I/O error in %pS (\"%s\") at daddr 0x%llx len %d error %d",
> > + __return_address, func, (uint64_t)XFS_BUF_ADDR(bp),
> > + bp->b_length, -bp->b_error);
> > }
>
> Same here - the return address is basically going to tell us exactly
> the same information as the func parameter that is passed in.
I was making a follow-on patch that removes the func argument entirely,
but eh, maybe I'll stick to strictly cleaning up units and reporting
formats here.
--D
> Cheers,
>
> Dave.
>
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] xfs: clarify units in the failed metadata io message
2018-01-08 19:40 [PATCH] xfs: clarify units in the failed metadata io message Darrick J. Wong
2018-01-08 20:19 ` Eric Sandeen
2018-01-08 21:32 ` [PATCH v2] " Darrick J. Wong
@ 2018-01-08 22:38 ` Darrick J. Wong
2018-01-08 22:57 ` Dave Chinner
2 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2018-01-08 22:38 UTC (permalink / raw)
To: xfs; +Cc: Eric Sandeen, Dave Chinner
From: Darrick J. Wong <darrick.wong@oracle.com>
If a metadata IO error happens, we report the location of the failed IO
request in units of daddrs. However, the printk message misleads people
into thinking that the units are fs blocks, so fix the reported units.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: fix only the units reporting, don't print the caller
v2: fix more messages
---
fs/xfs/xfs_buf.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 1981ef7..0820c1c 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -585,7 +585,7 @@ _xfs_buf_find(
* returning a specific error on buffer lookup failures.
*/
xfs_alert(btp->bt_mount,
- "%s: Block out of range: block 0x%llx, EOFS 0x%llx ",
+ "%s: daddr 0x%llx out of range, EOFS 0x%llx",
__func__, cmap.bm_bn, eofs);
WARN_ON(1);
return NULL;
@@ -1196,8 +1196,9 @@ xfs_buf_ioerror_alert(
const char *func)
{
xfs_alert(bp->b_target->bt_mount,
-"metadata I/O error: block 0x%llx (\"%s\") error %d numblks %d",
- (uint64_t)XFS_BUF_ADDR(bp), func, -bp->b_error, bp->b_length);
+"metadata I/O error in \"%s\" at daddr 0x%llx len %d error %d",
+ func, (uint64_t)XFS_BUF_ADDR(bp), bp->b_length,
+ -bp->b_error);
}
int
@@ -1379,7 +1380,7 @@ _xfs_buf_ioapply(
*/
if (xfs_sb_version_hascrc(&mp->m_sb)) {
xfs_warn(mp,
- "%s: no ops on block 0x%llx/0x%x",
+ "%s: no buf ops on daddr 0x%llx len %d",
__func__, bp->b_bn, bp->b_length);
xfs_hex_dump(bp->b_addr,
XFS_CORRUPTION_DUMP_LEN);
@@ -1673,7 +1674,7 @@ xfs_wait_buftarg(
list_del_init(&bp->b_lru);
if (bp->b_flags & XBF_WRITE_FAIL) {
xfs_alert(btp->bt_mount,
-"Corruption Alert: Buffer at block 0x%llx had permanent write failures!",
+"Corruption Alert: Buffer at daddr 0x%llx had permanent write failures!",
(long long)bp->b_bn);
xfs_alert(btp->bt_mount,
"Please run xfs_repair to determine the extent of the problem.");
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3] xfs: clarify units in the failed metadata io message
2018-01-08 22:38 ` [PATCH v3] " Darrick J. Wong
@ 2018-01-08 22:57 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2018-01-08 22:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xfs, Eric Sandeen
On Mon, Jan 08, 2018 at 02:38:45PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> If a metadata IO error happens, we report the location of the failed IO
> request in units of daddrs. However, the printk message misleads people
> into thinking that the units are fs blocks, so fix the reported units.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: fix only the units reporting, don't print the caller
> v2: fix more messages
Looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread