Skip to contentSkip to conversation

xfs_repair: handle missing extent states

PATCH In review Darrick J. Wong 2 participants 6 messages Sep 18, 2017

lore
Share & tools
lore

Darrick J. Wong

author
#0 lore raw

PATCH 1/1 xfs_repair: handle missing extent states

Missed a couple of the new extent states in the bmbt processing, so add them to avoid aborting xfs_repair.

Signed-off-by Darrick J. Wong <[email protected]>
---
 repair/dinode.c |    7 +++++++
 1 file changed, 7 insertions(+)
repair/dinode.c +7 -0
index f817b5a..b35a523 100644
@@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
 			case XR_E_FS_MAP:
 			case XR_E_INO:
 			case XR_E_INUSE_FS:
+			case XR_E_REFC:
 				do_warn(
 _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
 					forkname, ino, b);

@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+
 			default:
 				do_error(
 _("illegal state %d in block map %" PRIu64 "\n"),
#1 lore raw
Quoted 13 lines · Darrick J. Wong

Missed a couple of the new extent states in the bmbt processing, so add them to avoid aborting xfs_repair.

Signed-off-by Darrick J. Wong <[email protected]>
---
 repair/dinode.c |    7 +++++++
 1 file changed, 7 insertions(+)
repair/dinode.c +0 -0
index f817b5a..b35a523 100644

repair/dinode.c _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"), · line 802

Quoted 7 lines
repair/dinode.c +1 -0
@@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
 			case XR_E_FS_MAP:
 			case XR_E_INO:
 			case XR_E_INUSE_FS:
+			case XR_E_REFC:
 				do_warn(
 _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
 					forkname, ino, b);

I already whined about the naming and use of these XR_E #defines on irc, but I think that since XR_E_REFC is set in process_rmap_rec when it's found in the rmap btree, this case should be above the /* fallthrough */ so it says "rmap claims metadata use!\n" like everything else set there, no?

repair/dinode.c _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"), · line 821

Quoted 10 lines
repair/dinode.c +6 -0
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+

why do cow blocks get a special case and custom warning vs the above cases that just say "metadata?"

Obviously it's just nitpicking over the do_warn message string, just double checking on the consistency front.

-Eric

#2 lore raw
Quoted 13 lines · Eric Sandeen

repair/dinode.c _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"), · line 821

Quoted 10 lines
repair/dinode.c +6 -0
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+

why do cow blocks get a special case and custom warning vs the above cases that just say "metadata?"

Quoted 2 lines

Obviously it's just nitpicking over the do_warn message string, just double checking on the consistency front.

And this was also found via the rmap, but that fact isn't printed like it is for every other type. For that reason I'd probably rather just add XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?

Thanks,
-Eric

author
#3 lore raw
Quoted 16 lines · Eric Sandeen
Quoted 13 lines · Eric Sandeen

repair/dinode.c _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"), · line 821

Quoted 10 lines
repair/dinode.c +6 -0
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+

why do cow blocks get a special case and custom warning vs the above cases that just say "metadata?"

Quoted 6 lines
Quoted 2 lines

Obviously it's just nitpicking over the do_warn message string, just double checking on the consistency front.

And this was also found via the rmap, but that fact isn't printed like it is for every other type. For that reason I'd probably rather just add XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?

Strictly speaking, CoW blocks aren't metadata; they're, uh, alternate data. I'd rather have xfs_repair deliver a more specific message about the other alleged owner of a block so I'd know where to start looking -- "claims CoW blocks" implies that I ought to start looking at the rmapbt for cow-owned blocks and the refcountbt, vs. just "shares with metadata, urk".

That said, we probably ought to have the rmapbt scan set XR_E_COW1 and have the refcountbt scan set XR_E_COW (similar to what we do for things like INUSE/INUSE1). Will try to work on that for 4.14, but for now let's at least not abort xfs_repair here.

--D

Quoted 7 lines

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

#4 lore raw
Quoted 20 lines · Darrick J. Wong
Quoted 19 lines · Eric Sandeen
Quoted 15 lines · Eric Sandeen

repair/dinode.c _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"), · line 821

Quoted 10 lines
repair/dinode.c +6 -0
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+

why do cow blocks get a special case and custom warning vs the above cases that just say "metadata?"

Obviously it's just nitpicking over the do_warn message string, just double checking on the consistency front.

Quoted 6 lines
Quoted 3 lines

And this was also found via the rmap, but that fact isn't printed like it is for every other type. For that reason I'd probably rather just add XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?

Strictly speaking, CoW blocks aren't metadata; they're, uh, alternate data.

Oh, fair point. ok.

Quoted 5 lines

I'd rather have xfs_repair deliver a more specific message about the other alleged owner of a block so I'd know where to start looking -- "claims CoW blocks" implies that I ought to start looking at the rmapbt for cow-owned blocks and the refcountbt, vs. just "shares with metadata, urk".

*nod*

Quoted 4 lines

That said, we probably ought to have the rmapbt scan set XR_E_COW1 and have the refcountbt scan set XR_E_COW (similar to what we do for things like INUSE/INUSE1). Will try to work on that for 4.14, but for now let's at least not abort xfs_repair here.

yep not asking for the world here, just trying to understand and maybe plant the seeds for future cleanups. ;)

-Eric

author
#5 lore raw
In reply to Eric Sandeen
Quoted 18 lines · Eric Sandeen
Quoted 17 lines · Darrick J. Wong

Missed a couple of the new extent states in the bmbt processing, so add them to avoid aborting xfs_repair.

Signed-off-by Darrick J. Wong <[email protected]>
---
 repair/dinode.c |    7 +++++++
 1 file changed, 7 insertions(+)
repair/dinode.c +1 -0
index f817b5a..b35a523 100644
@@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
 			case XR_E_FS_MAP:
 			case XR_E_INO:
 			case XR_E_INUSE_FS:
+			case XR_E_REFC:
Quoted 8 lines

repair/dinode.c _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"), · line 802

Quoted 3 lines
repair/dinode.c +0 -0
 				do_warn(
 _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
 					forkname, ino, b);

I already whined about the naming and use of these XR_E #defines on irc, but I think that since XR_E_REFC is set in process_rmap_rec when it's found in the rmap btree, this case should be above the /* fallthrough */ so it says "rmap claims metadata use!\n" like everything else set there, no?

Sure.

--D

Quoted 23 lines

repair/dinode.c _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"), · line 821

Quoted 11 lines
repair/dinode.c +6 -0
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
 					forkname, ftype, ino, b);
 				goto done;
 
+			case XR_E_COW:
+				do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+					forkname, ftype, ino, b);
+				goto done;
+

why do cow blocks get a special case and custom warning vs the above cases that just say "metadata?"

Obviously it's just nitpicking over the do_warn message string, just double checking on the consistency front.

-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