xfs_repair fixes, part 1

process_dinode_int() reports bad flags if dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if any flags are set outside the known set. But then instead of clearing them, it does flags &= ~XFS_DIFLAG_ANY which keeps *only* the bad flags. This leads to persistent, unrepairable errors of the form:

"Bad flags set in inode XXX"

Fix this.

While we are at it, fix a couple lines which look like they used to be continuation lines, but are no longer.

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

---

 repair/dinode.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

repair/dinode.c +3 -3

index 8891e84..38a6562 100644
@@ -2456,7 +2456,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	_("Bad flags set in inode %" PRIu64 "\n"),
 					lino);
 			}
-			flags &= ~XFS_DIFLAG_ANY;
+			flags &= XFS_DIFLAG_ANY;
 		}
 
 		if (flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) {

@@ -2513,11 +2513,11 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 		}
 		if (!verify_mode && flags != be16_to_cpu(dino->di_flags)) {
 			if (!no_modify) {
-				do_warn(_(", fixing bad flags.\n"));
+				do_warn(_("fixing bad flags.\n"));
 				dino->di_flags = cpu_to_be16(flags);
 				*dirty = 1;
 			} else
-				do_warn(_(", would fix bad flags.\n"));
+				do_warn(_("would fix bad flags.\n"));
 		}
 	}
 

Signature · 6 lines

1.7.1

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

process_dinode_int() reports bad flags if dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if any flags are set outside the known set. But then instead of clearing them, it does flags &= ~XFS_DIFLAG_ANY which keeps *only* the bad flags. This leads to persistent, unrepairable errors of the form:

Quoted 7 lines · Eric Sandeen

process_dinode_int() reports bad flags if dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if any flags are set outside the known set. But then instead of clearing them, it does flags &= ~XFS_DIFLAG_ANY which keeps *only* the bad flags. This leads to persistent, unrepairable errors of the form:

You know you can use up to 75 characters per line for your commit messages, don't you? :)

Quoted 9 lines · Christoph Hellwig

Quoted 7 lines · Eric Sandeen

process_dinode_int() reports bad flags if dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if any flags are set outside the known set. But then instead of clearing them, it does flags &= ~XFS_DIFLAG_ANY which keeps *only* the bad flags. This leads to persistent, unrepairable errors of the form:

Quoted 2 lines

You know you can use up to 75 characters per line for your commit messages, don't you? :)

hah, it's not automated at all, I guess my visual perception of the window is shrinking. Dave, feel free to fix on commit if inclined :)

process_shortform_attr uses the "junkit" error to track whether an error was found, but by assigning it directly to the result of valuecheck, previous errors are ignored, leading to unrepairable errors of the form i.e.

"entry has INCOMPLETE flag on in shortform attribute" or
"entry contains illegal character in shortform attribute name"

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

---

 repair/attr_repair.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

repair/attr_repair.c +2 -1

index a27a3ec..d60b664 100644
@@ -914,7 +914,8 @@ process_shortform_attr(
 
 		/* Only check values for root security attributes */
 		if (currententry->flags & XFS_ATTR_ROOT)
-		       junkit = valuecheck(mp, (char *)&currententry->nameval[0],
+		       junkit |= valuecheck(mp,
+					(char *)&currententry->nameval[0],
 					NULL, currententry->namelen, 
 					currententry->valuelen);
 

Signature · 6 lines

1.7.1

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

process_shortform_attr uses the "junkit" error to track whether an error was found, but by assigning it directly to the result of valuecheck, previous errors are ignored, leading to unrepairable errors of the form i.e.

"entry has INCOMPLETE flag on in shortform attribute" or
"entry contains illegal character in shortform attribute name"

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

In phase 6's longform_dir2_entry_check, if we never find a '.' entry we never add a reference to that entry; if we subsequently rebuild it, '.' gets added, but no ref to it is ever made. This leads to Phase 7 doing i.e.:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 2 to 1

and the next run will do:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 1 to 2

So if '.' was never found, but the directory got rebuilt, manually add the ref for it.

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

---
 repair/phase6.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

repair/phase6.c +0 -0

index f13069f..cc36a9c 100644

repair/phase6.c +0 -0

 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {

Signature · 6 lines

1.7.1

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

Quoted 27 lines · Eric Sandeen

In phase 6's longform_dir2_entry_check, if we never find a '.' entry we never add a reference to that entry; if we subsequently rebuild it, '.' gets added, but no ref to it is ever made. This leads to Phase 7 doing i.e.:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 2 to 1

and the next run will do:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 1 to 2

So if '.' was never found, but the directory got rebuilt, manually add the ref for it.

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

---
 repair/phase6.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

repair/phase6.c +0 -0

index f13069f..cc36a9c 100644

repair/phase6.c out_fix: · line 2296

Quoted 9 lines

repair/phase6.c +6 -0

@@ -2288,6 +2288,12 @@ out_fix:
 			if (bplist[i])
 				libxfs_putbuf(bplist[i]);
 		longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+		/*
+		 * If we didn't find a dot, we never added a ref for it;
+		 * it's there now after the rebuild, so mark it as reached.
+		 */
+		if (*need_dot)
+			add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name to the hashtable and handle the inode reference count in the first longform_dir2_entry_check() loop. If something is wrong, we call longform_dir2_rebuild() to rebuild the dir from the hashtable of names/inodes. We may or may not have added a reference for dot at that point, and need_dot is set appropriately.

This seems Ok, but where is the dot entry actually added? Hmm, I see that we handle dot in the longform_dir2_rebuild() loop by just skipping over it...

Brian

Quoted 10 lines

repair/phase6.c +0 -0

 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {

Signature · 6 lines

1.7.1

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

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

Quoted 45 lines · Brian Foster

Quoted 27 lines · Eric Sandeen

In phase 6's longform_dir2_entry_check, if we never find a '.' entry we never add a reference to that entry; if we subsequently rebuild it, '.' gets added, but no ref to it is ever made. This leads to Phase 7 doing i.e.:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 2 to 1

and the next run will do:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 1 to 2

So if '.' was never found, but the directory got rebuilt, manually add the ref for it.

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

---
  repair/phase6.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

repair/phase6.c +0 -0

index f13069f..cc36a9c 100644

repair/phase6.c out_fix: · line 2296

Quoted 9 lines

repair/phase6.c +6 -0

@@ -2288,6 +2288,12 @@ out_fix:
  			if (bplist[i])
  				libxfs_putbuf(bplist[i]);
  		longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+		/*
+		 * If we didn't find a dot, we never added a ref for it;
+		 * it's there now after the rebuild, so mark it as reached.
+		 */
+		if (*need_dot)
+			add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name to the hashtable and handle the inode reference count in the first longform_dir2_entry_check() loop. If something is wrong, we call longform_dir2_rebuild() to rebuild the dir from the hashtable of names/inodes. We may or may not have added a reference for dot at that point, and need_dot is set appropriately.

Quoted 4 lines

This seems Ok, but where is the dot entry actually added? Hmm, I see that we handle dot in the longform_dir2_rebuild() loop by just skipping over it...

It looks like this happens in process_dir_inode() after this whole check/rebuild sequence, directory format permitting. There's also an add_inode_ref() there. Perhaps the bug here is that we clear need_dot when we shouldn't..?

Quoted 27 lines · Eric Sandeen

In phase 6's longform_dir2_entry_check, if we never find a '.' entry we never add a reference to that entry; if we subsequently rebuild it, '.' gets added, but no ref to it is ever made. This leads to Phase 7 doing i.e.:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 2 to 1

and the next run will do:

Phase 7 - verify and correct link counts... resetting inode 5184 nlinks from 1 to 2

So if '.' was never found, but the directory got rebuilt, manually add the ref for it.

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

---
  repair/phase6.c |    6 ++++++
  1 files changed, 6 insertions(+), 0 deletions(-)

repair/phase6.c +0 -0

index f13069f..cc36a9c 100644

repair/phase6.c out_fix: · line 2296

Quoted 9 lines

repair/phase6.c +6 -0

@@ -2288,6 +2288,12 @@ out_fix:
  			if (bplist[i])
  				libxfs_putbuf(bplist[i]);
  		longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+		/*
+		 * If we didn't find a dot, we never added a ref for it;
+		 * it's there now after the rebuild, so mark it as reached.
+		 */
+		if (*need_dot)
+			add_inode_ref(irec, ino_offset);

So if I follow this correctly, we iterate through the dir, add each name to the hashtable and handle the inode reference count in the first longform_dir2_entry_check() loop. If something is wrong, we call longform_dir2_rebuild() to rebuild the dir from the hashtable of names/inodes. We may or may not have added a reference for dot at that point, and need_dot is set appropriately.

This seems Ok, but where is the dot entry actually added? Hmm, I see that we handle dot in the longform_dir2_rebuild() loop by just skipping over it...

xfs_dir3_dirent_get_ftype() gets the file type off disk, but ASSERTs if it's invalid:

	ASSERT(type < XFS_DIR3_FT_MAX);

This might be cut & paste from xfs_dir3_dirent_put_ftype which should be checking that it's not been passed bad values, but we shouldn't ASSERT on bad values read from disk.

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

---
 include/xfs_da_format.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

include/xfs_da_format.h +0 -0

index 89a1a21..11f1420 100644

On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:

Quoted 8 lines

include/xfs_da_format.h +0 -0

index 89a1a21..11f1420 100644
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
  		__uint8_t	type = dep->name[dep->namelen];
 

include/xfs_da_format.h xfs_dir3_dirent_get_ftype( · line 565

Quoted 3 lines

include/xfs_da_format.h +0 -1

-		ASSERT(type < XFS_DIR3_FT_MAX);
  		if (type < XFS_DIR3_FT_MAX)
  			return type;

Needs to be fixed kernel-side first.

Cheers,

Dave.

Quoted 1 line · Dave Chinner

On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:

...

Quoted 8 lines

Quoted 8 lines

include/xfs_da_format.h +0 -0

index 89a1a21..11f1420 100644
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
 	if (xfs_sb_version_hasftype(&mp->m_sb)) {
 		__uint8_t	type = dep->name[dep->namelen];
 

Quoted 5 lines

include/xfs_da_format.h xfs_dir3_dirent_get_ftype( · line 565

Quoted 3 lines

include/xfs_da_format.h +0 -1

-		ASSERT(type < XFS_DIR3_FT_MAX);
 		if (type < XFS_DIR3_FT_MAX)
 			return type;

Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)

bleah, why do they have different names... Ok, will send.

Quoted 1 line · Eric Sandeen

On 9/7/14 7:10 PM, Dave Chinner wrote:

Quoted 5 lines

Quoted 1 line

Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)

bleah, why do they have different names... Ok, will send.

Because kernel has changed, and we need to do yet another large kernel/user libxfs sync.

I haven't found time to do that yet. Unless you want to volunteer for it....

Quoted 2 lines · Dave Chinner

Quoted 1 line · Eric Sandeen

On 9/7/14 7:10 PM, Dave Chinner wrote:

...

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Needs to be fixed kernel-side first.

xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)

bleah, why do they have different names... Ok, will send.

Quoted 5 lines

Because kernel has changed, and we need to do yet another large kernel/user libxfs sync.

I haven't found time to do that yet. Unless you want to volunteer for it....

I could give it a shot. Do you usually do it commit-by-commit, diff-and-edit, or a big copy?

When we move files to lost+found, we're setting the filetype to UNKNOWN. This leaves an inconsistency which is discovered on a subsequent repair:

would fix ftype mismatch (0/1) in directory/child inode 5838/5839

Setting the proper ftype at the time of the move
resolves this: