Skip to contentSkip to conversation

xfsprogs: a handful of fixes

PATCH New Eric Sandeen 4 participants 13 messages Mar 4, 2015 – Mar 13, 2015

lore
Share & tools
lore
#1 lore raw

PATCH 1/4 xfs_repair: validate & fix inode CRCs

xfs_repair doesn't ever check an inode's CRC, so it never repairs them. If the root inode or realtime inodes have bad crcs, the fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before we do any other checking or modification of the inode, once we get past the "verify only" phase of process_dinode_int(); just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by Eric Sandeen <[email protected]>

---

Resend of patch from a while ago, but with CRC verification now earlier in the function, prior to any changes are made.

repair/dinode.c +21 -0
index 5d9094b..384e2dd 100644
@@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);
 
+	/*
+	 * We'd really like to know if the CRC is bad before we
+	 * go fixing anything; that way we have some hint about
+	 * bit-rot vs bugs.  Also, any changes will invalidate the
+	 * existing CRC, so this is the only valid point to test it.
+	 *
+	 * Of course if we make any modifications after this, the
+	 * inode gets rewritten, and CRC is updated automagically.
+	 */
+	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {
+		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+                		XFS_DINODE_CRC_OFF)) {
+			do_warn(_("bad CRC for inode %" PRIu64), lino);
+			if (!no_modify) {
+				do_warn(_(", will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(", would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw

PATCH 2/4 xfs_repair: clear need_root_dotdot if we rebuild the root dir

In reply to Eric Sandeen

It's possible to enter longform_dir2_rebuild with need_root_dotdot set; rebuilding it will add "..", and if need_root_dotdot stays set, we'll add another ".." entry, causing a second repair to find and fix that newly-introduced problem.

Signed-off-by Eric Sandeen <[email protected]>

---

repair/phase6.c +3 -0
index 0ec4f07..1728609 100644
@@ -1354,6 +1354,9 @@ longform_dir2_rebuild(
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
+	if (ino == mp->m_sb.sb_rootino)
+		need_root_dotdot = 0;
+
 	/* go through the hash list and re-add the inodes */
 
 	for (p = hashtab->first; p; p = p->nextbyorder) {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#7 lore raw

PATCH 3/4 xfs_db: show nlink fields for di_version == 3, too

In reply to Eric Sandeen

Printing inodes with di_version == 3 skips the nlink fields, because they are only printed if di_version == 2. This was intended to separate them from di_version == 1, but it mistakenly excluded di_version == 3, which also contains these fields.

Signed-off-by Eric Sandeen <[email protected]>

---

Not sure; is >= 2 ok, or should it be == 2 || == 3? Choose your poison, I guess.

db/inode.c +3 -3
index 982acb7..c26e1a0 100644
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#11 lore raw

PATCH 4/4 xfs_repair: set *parent if process_dir2_data() fixes root inode's parent

In reply to Eric Sandeen

process_dir2_data() may fix the root dir's parent inode:

"bad .. entry in root directory inode 6912, was 7159: correcting"

But we don't update the *parent passed in in that case; this then leads to an assert later in process_dir2, because *parent is still the wrong value:

xfs_repair: dir2.c:2039: process_dir2:
Assertion `(ino != mp->m_sb.sb_rootino && ino != *parent) || (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))' failed.

Updating the value of *parent when we fix the parent value resolves this problem. Do it whether or not we're in no_modify mode.

Signed-off-by Eric Sandeen <[email protected]>

---

repair/dir2.c +1 -0
index 6b8964d..67cd9d1 100644
@@ -1468,6 +1468,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
 					} else {
 						do_warn(_("would correct\n"));
 					}
+					*parent = ino;
 				}
 			}
 			/*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/4
#2 lore raw

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

In reply to Eric Sandeen

Looks good to me.

Reviewed-by Carlos Maiolino <[email protected]>
Quoted 52 lines · Eric Sandeen

xfs_repair doesn't ever check an inode's CRC, so it never repairs them. If the root inode or realtime inodes have bad crcs, the fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before we do any other checking or modification of the inode, once we get past the "verify only" phase of process_dinode_int(); just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by Eric Sandeen <[email protected]>

---

Resend of patch from a while ago, but with CRC verification now earlier in the function, prior to any changes are made.

repair/dinode.c +21 -0
index 5d9094b..384e2dd 100644
@@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);
 
+	/*
+	 * We'd really like to know if the CRC is bad before we
+	 * go fixing anything; that way we have some hint about
+	 * bit-rot vs bugs.  Also, any changes will invalidate the
+	 * existing CRC, so this is the only valid point to test it.
+	 *
+	 * Of course if we make any modifications after this, the
+	 * inode gets rewritten, and CRC is updated automagically.
+	 */
+	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {
+		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+                		XFS_DINODE_CRC_OFF)) {
+			do_warn(_("bad CRC for inode %" PRIu64), lino);
+			if (!no_modify) {
+				do_warn(_(", will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(", would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Signature · 6 lines
Carlos

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 2/4
#6 lore raw

Re: [PATCH 2/4] xfs_repair: clear need_root_dotdot if we rebuild the root dir

In reply to Eric Sandeen

Looks good

Reviewed-by Carlos Maiolino <[email protected]>
Quoted 28 lines · Eric Sandeen

It's possible to enter longform_dir2_rebuild with need_root_dotdot set; rebuilding it will add "..", and if need_root_dotdot stays set, we'll add another ".." entry, causing a second repair to find and fix that newly-introduced problem.

Signed-off-by Eric Sandeen <[email protected]>

---

repair/phase6.c +3 -0
index 0ec4f07..1728609 100644
@@ -1354,6 +1354,9 @@ longform_dir2_rebuild(
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
+	if (ino == mp->m_sb.sb_rootino)
+		need_root_dotdot = 0;
+
 	/* go through the hash list and re-add the inodes */
 
 	for (p = hashtab->first; p; p = p->nextbyorder) {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Signature · 6 lines
Carlos

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#8 lore raw

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

In reply to Eric Sandeen

I'm not sure why we differentiate inode versions here, to print the nlink, I probably have never seen a filesystem with di_version == 1 (feeling young :)

Why does it needs to be tested? I don't think di_version 1 didn't have nlink field, but if it did, and if this is the only difference, in this subject, I don't see a problem in having it tested with >= 2. But, if there could be more than only this difference, maybe some kind of helper function, or even a macro to make this test, taking into account the di_version.

Quoted 49 lines · Eric Sandeen

Printing inodes with di_version == 3 skips the nlink fields, because they are only printed if di_version == 2. This was intended to separate them from di_version == 1, but it mistakenly excluded di_version == 3, which also contains these fields.

Signed-off-by Eric Sandeen <[email protected]>

---

Not sure; is >= 2 ok, or should it be == 2 || == 3? Choose your poison, I guess.

db/inode.c +3 -3
index 982acb7..c26e1a0 100644
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Signature · 6 lines
Carlos

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 4/4
#12 lore raw

Re: [PATCH 4/4] xfs_repair: set *parent if process_dir2_data() fixes root inode's parent

In reply to Eric Sandeen

Makes sense to me.

Reviewed-by Carlos Maiolino <[email protected]>

Thanks Eric :-)

Cheers,

Quoted 35 lines · Eric Sandeen

process_dir2_data() may fix the root dir's parent inode:

"bad .. entry in root directory inode 6912, was 7159: correcting"

But we don't update the *parent passed in in that case; this then leads to an assert later in process_dir2, because *parent is still the wrong value:

xfs_repair: dir2.c:2039: process_dir2:
Assertion `(ino != mp->m_sb.sb_rootino && ino != *parent) || (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))' failed.

Updating the value of *parent when we fix the parent value resolves this problem. Do it whether or not we're in no_modify mode.

Signed-off-by Eric Sandeen <[email protected]>

---

repair/dir2.c +1 -0
index 6b8964d..67cd9d1 100644
@@ -1468,6 +1468,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
 					} else {
 						do_warn(_("would correct\n"));
 					}
+					*parent = ino;
 				}
 			}
 			/*

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Signature · 6 lines
Carlos

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#9 lore raw

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

In reply to Carlos Maiolino
Quoted 2 lines · Carlos Maiolino

I'm not sure why we differentiate inode versions here, to print the nlink, I probably have never seen a filesystem with di_version == 1 (feeling young :)

di_version=1 had only a 16-bit nlink field, now called di_onlink ("old nlink"). Later a new 32-bit structure member was added for this purpose.

xfs_db decides what to print, from this table:

{ "nlinkv1", FLDT_UINT16D, OI(COFF(onlink)), inode_core_nlinkv1_count,
FLD_COUNT, TYP_NONE },
{ "nlinkv2", FLDT_UINT32D, OI(COFF(nlink)), inode_core_nlinkv2_count,
FLD_COUNT, TYP_NONE },
{ "onlink", FLDT_UINT16D, OI(COFF(onlink)), inode_core_onlink_count,
FLD_COUNT, TYP_NONE },

where the specified functions test whether the field should be printed.

This means "nlinkv1" will be displayed only if di_version==1, and "onlink" & "nlinkv2" will be displayed only if di_version==2 (today) or >= 2 (with my patch).

(the di_nlink field got renamed to di_onlink, and a new 16-bit di_nlink member was added. xfs_db doesn't always print the actual structure names verbatim)

Quoted 5 lines

Why does it needs to be tested? I don't think di_version 1 didn't have nlink field, but if it did, and if this is the only difference, in this subject, I don't see a problem in having it tested with >= 2. But, if there could be more than only this difference, maybe some kind of helper function, or even a macro to make this test, taking into account the di_version.

It's tested because on a version 1 inode, the 32-bit nlink field doesn't even exist. We'd be printing random data from the disk.

-Eric

Quoted 43 lines · Eric Sandeen
Quoted 42 lines

Printing inodes with di_version == 3 skips the nlink fields, because they are only printed if di_version == 2. This was intended to separate them from di_version == 1, but it mistakenly excluded di_version == 3, which also contains these fields.

Signed-off-by Eric Sandeen <[email protected]>

---

Not sure; is >= 2 ok, or should it be == 2 || == 3? Choose your poison, I guess.

db/inode.c +3 -3
index 982acb7..c26e1a0 100644
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
Quoted 8 lines
Quoted 7 lines
db/inode.c +0 -0
 static int

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

In reply to Eric Sandeen
Quoted 23 lines · Eric Sandeen

xfs_repair doesn't ever check an inode's CRC, so it never repairs them. If the root inode or realtime inodes have bad crcs, the fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before we do any other checking or modification of the inode, once we get past the "verify only" phase of process_dinode_int(); just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by Eric Sandeen <[email protected]>

---

Resend of patch from a while ago, but with CRC verification now earlier in the function, prior to any changes are made.

repair/dinode.c +0 -0
index 5d9094b..384e2dd 100644
@@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);

repair/dinode.c process_dinode_int(xfs_mount_t *mp, · line 2303

Quoted 10 lines
repair/dinode.c +10 -0
+	/*
+	 * We'd really like to know if the CRC is bad before we
+	 * go fixing anything; that way we have some hint about
+	 * bit-rot vs bugs.  Also, any changes will invalidate the
+	 * existing CRC, so this is the only valid point to test it.
+	 *
+	 * Of course if we make any modifications after this, the
+	 * inode gets rewritten, and CRC is updated automagically.
+	 */
+	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {

What about verify_mode? It should probably report the inode as bunk if the crc is bad. Some of the subsequent checks handle this with some logic to just return 1 if something is bogus and we're in verify mode.

repair/dinode.c process_dinode_int(xfs_mount_t *mp, · line 2305

Quoted 2 lines
repair/dinode.c +2 -0
+		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+                		XFS_DINODE_CRC_OFF)) {

I got a whitespace damage warning here:

/.../xfsprogs-dev/.git/rebase-apply/patch:25: space before tab in indent.

                		XFS_DINODE_CRC_OFF)) {

warning: 1 line adds whitespace errors.

Brian

Quoted 13 lines
repair/dinode.c +9 -0
+			do_warn(_("bad CRC for inode %" PRIu64), lino);
+			if (!no_modify) {
+				do_warn(_(", will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(", would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)
Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 3/4
#10 lore raw

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

In reply to Eric Sandeen
Quoted 6 lines · Eric Sandeen

Printing inodes with di_version == 3 skips the nlink fields, because they are only printed if di_version == 2. This was intended to separate them from di_version == 1, but it mistakenly excluded di_version == 3, which also contains these fields.

Quoted 6 lines
Signed-off-by Eric Sandeen <[email protected]>

---

Not sure; is >= 2 ok, or should it be == 2 || == 3? Choose your poison, I guess.

Seems fine to me. I think it's better to only have to worry about this code if something related to these fields changes rather than every time a version update occurs.

Reviewed-by Brian Foster <[email protected]>
Quoted 33 lines
db/inode.c +3 -3
index 982acb7..c26e1a0 100644
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int

@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#4 lore raw

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

In reply to Brian Foster

On 3/13/15 8:20 AM, Brian Foster wrote:

...

Quoted 3 lines

What about verify_mode? It should probably report the inode as bunk if the crc is bad. Some of the subsequent checks handle this with some logic to just return 1 if something is bogus and we're in verify mode.

So, I read through the verify_mode portion on the plane ride back; I think it's gotten a bit inconsistent over time. I feel like I understand it a bit better now, and will send a v3 which matches what I understand to be its intent, and another patch to document it & make the rest consistent.

Thanks for keeping me honest. ;)

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs