[2/7] repair: don't dirty inodes which are not unlinked
diff mbox series

Message ID 20181030112043.6034-3-david@fromorbit.com
State New
Headers show
Series
  • xfs_repair: scale to 150,000 iops
Related show

Commit Message

Dave Chinner Oct. 30, 2018, 11:20 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

I noticed phase 4 writing back lots of inode buffers during recent
testing. The recent rework of clear_inode() in commit 0724d0f4cb53
("xfs_repair: clear_dinode should simply clear, not check contents")
accidentally caught a call to clear_inode_unlinked() as well,
resulting in all inodes being marked dirty whether then needed
updating or not.

Fix it by reverting the erroneous hunk and adding warnings so taht
this corruption is no longer silently fixed.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 repair/dinode.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Darrick J. Wong Oct. 30, 2018, 5:26 p.m. UTC | #1
On Tue, Oct 30, 2018 at 10:20:38PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> I noticed phase 4 writing back lots of inode buffers during recent
> testing. The recent rework of clear_inode() in commit 0724d0f4cb53
> ("xfs_repair: clear_dinode should simply clear, not check contents")
> accidentally caught a call to clear_inode_unlinked() as well,
> resulting in all inodes being marked dirty whether then needed
> updating or not.
> 
> Fix it by reverting the erroneous hunk and adding warnings so taht
> this corruption is no longer silently fixed.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Oops... that explains a lot. :/

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  repair/dinode.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 379f85cf1268..90400128d4bb 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2675,9 +2675,15 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	 * we're going to find.  check_dups is set to 1 only during
>  	 * phase 4.  Ugly.
>  	 */
> -	if (check_dups && !no_modify) {
> -		clear_dinode_unlinked(mp, dino);
> -		*dirty += 1;
> +	if (check_dups && clear_dinode_unlinked(mp, dino)) {
> +		if (no_modify) {
> +			do_warn(
> +	_("Would clear unlinked_next in inode %" PRIu64 "\n"), lino);
> +		} else  {
> +			do_warn(
> +	_("Cleared unlinked_next in inode %" PRIu64 "\n"), lino);
> +			*dirty += 1;
> +		}
>  	}
>  
>  	/* set type and map type info */
> -- 
> 2.19.1
>
Eric Sandeen Oct. 30, 2018, 8:03 p.m. UTC | #2
On 10/30/18 6:20 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> I noticed phase 4 writing back lots of inode buffers during recent
> testing. The recent rework of clear_inode() in commit 0724d0f4cb53
> ("xfs_repair: clear_dinode should simply clear, not check contents")
> accidentally caught a call to clear_inode_unlinked() as well,
> resulting in all inodes being marked dirty whether then needed
> updating or not.
> 
> Fix it by reverting the erroneous hunk and adding warnings so taht
> this corruption is no longer silently fixed.

I find it confusing that "clear_dinode_unlinked" may or may not clear
unlinked.  Can we change it to actually do as it's named, and move the
test to the (one) caller where it's needed, something like:

===

repair: don't dirty inodes unconditionally when testing unlinked state

Dave noticed phase 4 writing back lots of inode buffers during recent
testing. The recent rework of clear_inode() in commit 0724d0f4cb53
("xfs_repair: clear_dinode should simply clear, not check contents")
accidentally caught a call to clear_inode_unlinked() as well,
resulting in all inodes being marked dirty whether then needed
updating or not.

Fix it by making clear_inode_unlinked unconditionally do the clear
(as was done for clear_inode), and move the test to the caller.
Add warnings as well so that this corruption is no longer silently
fixed.

Reported-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

(I can keep Dave's SOB if preferred with a [sandeen: do it different]
tag if this looks ok and that approach is preferred)

(this also fixes unlinked_next lysdexia in the warnings)

diff --git a/repair/dinode.c b/repair/dinode.c
index 379f85c..c0fa3df 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -125,23 +125,16 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 	return;
 }
 
-static int
+static void
 clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
 {
 
-	if (be32_to_cpu(dino->di_next_unlinked) != NULLAGINO)  {
-		if (!no_modify)
-			dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
-		return(1);
-	}
-
-	return(0);
+	dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
 }
 
 /*
  * this clears the unlinked list too so it should not be called
  * until after the agi unlinked lists are walked in phase 3.
- * returns > zero if the inode has been altered while being cleared
  */
 static void
 clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
@@ -2675,9 +2668,16 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	 * we're going to find.  check_dups is set to 1 only during
 	 * phase 4.  Ugly.
 	 */
