xfs: clarify units in the failed metadata io message

PATCH Superseded Darrick J. Wong 3 participants 8 messages Jan 8, 2018 XFS filesystem

lore

Share & tools

lore

Darrick J. Wong

author

2018-01-08 19:40 UTC #0 lore raw

PATCH 1/1 xfs: clarify units in the failed metadata io message

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

---
 fs/xfs/xfs_buf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_buf.c +1 -1

index 1981ef7..582c64a 100644
@@ -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);
 }

Eric Sandeen

2018-01-08 20:20 UTC #1 lore raw

On 1/8/18 1:40 PM, Darrick J. Wong wrote:> From: Darrick J. Wong <[email protected]>

Quoted 15 lines

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 <[email protected]>
---
 fs/xfs/xfs_buf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_buf.c +0 -0
index 1981ef7..582c64a 100644

fs/xfs/xfs_buf.c xfs_buf_ioerror_alert( · line 1200

Quoted 6 lines

fs/xfs/xfs_buf.c +1 -1

@@ -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

Darrick J. Wong

author

2018-01-08 20:32 UTC #2 lore raw

Quoted 27 lines · Eric Sandeen

On 1/8/18 1:40 PM, Darrick J. Wong wrote:> From: Darrick J. Wong <[email protected]>
Quoted 15 lines
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 <[email protected]>
---
 fs/xfs/xfs_buf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_buf.c +0 -0
index 1981ef7..582c64a 100644
fs/xfs/xfs_buf.c xfs_buf_ioerror_alert( · line 1200
Quoted 6 lines
fs/xfs/xfs_buf.c +1 -1
@@ -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 ...

Quoted 1 line

"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

Quoted 9 lines

? 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

author

2018-01-08 21:32 UTC #3 lore raw

PATCH v2 1/1 xfs: clarify units in the failed metadata io message

In reply to Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
v2: fix more messages

---
 fs/xfs/xfs_buf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/xfs/xfs_buf.c +7 -6

index 1981ef7..641b823 100644
@@ -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.");

Dave Chinner

2018-01-08 22:08 UTC #4 lore raw

Quoted 9 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
v2: fix more messages

A useful cleanup...

Quoted 9 lines

---
 fs/xfs/xfs_buf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/xfs/xfs_buf.c +0 -0

index 1981ef7..641b823 100644

fs/xfs/xfs_buf.c _xfs_buf_find( · line 589

Quoted 7 lines

fs/xfs/xfs_buf.c +2 -2

@@ -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....

fs/xfs/xfs_buf.c xfs_buf_ioerror_alert( · line 1202

Quoted 13 lines

fs/xfs/xfs_buf.c +3 -2

 		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.

Signature · 2 lines

Dave Chinner
[email protected]

Darrick J. Wong

author

2018-01-08 22:20 UTC #5 lore raw

Quoted 17 lines · Dave Chinner

Quoted 9 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
v2: fix more messages
A useful cleanup...
Quoted 4 lines
---
 fs/xfs/xfs_buf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Quoted 16 lines

fs/xfs/xfs_buf.c _xfs_buf_find( · line 589
Quoted 12 lines
fs/xfs/xfs_buf.c +2 -2
index 1981ef7..641b823 100644
@@ -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>

Quoted 8 lines

fs/xfs/xfs_buf.c +0 -1

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

Quoted 8 lines

fs/xfs/xfs_buf.c xfs_buf_ioerror_alert( · line 1202
Quoted 5 lines
fs/xfs/xfs_buf.c +3 -1
-		(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

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

author

2018-01-08 22:42 UTC #6 lore raw

PATCH v3 1/1 xfs: clarify units in the failed metadata io message

In reply to Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

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

fs/xfs/xfs_buf.c +6 -5

index 1981ef7..0820c1c 100644
@@ -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.");

Dave Chinner

Reviewed-by on v3 1/1

2018-01-08 22:57 UTC #7 lore raw

Quoted 10 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
v3: fix only the units reporting, don't print the caller
v2: fix more messages

Looks good.

Reviewed-by Dave Chinner <[email protected]>

Signature · 2 lines

Dave Chinner
[email protected]