[PATCH 0/5] xfs_repair fixes, part 1

[PATCH 0/5] xfs_repair fixes, part 1

[Eric Sandeen]

I've been running a modified fsfuzzer and looking at the results
of xfs_repair of the corrupt images; plenty of things pop out.

I'm calling this "part one" because I expect that I'll find
more, but will send things out in digestable chunks as I
accumulate them...

Most of these are obviously correct and trivial; a coupl
require more thought & review.

Thanks,
-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int

[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:

"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 <sandeen@redhat.com>
---
 repair/dinode.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index 8891e84..38a6562 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -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"));
 		}
 	}
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr

[Eric Sandeen]

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 <sandeen@redhat.com>
---
 repair/attr_repair.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index a27a3ec..d60b664 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -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);
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt

[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 <sandeen@redhat.com>
---
 repair/phase6.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -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);
 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Eric Sandeen]

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 <sandeen@redhat.com>
---
 include/xfs_da_format.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
 	if (xfs_sb_version_hasftype(&mp->m_sb)) {
 		__uint8_t	type = dep->name[dep->namelen];
 
-		ASSERT(type < XFS_DIR3_FT_MAX);
 		if (type < XFS_DIR3_FT_MAX)
 			return type;
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/5] xfs_repair: set proper ftype when moving to lost+found

[Eric Sandeen]

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:

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/phase6.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index cc36a9c..02714c2 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1091,14 +1091,13 @@ mv_orphanage(
 	ino_tree_node_t		*irec;
 	int			ino_offset = 0;
 	struct xfs_name		xname;
+	__uint16_t		di_mode;
 
 	ASSERT(xfs_sb_version_hasdirv2(&mp->m_sb));
 
 	xname.name = fname;
 	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
 				(unsigned long long)ino);
-	/* XXX use xfs_mode_to_ftype[] when userspace gains it */
-	xname.type = XFS_DIR3_FT_UNKNOWN;
 
 	err = libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip, 0);
 	if (err)
@@ -1117,6 +1116,10 @@ mv_orphanage(
 	if ((err = libxfs_iget(mp, NULL, ino, 0, &ino_p, 0)))
 		do_error(_("%d - couldn't iget disconnected inode\n"), err);
 
+	di_mode = ino_p->i_d.di_mode;
+	di_mode = (di_mode & S_IFMT) >> S_SHIFT;
+	xname.type = xfs_mode_to_ftype[di_mode];
+
 	if (isa_dir)  {
 		irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp, orphanage_ino),
 				XFS_INO_TO_AGINO(mp, orphanage_ino));
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/5] xfs_repair: don't re-add root dotdot if root dir was rebuilt

[Eric Sandeen]

If we've rebuilt the root directory, ".." was taken
care of, so clear need_root_dotdot.

Otherwise it will be added twice, and a subsequent repair
will say:

  entry ".." (ino 5824) in dir 5824 is a duplicate name, would junk entry

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

(sorry for 6/5, this just popped out and is similar to the
patch 3/5 I just sent, so probably worth doing at the same time.

diff --git a/repair/phase6.c b/repair/phase6.c
index cc36a9c..2e67c60 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2294,6 +2297,9 @@ out_fix:
  		 */
  		if (*need_dot)
  			add_inode_ref(irec, ino_offset);
+		/* If we rebuilt the root dir, dot dot is in good shape */
+		if (ino == mp->m_sb.sb_rootino)
+			need_root_dotdot = 0;
  		*num_illegal = 0;
  		*need_dot = 0;
  	} else {

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs_repair: set proper ftype when moving to lost+found

[Eric Sandeen]

On 9/7/14 11:41 AM, Eric Sandeen wrote:
> 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:

Ah, arekm points out that Jan already sent a patch to do this:

[PATCH] repair: Set ftype for entries in lost+found

http://oss.sgi.com/archives/xfs/2014-07/msg00314.html

Sorry, missed that.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Dave Chinner]

On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>  include/xfs_da_format.h |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> index 89a1a21..11f1420 100644
> --- a/include/xfs_da_format.h
> +++ b/include/xfs_da_format.h
> @@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
>  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
>  		__uint8_t	type = dep->name[dep->namelen];
>  
> -		ASSERT(type < XFS_DIR3_FT_MAX);
>  		if (type < XFS_DIR3_FT_MAX)
>  			return type;

Needs to be fixed kernel-side first.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Eric Sandeen]

On 9/7/14 7:10 PM, Dave Chinner wrote:
> On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:

...

>> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
>> index 89a1a21..11f1420 100644
>> --- a/include/xfs_da_format.h
>> +++ b/include/xfs_da_format.h
>> @@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
>>   	if (xfs_sb_version_hasftype(&mp->m_sb)) {
>>   		__uint8_t	type = dep->name[dep->namelen];
>>
>> -		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.

-Eric
  
> Cheers,
>
> Dave.
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Dave Chinner]

On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
> On 9/7/14 7:10 PM, Dave Chinner wrote:
> >On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:
> 
> ...
> 
> >>diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> >>index 89a1a21..11f1420 100644
> >>--- a/include/xfs_da_format.h
> >>+++ b/include/xfs_da_format.h
> >>@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
> >>  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
> >>  		__uint8_t	type = dep->name[dep->namelen];
> >>
> >>-		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.

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Eric Sandeen]

On 9/7/14 10:16 PM, Dave Chinner wrote:
> On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
>> On 9/7/14 7:10 PM, Dave Chinner wrote:

...

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

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

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype

[Dave Chinner]