-	if (check_dups && !no_modify) {
-		clear_dinode_unlinked(mp, dino);
-		*dirty += 1;
+	if (check_dups && be32_to_cpu(dino->di_next_unlinked) != NULLAGINO) {
+		if (no_modify) {
+			do_warn(
+	_("Would clear next_unlinked in inode %" PRIu64 "\n"), lino);
+		} else  {
+			clear_dinode_unlinked(mp, dino);
+			do_warn(
+	_("Cleared next_unlinked in inode %" PRIu64 "\n"), lino);
+			*dirty += 1;
+		}
 	}
 
 	/* set type and map type info */
Eric Sandeen Oct. 30, 2018, 8:09 p.m. UTC | #3
So I'm not stealing commits: ;)

===

repair: don't dirty inodes unconditionally when testing unlinked state

From: Dave Chinner <dchinner@redhat.com>

I noticed phase 4 writing back lots of inode buffers during recent
testing. The recent rework of clear_inode() in commit 0724d0f4cb53
("xfs_repair: clear_dinode should simply clear, not check contents")
accidentally caught a call to clear_inode_unlinked() as well,
resulting in all inodes being marked dirty whether then needed
updating or not.

Fix it by making clear_inode_unlinked unconditionally do the clear
(as was done for clear_inode), and move the test to the caller.
Add warnings as well so that this corruption is no longer silently
fixed.

Reported-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
[sandeen: rework so clear_inode_unlinked is unconditional]
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
diff --git a/repair/dinode.c b/repair/dinode.c
index 379f85c..f466e77 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -125,23 +125,16 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 	return;
 }
 
-static int
+static void
 clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
 {
 
-	if (be32_to_cpu(dino->di_next_unlinked) != NULLAGINO)  {
-		if (!no_modify)
-			dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
-		return(1);
-	}
-
-	return(0);
+	dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
 }
 
 /*
  * this clears the unlinked list too so it should not be called
  * until after the agi unlinked lists are walked in phase 3.
- * returns > zero if the inode has been altered while being cleared
  */
 static void
 clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
@@ -2675,9 +2668,16 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	 * we're going to find.  check_dups is set to 1 only during
 	 * phase 4.  Ugly.
 	 */
-	if (check_dups && !no_modify) {
-		clear_dinode_unlinked(mp, dino);
-		*dirty += 1;
+	if (check_dups && be32_to_cpu(dino->di_next_unlinked) != NULLAGINO) {
+		if (no_modify) {
+			do_warn(
+	_("Would clear next_unlinked in inode %" PRIu64 "\n"), lino);
+		} else  {
+			clear_dinode_unlinked(mp, dino);
+			do_warn(
+	_("Cleared next_unlinked in inode %" PRIu64 "\n"), lino);
+			*dirty += 1;
+		}
 	}
 
 	/* set type and map type info */
Dave Chinner Oct. 30, 2018, 8:34 p.m. UTC | #4
On Tue, Oct 30, 2018 at 03:09:56PM -0500, Eric Sandeen wrote:
> So I'm not stealing commits: ;)
> 
> ===
> 
> repair: don't dirty inodes unconditionally when testing unlinked state
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> I noticed phase 4 writing back lots of inode buffers during recent
> testing. The recent rework of clear_inode() in commit 0724d0f4cb53
> ("xfs_repair: clear_dinode should simply clear, not check contents")
> accidentally caught a call to clear_inode_unlinked() as well,
> resulting in all inodes being marked dirty whether then needed
> updating or not.
> 
> Fix it by making clear_inode_unlinked unconditionally do the clear
> (as was done for clear_inode), and move the test to the caller.
> Add warnings as well so that this corruption is no longer silently
> fixed.
> 
> Reported-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> [sandeen: rework so clear_inode_unlinked is unconditional]
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 379f85c..f466e77 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -125,23 +125,16 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
>  	return;
>  }
>  
> -static int
> +static void
>  clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
>  {
>  
> -	if (be32_to_cpu(dino->di_next_unlinked) != NULLAGINO)  {
> -		if (!no_modify)
> -			dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
> -		return(1);
> -	}
> -
> -	return(0);
> +	dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
>  }

What's the point of keeping this wrapper now?

Cheers,

Dave.
Eric Sandeen Oct. 30, 2018, 8:40 p.m. UTC | #5
On 10/30/18 3:34 PM, Dave Chinner wrote:
> On Tue, Oct 30, 2018 at 03:09:56PM -0500, Eric Sandeen wrote:
>> So I'm not stealing commits: ;)
>>
>> ===
>>
>> repair: don't dirty inodes unconditionally when testing unlinked state
>>
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> I noticed phase 4 writing back lots of inode buffers during recent
>> testing. The recent rework of clear_inode() in commit 0724d0f4cb53
>> ("xfs_repair: clear_dinode should simply clear, not check contents")
>> accidentally caught a call to clear_inode_unlinked() as well,
>> resulting in all inodes being marked dirty whether then needed
>> updating or not.
>>
>> Fix it by making clear_inode_unlinked unconditionally do the clear
>> (as was done for clear_inode), and move the test to the caller.
>> Add warnings as well so that this corruption is no longer silently
>> fixed.
>>
>> Reported-by: Dave Chinner <dchinner@redhat.com>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>> [sandeen: rework so clear_inode_unlinked is unconditional]
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> diff --git a/repair/dinode.c b/repair/dinode.c
>> index 379f85c..f466e77 100644
>> --- a/repair/dinode.c
>> +++ b/repair/dinode.c
>> @@ -125,23 +125,16 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
>>  	return;
>>  }
>>  
>> -static int
>> +static void
>>  clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
>>  {
>>  
>> -	if (be32_to_cpu(dino->di_next_unlinked) != NULLAGINO)  {
>> -		if (!no_modify)
>> -			dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
>> -		return(1);
>> -	}
>> -
>> -	return(0);
>> +	dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
>>  }
> 
> What's the point of keeping this wrapper now?

Not much other than making clear_dinode() sightly prettier.

Happy to nuke it if preferred.

-Eric
Dave Chinner Oct. 30, 2018, 8:58 p.m. UTC | #6
On Tue, Oct 30, 2018 at 03:40:20PM -0500, Eric Sandeen wrote:
> 
> 
> On 10/30/18 3:34 PM, Dave Chinner wrote:
> > On Tue, Oct 30, 2018 at 03:09:56PM -0500, Eric Sandeen wrote:
> >> So I'm not stealing commits: ;)
> >>
> >> ===
> >>
> >> repair: don't dirty inodes unconditionally when testing unlinked state
> >>
> >> From: Dave Chinner <dchinner@redhat.com>
> >>
> >> I noticed phase 4 writing back lots of inode buffers during recent
> >> testing. The recent rework of clear_inode() in commit 0724d0f4cb53
> >> ("xfs_repair: clear_dinode should simply clear, not check contents")
> >> accidentally caught a call to clear_inode_unlinked() as well,
> >> resulting in all inodes being marked dirty whether then needed
> >> updating or not.
> >>
> >> Fix it by making clear_inode_unlinked unconditionally do the clear
> >> (as was done for clear_inode), and move the test to the caller.
> >> Add warnings as well so that this corruption is no longer silently
> >> fixed.
> >>
> >> Reported-by: Dave Chinner <dchinner@redhat.com>
> >> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> >> [sandeen: rework so clear_inode_unlinked is unconditional]
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> diff --git a/repair/dinode.c b/repair/dinode.c
> >> index 379f85c..f466e77 100644
> >> --- a/repair/dinode.c
> >> +++ b/repair/dinode.c
> >> @@ -125,23 +125,16 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
> >>  	return;
> >>  }
> >>  
> >> -static int
> >> +static void
> >>  clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
> >>  {
> >>  
> >> -	if (be32_to_cpu(dino->di_next_unlinked) != NULLAGINO)  {
> >> -		if (!no_modify)
> >> -			dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
> >> -		return(1);
> >> -	}
> >> -
> >> -	return(0);
> >> +	dino->di_next_unlinked = cpu_to_be32(NULLAGINO);
> >>  }
> > 
> > What's the point of keeping this wrapper now?
> 
> Not much other than making clear_dinode() sightly prettier.
> 
> Happy to nuke it if preferred.

The code is already a mess with all the no_modify/fix branches and
warnings, so it really doesn't matter that much. Keep it as is...

Cheers,

Dave.

Patch
diff mbox series

diff --git a/repair/dinode.c b/repair/dinode.c
index 379f85cf1268..90400128d4bb 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2675,9 +2675,15 @@  _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	 * we're going to find.  check_dups is set to 1 only during
 	 * phase 4.  Ugly.
 	 */
-	if (check_dups && !no_modify) {
-		clear_dinode_unlinked(mp, dino);
-		*dirty += 1;
+	if (check_dups && clear_dinode_unlinked(mp, dino)) {
+		if (no_modify) {
+			do_warn(
+	_("Would clear unlinked_next in inode %" PRIu64 "\n"), lino);
+		} else  {
+			do_warn(
+	_("Cleared unlinked_next in inode %" PRIu64 "\n"), lino);
+			*dirty += 1;
+		}
 	}
 
 	/* set type and map type info */