[PATCH 0/4] xfs_repair: clean up process_sf_dir2

[PATCH 0/4] xfs_repair: clean up process_sf_dir2

[Eric Sandeen]

4 patches, which in the end collapse & remove a bit of code.

First patch adds a missing dino_dirty flag in a cut&paste codepath
Second patch removes impossible tests (if (A) tests under a
(!A) codepath).
Third patch collapses two different "bad namelen" casess into one.
Fourth removes special handling of the last entry, which doesn't work anyway.

More details in the individual patches.

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

[PATCH 1/4] xfs_repair: dirty inode in process_sf_dir2 if we change namelen

[Eric Sandeen]

There are two "fix sfep->namelen" cases, but we only mark
*dino_dirty = 1 in one of them.  Add the other to ensure that
the change gets written out.

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

diff --git a/repair/dir2.c b/repair/dir2.c
index 6b8964d..25793e9 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -881,6 +881,7 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
 _("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
 						ino, namelen);
 					sfep->namelen = namelen;
+					*dino_dirty = 1;
 				} else  {
 					do_warn(
 _("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),

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

Re: [PATCH 1/4] xfs_repair: dirty inode in process_sf_dir2 if we change namelen

[Brian Foster]

On Tue, Mar 10, 2015 at 04:55:59PM -0400, Eric Sandeen wrote:
> There are two "fix sfep->namelen" cases, but we only mark
> *dino_dirty = 1 in one of them.  Add the other to ensure that
> the change gets written out.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 6b8964d..25793e9 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -881,6 +881,7 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
>  _("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
>  						ino, namelen);
>  					sfep->namelen = namelen;
> +					*dino_dirty = 1;
>  				} else  {
>  					do_warn(
>  _("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),
> 
> _______________________________________________
> 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

[PATCH 2/4] xfs_repair: remove impossible tests in process_sf_dir2

[Eric Sandeen]

We're testing for an impossible case in here:

                if (i == num_entries - 1)  {
...
                } else  {
...
                                if (i == num_entries - 1)
                                    /* can't happen! */
...
                }

So, remove the impossible case.

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

diff --git a/repair/dir2.c b/repair/dir2.c
index 25793e9..7aede6a 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -928,26 +928,12 @@ _("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
 				do_warn(
 _("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
 					i, ino);
-				if (!no_modify)  {
-					if (i == num_entries - 1)
-						do_warn(
-						_("junking entry #%d\n"),
-							i);
-					else
-						do_warn(
-						_("junking %d entries\n"),
-							num_entries - i);
-				} else  {
-					if (i == num_entries - 1)
-						do_warn(
-						_("would junk entry #%d\n"),
-							i);
-					else
-						do_warn(
-						_("would junk %d entries\n"),
-							num_entries - i);
-				}
-
+				if (!no_modify)
+					do_warn(_("junking %d entries\n"),
+						num_entries - i);
+				else
+					do_warn(_("would junk %d entries\n"),
+						num_entries - i);
 				break;
 			}
 		}

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

Re: [PATCH 2/4] xfs_repair: remove impossible tests in process_sf_dir2

[Brian Foster]

On Tue, Mar 10, 2015 at 05:01:26PM -0400, Eric Sandeen wrote:
> We're testing for an impossible case in here:
> 
>                 if (i == num_entries - 1)  {
> ...
>                 } else  {
> ...
>                                 if (i == num_entries - 1)
>                                     /* can't happen! */
> ...
>                 }
> 
> So, remove the impossible case.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 25793e9..7aede6a 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -928,26 +928,12 @@ _("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
>  				do_warn(
>  _("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
>  					i, ino);
> -				if (!no_modify)  {
> -					if (i == num_entries - 1)
> -						do_warn(
> -						_("junking entry #%d\n"),
> -							i);
> -					else
> -						do_warn(
> -						_("junking %d entries\n"),
> -							num_entries - i);
> -				} else  {
> -					if (i == num_entries - 1)
> -						do_warn(
> -						_("would junk entry #%d\n"),
> -							i);
> -					else
> -						do_warn(
> -						_("would junk %d entries\n"),
> -							num_entries - i);
> -				}
> -
> +				if (!no_modify)
> +					do_warn(_("junking %d entries\n"),
> +						num_entries - i);
> +				else
> +					do_warn(_("would junk %d entries\n"),
> +						num_entries - i);
>  				break;
>  			}
>  		}
> 
> _______________________________________________
> 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

[PATCH 3/4] xfs_repair: collapse 2 cases in process_sf_dir2

[Eric Sandeen]

process_sf_dir2() has 2 cases for a bad namelen: on-disk namelen
is 0, or on-disk namelen extends beyond the directory i_size.

After the prior 2 patches, the code for each case is now essentially
the same, so collapse the two cases.

Note, this does slightly change some of the error messages.

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

diff --git a/repair/dir2.c b/repair/dir2.c
index 7aede6a..5512e41 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -861,36 +861,44 @@ process_sf_dir2(
 _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRIu64 "\n"),
 				namelen, namelen, sfep->name, ino, junkreason,
 				lino);
-		if (namelen == 0)  {
-			/*
-			 * if we're really lucky, this is
-			 * the last entry in which case we
-			 * can use the dir size to set the
-			 * namelen value.  otherwise, forget
-			 * it because we're not going to be
-			 * able to find the next entry.
-			 */
+
+		/* is dir namelen 0 or does this entry extend past dir size? */
+		if (namelen == 0) {
+			junkreason = _("is zero length");
 			bad_sfnamelen = 1;
+		} else if ((__psint_t) sfep - (__psint_t) sfp +
+				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
+							> ino_dir_size)  {
+			junkreason = _("extends past end of dir");
+			bad_sfnamelen = 1;
+		}
 
+		if (bad_sfnamelen) {
+			/*
+			 * if we're really lucky, this is the last entry in
+			 * case we can use the dir size to set the namelen
+			 * value.  otherwise, forget it because we're not going
+			 * to be able to find the next entry.
+			 */
 			if (i == num_entries - 1)  {
 				namelen = ino_dir_size -
 					((__psint_t) &sfep->name[0] -
 					 (__psint_t) sfp);
 				if (!no_modify)  {
 					do_warn(
-_("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
-						ino, namelen);
+_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
+						i, junkreason, ino, namelen);
 					sfep->namelen = namelen;
 					*dino_dirty = 1;
 				} else  {
 					do_warn(
-_("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),
-						ino, namelen);
+_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
+						i, junkreason, ino, namelen);
 				}
 			} else  {
 				do_warn(
-_("zero length entry in shortform dir %" PRIu64 ""),
-					ino);
+_("entry #%d %s in shortform dir %" PRIu64),
+					i, junkreason, ino);
 				if (!no_modify)
 					do_warn(_(", junking %d entries\n"),
 						num_entries - i);
@@ -903,39 +911,6 @@ _("zero length entry in shortform dir %" PRIu64 ""),
 				 */
 				break;
 			}
-		} else if ((__psint_t) sfep - (__psint_t) sfp +
-				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
-							> ino_dir_size)  {
-			bad_sfnamelen = 1;
-
-			if (i == num_entries - 1)  {
-				namelen = ino_dir_size -
-					((__psint_t) &sfep->name[0] -
-					 (__psint_t) sfp);
-				do_warn(
-_("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
-					ino);
-				if (!no_modify)  {
-					do_warn(_("resetting to %d\n"),
-						namelen);
-					sfep->namelen = namelen;
-					*dino_dirty = 1;
-				} else  {
-					do_warn(_("would reset to %d\n"),
-						namelen);
-				}
-			} else  {
-				do_warn(
-_("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
-					i, ino);
-				if (!no_modify)
-					do_warn(_("junking %d entries\n"),
-						num_entries - i);
-				else
-					do_warn(_("would junk %d entries\n"),
-						num_entries - i);
-				break;
-			}
 		}
 
 		/*

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

Re: [PATCH 3/4] xfs_repair: collapse 2 cases in process_sf_dir2

[Brian Foster]

On Tue, Mar 10, 2015 at 05:04:15PM -0400, Eric Sandeen wrote:
> process_sf_dir2() has 2 cases for a bad namelen: on-disk namelen
> is 0, or on-disk namelen extends beyond the directory i_size.
> 
> After the prior 2 patches, the code for each case is now essentially
> the same, so collapse the two cases.
> 
> Note, this does slightly change some of the error messages.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 7aede6a..5512e41 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -861,36 +861,44 @@ process_sf_dir2(
>  _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRIu64 "\n"),
>  				namelen, namelen, sfep->name, ino, junkreason,
>  				lino);
> -		if (namelen == 0)  {
> -			/*
> -			 * if we're really lucky, this is
> -			 * the last entry in which case we
> -			 * can use the dir size to set the
> -			 * namelen value.  otherwise, forget
> -			 * it because we're not going to be
> -			 * able to find the next entry.
> -			 */
> +
> +		/* is dir namelen 0 or does this entry extend past dir size? */
> +		if (namelen == 0) {
> +			junkreason = _("is zero length");
>  			bad_sfnamelen = 1;
> +		} else if ((__psint_t) sfep - (__psint_t) sfp +
> +				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
> +							> ino_dir_size)  {
> +			junkreason = _("extends past end of dir");
> +			bad_sfnamelen = 1;
> +		}
>  
> +		if (bad_sfnamelen) {
> +			/*
> +			 * if we're really lucky, this is the last entry in

"... in which case ..."

(irrelevant as this is removed by the next patch)

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

> +			 * case we can use the dir size to set the namelen
> +			 * value.  otherwise, forget it because we're not going
> +			 * to be able to find the next entry.
> +			 */
>  			if (i == num_entries - 1)  {
>  				namelen = ino_dir_size -
>  					((__psint_t) &sfep->name[0] -
>  					 (__psint_t) sfp);
>  				if (!no_modify)  {
>  					do_warn(
> -_("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
> -						ino, namelen);
> +_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
> +						i, junkreason, ino, namelen);
>  					sfep->namelen = namelen;
>  					*dino_dirty = 1;
>  				} else  {
>  					do_warn(
> -_("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),
> -						ino, namelen);
> +_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
> +						i, junkreason, ino, namelen);
>  				}
>  			} else  {
>  				do_warn(
> -_("zero length entry in shortform dir %" PRIu64 ""),
> -					ino);
> +_("entry #%d %s in shortform dir %" PRIu64),
> +					i, junkreason, ino);
>  				if (!no_modify)
>  					do_warn(_(", junking %d entries\n"),
>  						num_entries - i);
> @@ -903,39 +911,6 @@ _("zero length entry in shortform dir %" PRIu64 ""),
>  				 */
>  				break;
>  			}
> -		} else if ((__psint_t) sfep - (__psint_t) sfp +
> -				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
> -							> ino_dir_size)  {
> -			bad_sfnamelen = 1;
> -
> -			if (i == num_entries - 1)  {
> -				namelen = ino_dir_size -
> -					((__psint_t) &sfep->name[0] -
> -					 (__psint_t) sfp);
> -				do_warn(
> -_("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
> -					ino);
> -				if (!no_modify)  {
> -					do_warn(_("resetting to %d\n"),
> -						namelen);
> -					sfep->namelen = namelen;
> -					*dino_dirty = 1;
> -				} else  {
> -					do_warn(_("would reset to %d\n"),
> -						namelen);
> -				}
> -			} else  {
> -				do_warn(
> -_("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
> -					i, ino);
> -				if (!no_modify)
> -					do_warn(_("junking %d entries\n"),
> -						num_entries - i);
> -				else
> -					do_warn(_("would junk %d entries\n"),
> -						num_entries - i);
> -				break;
> -			}
>  		}
>  
>  		/*
> 
> _______________________________________________
> 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

[PATCH 4/4] xfs_repair: remove last-entry hack in process_sf_dir2

[Eric Sandeen]

process_sf_dir2() tries to special-case the last entry in
a short form dir, and salvage it if the name length is wrong
by using the dir size as a clue to what the length should be.

However, this doesn't actually work; there is either a 32-bit
or 64-bit inode after the name, and with dirv3 there's a file
type in there as well.  Extending the name length to the dir
size means it overlaps these fields, which often have a 0 in
the top bits, and then namecheck() fails the result and junks
it anyway:

> entry #1 is zero length in shortform dir 67, resetting to 29
> entry contains illegal character in shortform dir 67
> junking entry "bbbbbbbbbbbbbbbbbbbbbbbb" in directory inode 67

And depending on the corruption, the current code will set
a new negative namelen if it turns out that the name itself
starts beyond dir size; it can't be shortened enough.

So, we could fix this up, and choose a new namelen such that
the xfs_dir2_ino8_t and/or xfs_dir2_ino8_t and/or file type
still fits at the end, but we really seem to be reaching the
point of diminishing returns.  The chances that nothing but
the name length is wrong, and that the remaining few
bytes at the end of the dir size are actually correct, seems
awfully small.

Therefore just remove this special case, which I think is
of questionable value.

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

diff --git a/repair/dir2.c b/repair/dir2.c
index 5512e41..b1816f4 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -874,43 +874,20 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
 		}
 
 		if (bad_sfnamelen) {
+			do_warn(
+_("entry #%d %s in shortform dir %" PRIu64),
+				i, junkreason, ino);
+			if (!no_modify)
+				do_warn(_(", junking %d entries\n"),
+					num_entries - i);
+			else
+				do_warn(_(", would junk %d entries\n"),
+					num_entries - i);
 			/*
-			 * if we're really lucky, this is the last entry in
-			 * case we can use the dir size to set the namelen
-			 * value.  otherwise, forget it because we're not going
-			 * to be able to find the next entry.
+			 * don't process the rest of the directory,
+			 * break out of processing loop
 			 */
-			if (i == num_entries - 1)  {
-				namelen = ino_dir_size -
-					((__psint_t) &sfep->name[0] -
-					 (__psint_t) sfp);
-				if (!no_modify)  {
-					do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
-						i, junkreason, ino, namelen);
-					sfep->namelen = namelen;
-					*dino_dirty = 1;
-				} else  {
-					do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
-						i, junkreason, ino, namelen);
-				}
-			} else  {
-				do_warn(
-_("entry #%d %s in shortform dir %" PRIu64),
-					i, junkreason, ino);
-				if (!no_modify)
-					do_warn(_(", junking %d entries\n"),
-						num_entries - i);
-				else
-					do_warn(_(", would junk %d entries\n"),
-						num_entries - i);
-				/*
-				 * don't process the rest of the directory,
-				 * break out of processing looop
-				 */
-				break;
-			}
+			break;
 		}
 
 		/*

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

Re: [PATCH 4/4] xfs_repair: remove last-entry hack in process_sf_dir2

[Brian Foster]

On Tue, Mar 10, 2015 at 05:11:38PM -0400, Eric Sandeen wrote:
> process_sf_dir2() tries to special-case the last entry in
> a short form dir, and salvage it if the name length is wrong
> by using the dir size as a clue to what the length should be.
> 
> However, this doesn't actually work; there is either a 32-bit
> or 64-bit inode after the name, and with dirv3 there's a file
> type in there as well.  Extending the name length to the dir
> size means it overlaps these fields, which often have a 0 in
> the top bits, and then namecheck() fails the result and junks
> it anyway:
> 
> > entry #1 is zero length in shortform dir 67, resetting to 29
> > entry contains illegal character in shortform dir 67
> > junking entry "bbbbbbbbbbbbbbbbbbbbbbbb" in directory inode 67
> 
> And depending on the corruption, the current code will set
> a new negative namelen if it turns out that the name itself
> starts beyond dir size; it can't be shortened enough.
> 
> So, we could fix this up, and choose a new namelen such that
> the xfs_dir2_ino8_t and/or xfs_dir2_ino8_t and/or file type
> still fits at the end, but we really seem to be reaching the
> point of diminishing returns.  The chances that nothing but
> the name length is wrong, and that the remaining few
> bytes at the end of the dir size are actually correct, seems
> awfully small.
> 
> Therefore just remove this special case, which I think is
> of questionable value.
> 

Yeah... Unless I'm missing something, this doesn't seem to be worth the
lines of code. The entry is going to be junked anyways on recent
filesystems. With the optimization being for the last entry in the dir,
removing it shouldn't cause any kind of increase in junked entries in
the event that this occurs. Looks good to me:

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

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 5512e41..b1816f4 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -874,43 +874,20 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
>  		}
>  
>  		if (bad_sfnamelen) {
> +			do_warn(
> +_("entry #%d %s in shortform dir %" PRIu64),
> +				i, junkreason, ino);
> +			if (!no_modify)
> +				do_warn(_(", junking %d entries\n"),
> +					num_entries - i);
> +			else
> +				do_warn(_(", would junk %d entries\n"),
> +					num_entries - i);
>  			/*
> -			 * if we're really lucky, this is the last entry in
> -			 * case we can use the dir size to set the namelen
> -			 * value.  otherwise, forget it because we're not going
> -			 * to be able to find the next entry.
> +			 * don't process the rest of the directory,
> +			 * break out of processing loop
>  			 */
> -			if (i == num_entries - 1)  {
> -				namelen = ino_dir_size -
> -					((__psint_t) &sfep->name[0] -
> -					 (__psint_t) sfp);
> -				if (!no_modify)  {
> -					do_warn(
> -_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
> -						i, junkreason, ino, namelen);
> -					sfep->namelen = namelen;
> -					*dino_dirty = 1;
> -				} else  {
> -					do_warn(
> -_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
> -						i, junkreason, ino, namelen);
> -				}
> -			} else  {
> -				do_warn(
> -_("entry #%d %s in shortform dir %" PRIu64),
> -					i, junkreason, ino);
> -				if (!no_modify)
> -					do_warn(_(", junking %d entries\n"),
> -						num_entries - i);
> -				else
> -					do_warn(_(", would junk %d entries\n"),
> -						num_entries - i);
> -				/*
> -				 * don't process the rest of the directory,
> -				 * break out of processing looop
> -				 */
> -				break;
> -			}
> +			break;
>  		}
>  
>  		/*
> 
> _______________________________________________
> 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