On Sun, Sep 07, 2014 at 10:18:09PM -0500, Eric Sandeen wrote:
> On 9/7/14 10:16 PM, Dave Chinner wrote:
> >On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
> >>On 9/7/14 7:10 PM, Dave Chinner wrote:
> 
> ...
> 
> >>>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....
> 
> I could give it a shot.  Do you usually do it commit-by-commit,
> diff-and-edit, or a big copy?

This one is going to need multiple phases, and in a moment you're
going to understand what all functions called into libxfs should
have a "libxfs define wrapper":

	1. sync up to kernel commit before error negation
	2. make sure all external function calls have libxfs
	   define wrappers
	3. commit error negation patch and change sign of all libxfs
	   define wrappers
	4. apply kernel libxfs rename patch and restructure
	   libxfs userspace and xfsprogs build to match
	5. continue syncing kernel changes commit by commit.

Given the scope of changes, and the fact that some of the changes
have already been applied (i.e. bug fixes, finobt changes, etc)
step #1 is not going to be a straight forward commit-by-commit.
So I'm happy to do that as a bulk commit. Steps #2 and #3 are
relatively straight forward, too, just a bit painful as we need
to be sure we don't leak negative errors.

Step #4 is the big one; we want to end up with the kernel
fs/xfs/libxfs to be identical to the xfsprogs:/libxfs/ code and so
in userspace we've got to move headers around and rejig includes and
things like that. That's one I should probably do.

And from there, step 5 should be a simple extract/sed/commit process
of pulling patch by patch from the kernel changes that touch
fs/xfs/libxfs....

So, really, the big step right now is #1...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int

[Brian Foster]

On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  repair/dinode.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 8891e84..38a6562 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -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"));
>  		}
>  	}
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr

[Brian Foster]

On Sun, Sep 07, 2014 at 11:41:02AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  repair/attr_repair.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index a27a3ec..d60b664 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -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);
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt

[Brian Foster]

On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> ---
>  repair/phase6.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index f13069f..cc36a9c 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -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


>  		*num_illegal = 0;
>  		*need_dot = 0;
>  	} else {
> -- 
> 1.7.1
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt

[Brian Foster]

On Mon, Sep 08, 2014 at 09:45:25AM -0400, Brian Foster wrote:
> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
> > 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 <sandeen@redhat.com>
> > ---
> >  repair/phase6.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index f13069f..cc36a9c 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -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...
> 

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

Brian

> Brian
> 
> 
> >  		*num_illegal = 0;
> >  		*need_dot = 0;
> >  	} else {
> > -- 
> > 1.7.1
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt

[Eric Sandeen]

On 9/8/14 8:45 AM, Brian Foster wrote:
> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
>> 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 <sandeen@redhat.com>
>> ---
>>   repair/phase6.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/repair/phase6.c b/repair/phase6.c
>> index f13069f..cc36a9c 100644
>> --- a/repair/phase6.c
>> +++ b/repair/phase6.c
>> @@ -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...

longform_dir2_rebuild calls this before the loop:

/*
  * Initialize a directory with its "." and ".." entries.
  */
int
xfs_dir_init()

But it doesn't actually create .; this creates a shortform dir,
and shortform dirs have no '.' - I guess the comment could use
an update.

In the loop we call xfs_dir_createname() for everything in the hash,
eventually things don't fit in shortform and we do xfs_dir2_sf_to_block,
which creates the dot:

         /*
          * Create entry for .
          */
         dep = xfs_dir3_data_dot_entry_p(mp, hdr);
         dep->inumber = cpu_to_be64(dp->i_ino);
         dep->namelen = 1;
         dep->name[0] = '.';

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt

[Eric Sandeen]

On 9/8/14 9:25 AM, Brian Foster wrote:
> On Mon, Sep 08, 2014 at 09:45:25AM -0400, Brian Foster wrote:
>> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
>>> 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 <sandeen@redhat.com>
>>> ---
>>>   repair/phase6.c |    6 ++++++
>>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/repair/phase6.c b/repair/phase6.c
>>> index f13069f..cc36a9c 100644
>>> --- a/repair/phase6.c
>>> +++ b/repair/phase6.c
>>> @@ -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...
>>
>
> 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..?

If we do that, the first run says:

bad hash table for directory inode 5184 (no data entry): rebuilding
rebuilding directory inode 5184
creating missing "." entry in dir ino 5184

and then the 2nd run says:

multiple . entries in directory inode 5184: clearing entry

so, no.  ;)

The issue is that add_inode_ref() is keeping track (in repair)
of reached paths to the inode, in counted_nlinks.

If we didn't find '.' originally, we didn't add that ref.

When we do:


longform_dir2_rebuild
	xfs_dir_init()  // creates shortform
	<loop over names>
		xfs_dir_createname
			xfs_dir2_sf_to_block when it's big enough
				add '.' entry

and then we've added the '.' but haven't added the reference repair needs
internally.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int

[Christoph Hellwig]

On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> 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? :)

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr

[Christoph Hellwig]

On Sun, Sep 07, 2014 at 11:41:02AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int

[Eric Sandeen]

On 9/9/14 5:28 PM, Christoph Hellwig wrote:
> On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
>> 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? :)

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

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int

[Dave Chinner]

On Tue, Sep 09, 2014 at 05:33:30PM -0500, Eric Sandeen wrote:
> On 9/9/14 5:28 PM, Christoph Hellwig wrote:
> >On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> >>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? :)
> 
> 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 :)

I mostly do already - I tend to reflow commit messages to 68
characters (same width I use for email) when I see something like
this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs