[PATCH 0/2 V3] xfs_repair: rework inode clearing and free inode validation

[PATCH 0/2 V3] xfs_repair: rework inode clearing and free inode validation

[Eric Sandeen]

Warn the user if any free inodes contain errors, and set exit code
in the process.  Do this by running the existing dinode verifier and
clearing / warning if it fails.

2nd patch separates out the inode validation from the clearing function

There is a functional change here which I'm on the fence about; free
inodes are now only cleared if they won't pass the dinode verifier,
which is the only thing the kernel would squawk about.  But that's
a subset of the clear_inode checks.

The old clear_dinode function checked a lot of things that the
verifier simply doesn't care about, but I think it's ok for
those tests to go away; if the kernel does care about, say, extent
count on a free inode, the verifier should test it.

[PATCH 1/2 V3] xfs_repair: notify user if free inodes contain errors

[Eric Sandeen]

xfs_repair checks allocated but unused (free) inodes in on-disk clusters,
and up until now silently repairs any errors, and as a result does not
alter exit status if errors are found.
    
The in-kernel verifiers will be noisy about these errors and instruct
the user to run repair, so it's best if repair is explicit about any
fixes it makes as a result.

To ensure we catch anything the kernel would complain about, re-use
xfs_dinode_verify to determine whether we must clear a free inode.

Note, however, that the verifier contains only a subset of the checks
currently in clear_dinode.  This should be ok; if it's not, the checks
should be added to the verifier in any case.

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

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index a98483b..05c06a7 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -134,6 +134,7 @@
 #define xfs_symlink_hdr_ok		libxfs_symlink_hdr_ok
 
 #define xfs_verify_cksum		libxfs_verify_cksum
+#define xfs_dinode_verify		libxfs_dinode_verify
 
 #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
 #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
diff --git a/repair/dinode.c b/repair/dinode.c
index d36338f..c0db15a 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 		 */
 		if (was_free) {
 			/*
-			 * easy case, inode free -- inode and map agree, clear
+			 * easy case, inode free -- inode and map agree, check
 			 * it just in case to ensure that format, etc. are
 			 * set correctly
 			 */
-			if (!no_modify)
-				*dirty += clear_dinode(mp, dino, lino);
+			if (libxfs_dinode_verify(mp, lino, dino) != NULL) {
+				do_warn(
+ _("free inode %" PRIu64 " contains errors, "), lino);
+				if (!no_modify) {
+					*dirty += clear_dinode(mp, dino, lino);
+					do_warn(_("corrected\n"));
+				} else {
+					do_warn(_("would correct\n"));
+				}
+			}
 			*used = is_free;
 			return 0;
 		}

[PATCH 2/2 V3] xfs_repair: clear_dinode should simply clear, not check contents

[Eric Sandeen]

Today clear_inode performs 2 separate tasks - it clears a disk inode
when the calling code detects an inconsistency, and it also validates
free inodes to some extent by checking each field before zeroing it
out.  This leads to unnecessary checks in the former case where we
just want to zap the inode, and duplicates some of the existing
verifier checks in the latter case.
    
Now that we're using xfs_dinode_verify to validate free inodes,
there's no reason to repeat all the checks inside clear_inode_core.
Drop them all and simply clear it when told to do so.

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

diff --git a/repair/dinode.c b/repair/dinode.c
index c0db15a..5b9fd9a 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -117,137 +117,24 @@ _("would have cleared inode %" PRIu64 " attributes\n"), ino_num);
 	return(1);
 }
 
-static int
+static void
 clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 {
-	int dirty = 0;
-	int i;
-
-#define __dirty_no_modify_ret(dirty) \
-	({ (dirty) = 1; if (no_modify) return 1; })
-
-	if (be16_to_cpu(dinoc->di_magic) != XFS_DINODE_MAGIC)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
-	}
-
-	if (!libxfs_dinode_good_version(mp, dinoc->di_version)) {
-		__dirty_no_modify_ret(dirty);
-		if (xfs_sb_version_hascrc(&mp->m_sb))
-			dinoc->di_version = 3;
-		else
-			dinoc->di_version = 2;
-	}
-
-	if (be16_to_cpu(dinoc->di_mode) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_mode = 0;
-	}
-
-	if (be16_to_cpu(dinoc->di_flags) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_flags = 0;
-	}
-
-	if (be32_to_cpu(dinoc->di_dmevmask) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_dmevmask = 0;
-	}
-
-	if (dinoc->di_forkoff != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_forkoff = 0;
-	}
-
-	if (dinoc->di_format != XFS_DINODE_FMT_EXTENTS)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
-	}
-
-	if (dinoc->di_aformat != XFS_DINODE_FMT_EXTENTS)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
-	}
-
-	if (be64_to_cpu(dinoc->di_size) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_size = 0;
-	}
-
-	if (be64_to_cpu(dinoc->di_nblocks) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_nblocks = 0;
-	}
-
-	if (be16_to_cpu(dinoc->di_onlink) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_onlink = 0;
-	}
-
-	if (be32_to_cpu(dinoc->di_nextents) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_nextents = 0;
-	}
-
-	if (be16_to_cpu(dinoc->di_anextents) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_anextents = 0;
-	}
-
-	if (be32_to_cpu(dinoc->di_extsize) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_extsize = 0;
-	}
-
-	if (dinoc->di_version > 1 &&
-			be32_to_cpu(dinoc->di_nlink) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_nlink = 0;
-	}
-
+	memset(dinoc, 0, sizeof(*dinoc));
+	dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
+	if (xfs_sb_version_hascrc(&mp->m_sb))
+		dinoc->di_version = 3;
+	else
+		dinoc->di_version = 2;
+	dinoc->di_gen = cpu_to_be32(random());
+	dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
+	dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
 	/* we are done for version 1/2 inodes */
 	if (dinoc->di_version < 3)
-		return dirty;
-
-	if (be64_to_cpu(dinoc->di_ino) != ino_num) {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_ino = cpu_to_be64(ino_num);
-	}
-
-	if (platform_uuid_compare(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid)) {
-		__dirty_no_modify_ret(dirty);
-		platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
-	}
-
-	for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) {
-		if (dinoc->di_pad2[i] != 0) {
-			__dirty_no_modify_ret(dirty);
-			memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2));
-			break;
-		}
-	}
-
-	if (be64_to_cpu(dinoc->di_flags2) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_flags2 = 0;
-	}
-
-	if (be64_to_cpu(dinoc->di_lsn) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_lsn = 0;
-	}
-
-	if (be64_to_cpu(dinoc->di_changecount) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_changecount = 0;
-	}
-
-	if (be32_to_cpu(dinoc->di_cowextsize) != 0)  {
-		__dirty_no_modify_ret(dirty);
-		dinoc->di_cowextsize = 0;
-	}
-
-	return dirty;
+		return;
+	dinoc->di_ino = cpu_to_be64(ino_num);
+	platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
+	return;
 }
 
 static int
@@ -268,21 +155,15 @@ clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
  * until after the agi unlinked lists are walked in phase 3.
  * returns > zero if the inode has been altered while being cleared
  */
-static int
+static void
 clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
 {
-	int dirty;
-
-	dirty = clear_dinode_core(mp, dino, ino_num);
-	dirty += clear_dinode_unlinked(mp, dino);
+	clear_dinode_core(mp, dino, ino_num);
+	clear_dinode_unlinked(mp, dino);
 
 	/* and clear the forks */
-
-	if (dirty && !no_modify)
-		memset(XFS_DFORK_DPTR(dino), 0,
-		       XFS_LITINO(mp, dino->di_version));
-
-	return(dirty);
+	memset(XFS_DFORK_DPTR(dino), 0, XFS_LITINO(mp, dino->di_version));
+	return;
 }
 
 
@@ -2140,8 +2021,8 @@ process_inode_data_fork(
 	if (err)  {
 		do_warn(_("bad data fork in inode %" PRIu64 "\n"), lino);
 		if (!no_modify)  {
-			*dirty += clear_dinode(mp, dino, lino);
-			ASSERT(*dirty > 0);
+			clear_dinode(mp, dino, lino);
+			*dirty += 1;
 		}
 		return 1;
 	}
@@ -2551,7 +2432,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	}
 
 	/*
-	 * if not in verify mode, check to sii if the inode and imap
+	 * if not in verify mode, check to see if the inode and imap
 	 * agree that the inode is free
 	 */
 	if (!verify_mode && di_mode == 0) {
@@ -2568,8 +2449,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 				do_warn(
  _("free inode %" PRIu64 " contains errors, "), lino);
 				if (!no_modify) {
-					*dirty += clear_dinode(mp, dino, lino);
+					clear_dinode(mp, dino, lino);
 					do_warn(_("corrected\n"));
+					*dirty += 1;
 				} else {
 					do_warn(_("would correct\n"));
 				}
@@ -2586,7 +2468,8 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	_("imap claims a free inode %" PRIu64 " is in use, "), lino);
 		if (!no_modify)  {
 			do_warn(_("correcting imap and clearing inode\n"));
-			*dirty += clear_dinode(mp, dino, lino);
+			clear_dinode(mp, dino, lino);
+			*dirty += 1;
 			retval = 1;
 		} else
 			do_warn(_("would correct imap and clear inode\n"));
@@ -2804,8 +2687,10 @@ _("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)
-		*dirty += clear_dinode_unlinked(mp, dino);
+	if (check_dups && !no_modify) {
+		clear_dinode_unlinked(mp, dino);
+		*dirty += 1;
+	}
 
 	/* set type and map type info */
 
@@ -3024,8 +2909,8 @@ _("Cannot have CoW extent size of zero on cowextsize inode %" PRIu64 ", "),
 
 clear_bad_out:
 	if (!no_modify)  {
-		*dirty += clear_dinode(mp, dino, lino);
-		ASSERT(*dirty > 0);
+		clear_dinode(mp, dino, lino);
+		*dirty += 1;
 	}
 bad_out:
 	*used = is_free;

Re: [PATCH 1/2 V3] xfs_repair: notify user if free inodes contain errors

[Eric Sandeen]

On 7/23/18 7:34 PM, Eric Sandeen wrote:
> xfs_repair checks allocated but unused (free) inodes in on-disk clusters,
> and up until now silently repairs any errors, and as a result does not
> alter exit status if errors are found.
>     
> The in-kernel verifiers will be noisy about these errors and instruct
> the user to run repair, so it's best if repair is explicit about any
> fixes it makes as a result.
> 
> To ensure we catch anything the kernel would complain about, re-use
> xfs_dinode_verify to determine whether we must clear a free inode.
> 
> Note, however, that the verifier contains only a subset of the checks
> currently in clear_dinode.  This should be ok; if it's not, the checks
> should be added to the verifier in any case.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Ugh, dammit, pulled the trigger too soon, tests were looking fine until
the last few.  Need to look at this more.

> ---
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index a98483b..05c06a7 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -134,6 +134,7 @@
>  #define xfs_symlink_hdr_ok		libxfs_symlink_hdr_ok
>  
>  #define xfs_verify_cksum		libxfs_verify_cksum
> +#define xfs_dinode_verify		libxfs_dinode_verify
>  
>  #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
>  #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
> diff --git a/repair/dinode.c b/repair/dinode.c
> index d36338f..c0db15a 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  		 */
>  		if (was_free) {
>  			/*
> -			 * easy case, inode free -- inode and map agree, clear
> +			 * easy case, inode free -- inode and map agree, check
>  			 * it just in case to ensure that format, etc. are
>  			 * set correctly
>  			 */
> -			if (!no_modify)
> -				*dirty += clear_dinode(mp, dino, lino);
> +			if (libxfs_dinode_verify(mp, lino, dino) != NULL) {
> +				do_warn(
> + _("free inode %" PRIu64 " contains errors, "), lino);
> +				if (!no_modify) {
> +					*dirty += clear_dinode(mp, dino, lino);
> +					do_warn(_("corrected\n"));
> +				} else {
> +					do_warn(_("would correct\n"));
> +				}
> +			}
>  			*used = is_free;
>  			return 0;
>  		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2 V3] xfs_repair: notify user if free inodes contain errors

[Eric Sandeen]

On 7/23/18 9:35 PM, Eric Sandeen wrote:
> On 7/23/18 7:34 PM, Eric Sandeen wrote:
>> xfs_repair checks allocated but unused (free) inodes in on-disk clusters,
>> and up until now silently repairs any errors, and as a result does not
>> alter exit status if errors are found.
>>     
>> The in-kernel verifiers will be noisy about these errors and instruct
>> the user to run repair, so it's best if repair is explicit about any
>> fixes it makes as a result.
>>
>> To ensure we catch anything the kernel would complain about, re-use
>> xfs_dinode_verify to determine whether we must clear a free inode.
>>
>> Note, however, that the verifier contains only a subset of the checks
>> currently in clear_dinode.  This should be ok; if it's not, the checks
>> should be added to the verifier in any case.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Ugh, dammit, pulled the trigger too soon, tests were looking fine until
> the last few.  Need to look at this more.

With the free inode extent validator fix I sent earlier today I think it's
safe to resume normal review activities for these 2 patches.  :)

Thanks,
-Eric

Re: [PATCH 1/2 V3] xfs_repair: notify user if free inodes contain errors

[Carlos Maiolino]

On Mon, Jul 23, 2018 at 07:34:13PM -0700, Eric Sandeen wrote:
> xfs_repair checks allocated but unused (free) inodes in on-disk clusters,
> and up until now silently repairs any errors, and as a result does not
> alter exit status if errors are found.
>     
> The in-kernel verifiers will be noisy about these errors and instruct
> the user to run repair, so it's best if repair is explicit about any
> fixes it makes as a result.
> 
> To ensure we catch anything the kernel would complain about, re-use
> xfs_dinode_verify to determine whether we must clear a free inode.
> 
> Note, however, that the verifier contains only a subset of the checks
> currently in clear_dinode.  This should be ok; if it's not, the checks
> should be added to the verifier in any case.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 

Looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index a98483b..05c06a7 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -134,6 +134,7 @@
>  #define xfs_symlink_hdr_ok		libxfs_symlink_hdr_ok
>  
>  #define xfs_verify_cksum		libxfs_verify_cksum
> +#define xfs_dinode_verify		libxfs_dinode_verify
>  
>  #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
>  #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
> diff --git a/repair/dinode.c b/repair/dinode.c
> index d36338f..c0db15a 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  		 */
>  		if (was_free) {
>  			/*
> -			 * easy case, inode free -- inode and map agree, clear
> +			 * easy case, inode free -- inode and map agree, check
>  			 * it just in case to ensure that format, etc. are
>  			 * set correctly
>  			 */
> -			if (!no_modify)
> -				*dirty += clear_dinode(mp, dino, lino);
> +			if (libxfs_dinode_verify(mp, lino, dino) != NULL) {
> +				do_warn(
> + _("free inode %" PRIu64 " contains errors, "), lino);
> +				if (!no_modify) {
> +					*dirty += clear_dinode(mp, dino, lino);
> +					do_warn(_("corrected\n"));
> +				} else {
> +					do_warn(_("would correct\n"));
> +				}
> +			}
>  			*used = is_free;
>  			return 0;
>  		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH 2/2 V3] xfs_repair: clear_dinode should simply clear, not check contents

[Carlos Maiolino]

On Mon, Jul 23, 2018 at 07:50:13PM -0700, Eric Sandeen wrote:
> Today clear_inode performs 2 separate tasks - it clears a disk inode
> when the calling code detects an inconsistency, and it also validates
> free inodes to some extent by checking each field before zeroing it
> out.  This leads to unnecessary checks in the former case where we
> just want to zap the inode, and duplicates some of the existing
> verifier checks in the latter case.
>     
> Now that we're using xfs_dinode_verify to validate free inodes,
> there's no reason to repeat all the checks inside clear_inode_core.
> Drop them all and simply clear it when told to do so.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks good.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c0db15a..5b9fd9a 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -117,137 +117,24 @@ _("would have cleared inode %" PRIu64 " attributes\n"), ino_num);
>  	return(1);
>  }
>  
> -static int
> +static void
>  clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
>  {
> -	int dirty = 0;
> -	int i;
> -
> -#define __dirty_no_modify_ret(dirty) \
> -	({ (dirty) = 1; if (no_modify) return 1; })
> -
> -	if (be16_to_cpu(dinoc->di_magic) != XFS_DINODE_MAGIC)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> -	}
> -
> -	if (!libxfs_dinode_good_version(mp, dinoc->di_version)) {
> -		__dirty_no_modify_ret(dirty);
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			dinoc->di_version = 3;
> -		else
> -			dinoc->di_version = 2;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_mode) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_mode = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_flags) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_flags = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_dmevmask) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_dmevmask = 0;
> -	}
> -
> -	if (dinoc->di_forkoff != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_forkoff = 0;
> -	}
> -
> -	if (dinoc->di_format != XFS_DINODE_FMT_EXTENTS)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
> -	}
> -
> -	if (dinoc->di_aformat != XFS_DINODE_FMT_EXTENTS)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_size) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_size = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_nblocks) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nblocks = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_onlink) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_onlink = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_nextents) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nextents = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_anextents) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_anextents = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_extsize) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_extsize = 0;
> -	}
> -
> -	if (dinoc->di_version > 1 &&
> -			be32_to_cpu(dinoc->di_nlink) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nlink = 0;
> -	}
> -
> +	memset(dinoc, 0, sizeof(*dinoc));
> +	dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> +	if (xfs_sb_version_hascrc(&mp->m_sb))
> +		dinoc->di_version = 3;
> +	else
> +		dinoc->di_version = 2;
> +	dinoc->di_gen = cpu_to_be32(random());
> +	dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
> +	dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
>  	/* we are done for version 1/2 inodes */
>  	if (dinoc->di_version < 3)
> -		return dirty;
> -
> -	if (be64_to_cpu(dinoc->di_ino) != ino_num) {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_ino = cpu_to_be64(ino_num);
> -	}
> -
> -	if (platform_uuid_compare(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid)) {
> -		__dirty_no_modify_ret(dirty);
> -		platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
> -	}
> -
> -	for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) {
> -		if (dinoc->di_pad2[i] != 0) {
> -			__dirty_no_modify_ret(dirty);
> -			memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2));
> -			break;
> -		}
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_flags2) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_flags2 = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_lsn) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_lsn = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_changecount) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_changecount = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_cowextsize) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_cowextsize = 0;
> -	}
> -
> -	return dirty;
> +		return;
> +	dinoc->di_ino = cpu_to_be64(ino_num);
> +	platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
> +	return;
>  }
>  
>  static int
> @@ -268,21 +155,15 @@ clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
>   * until after the agi unlinked lists are walked in phase 3.
>   * returns > zero if the inode has been altered while being cleared
>   */
> -static int
> +static void
>  clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
>  {
> -	int dirty;
> -
> -	dirty = clear_dinode_core(mp, dino, ino_num);
> -	dirty += clear_dinode_unlinked(mp, dino);
> +	clear_dinode_core(mp, dino, ino_num);
> +	clear_dinode_unlinked(mp, dino);
>  
>  	/* and clear the forks */
> -
> -	if (dirty && !no_modify)
> -		memset(XFS_DFORK_DPTR(dino), 0,
> -		       XFS_LITINO(mp, dino->di_version));
> -
> -	return(dirty);
> +	memset(XFS_DFORK_DPTR(dino), 0, XFS_LITINO(mp, dino->di_version));
> +	return;
>  }
>  
>  
> @@ -2140,8 +2021,8 @@ process_inode_data_fork(
>  	if (err)  {
>  		do_warn(_("bad data fork in inode %" PRIu64 "\n"), lino);
>  		if (!no_modify)  {
> -			*dirty += clear_dinode(mp, dino, lino);
> -			ASSERT(*dirty > 0);
> +			clear_dinode(mp, dino, lino);
> +			*dirty += 1;
>  		}
>  		return 1;
>  	}
> @@ -2551,7 +2432,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	}
>  
>  	/*
> -	 * if not in verify mode, check to sii if the inode and imap
> +	 * if not in verify mode, check to see if the inode and imap
>  	 * agree that the inode is free
>  	 */
>  	if (!verify_mode && di_mode == 0) {
> @@ -2568,8 +2449,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  				do_warn(
>   _("free inode %" PRIu64 " contains errors, "), lino);
>  				if (!no_modify) {
> -					*dirty += clear_dinode(mp, dino, lino);
> +					clear_dinode(mp, dino, lino);
>  					do_warn(_("corrected\n"));
> +					*dirty += 1;
>  				} else {
>  					do_warn(_("would correct\n"));
>  				}
> @@ -2586,7 +2468,8 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	_("imap claims a free inode %" PRIu64 " is in use, "), lino);
>  		if (!no_modify)  {
>  			do_warn(_("correcting imap and clearing inode\n"));
> -			*dirty += clear_dinode(mp, dino, lino);
> +			clear_dinode(mp, dino, lino);
> +			*dirty += 1;
>  			retval = 1;
>  		} else
>  			do_warn(_("would correct imap and clear inode\n"));
> @@ -2804,8 +2687,10 @@ _("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)
> -		*dirty += clear_dinode_unlinked(mp, dino);
> +	if (check_dups && !no_modify) {
> +		clear_dinode_unlinked(mp, dino);
> +		*dirty += 1;
> +	}
>  
>  	/* set type and map type info */
>  
> @@ -3024,8 +2909,8 @@ _("Cannot have CoW extent size of zero on cowextsize inode %" PRIu64 ", "),
>  
>  clear_bad_out:
>  	if (!no_modify)  {
> -		*dirty += clear_dinode(mp, dino, lino);
> -		ASSERT(*dirty > 0);
> +		clear_dinode(mp, dino, lino);
> +		*dirty += 1;
>  	}
>  bad_out:
>  	*used = is_free;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH 1/2 V3] xfs_repair: notify user if free inodes contain errors

[Darrick J. Wong]

On Mon, Jul 23, 2018 at 07:34:13PM -0700, Eric Sandeen wrote:
> xfs_repair checks allocated but unused (free) inodes in on-disk clusters,
> and up until now silently repairs any errors, and as a result does not
> alter exit status if errors are found.
>     
> The in-kernel verifiers will be noisy about these errors and instruct
> the user to run repair, so it's best if repair is explicit about any
> fixes it makes as a result.
> 
> To ensure we catch anything the kernel would complain about, re-use
> xfs_dinode_verify to determine whether we must clear a free inode.
> 
> Note, however, that the verifier contains only a subset of the checks
> currently in clear_dinode.  This should be ok; if it's not, the checks
> should be added to the verifier in any case.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

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

--D

> ---
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index a98483b..05c06a7 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -134,6 +134,7 @@
>  #define xfs_symlink_hdr_ok		libxfs_symlink_hdr_ok
>  
>  #define xfs_verify_cksum		libxfs_verify_cksum
> +#define xfs_dinode_verify		libxfs_dinode_verify
>  
>  #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
>  #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
> diff --git a/repair/dinode.c b/repair/dinode.c
> index d36338f..c0db15a 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2560,12 +2560,20 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  		 */
>  		if (was_free) {
>  			/*
> -			 * easy case, inode free -- inode and map agree, clear
> +			 * easy case, inode free -- inode and map agree, check
>  			 * it just in case to ensure that format, etc. are
>  			 * set correctly
>  			 */
> -			if (!no_modify)
> -				*dirty += clear_dinode(mp, dino, lino);
> +			if (libxfs_dinode_verify(mp, lino, dino) != NULL) {
> +				do_warn(
> + _("free inode %" PRIu64 " contains errors, "), lino);
> +				if (!no_modify) {
> +					*dirty += clear_dinode(mp, dino, lino);
> +					do_warn(_("corrected\n"));
> +				} else {
> +					do_warn(_("would correct\n"));
> +				}
> +			}
>  			*used = is_free;
>  			return 0;
>  		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 V3] xfs_repair: clear_dinode should simply clear, not check contents

[Darrick J. Wong]

On Mon, Jul 23, 2018 at 07:50:13PM -0700, Eric Sandeen wrote:
> Today clear_inode performs 2 separate tasks - it clears a disk inode
> when the calling code detects an inconsistency, and it also validates
> free inodes to some extent by checking each field before zeroing it
> out.  This leads to unnecessary checks in the former case where we
> just want to zap the inode, and duplicates some of the existing
> verifier checks in the latter case.
>     
> Now that we're using xfs_dinode_verify to validate free inodes,
> there's no reason to repeat all the checks inside clear_inode_core.
> Drop them all and simply clear it when told to do so.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

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

--D

> ---
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c0db15a..5b9fd9a 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -117,137 +117,24 @@ _("would have cleared inode %" PRIu64 " attributes\n"), ino_num);
>  	return(1);
>  }
>  
> -static int
> +static void
>  clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
>  {
> -	int dirty = 0;
> -	int i;
> -
> -#define __dirty_no_modify_ret(dirty) \
> -	({ (dirty) = 1; if (no_modify) return 1; })
> -
> -	if (be16_to_cpu(dinoc->di_magic) != XFS_DINODE_MAGIC)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> -	}
> -
> -	if (!libxfs_dinode_good_version(mp, dinoc->di_version)) {
> -		__dirty_no_modify_ret(dirty);
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> -			dinoc->di_version = 3;
> -		else
> -			dinoc->di_version = 2;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_mode) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_mode = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_flags) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_flags = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_dmevmask) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_dmevmask = 0;
> -	}
> -
> -	if (dinoc->di_forkoff != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_forkoff = 0;
> -	}
> -
> -	if (dinoc->di_format != XFS_DINODE_FMT_EXTENTS)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
> -	}
> -
> -	if (dinoc->di_aformat != XFS_DINODE_FMT_EXTENTS)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_size) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_size = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_nblocks) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nblocks = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_onlink) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_onlink = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_nextents) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nextents = 0;
> -	}
> -
> -	if (be16_to_cpu(dinoc->di_anextents) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_anextents = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_extsize) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_extsize = 0;
> -	}
> -
> -	if (dinoc->di_version > 1 &&
> -			be32_to_cpu(dinoc->di_nlink) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_nlink = 0;
> -	}
> -
> +	memset(dinoc, 0, sizeof(*dinoc));
> +	dinoc->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> +	if (xfs_sb_version_hascrc(&mp->m_sb))
> +		dinoc->di_version = 3;
> +	else
> +		dinoc->di_version = 2;
> +	dinoc->di_gen = cpu_to_be32(random());
> +	dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
> +	dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
>  	/* we are done for version 1/2 inodes */
>  	if (dinoc->di_version < 3)
> -		return dirty;
> -
> -	if (be64_to_cpu(dinoc->di_ino) != ino_num) {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_ino = cpu_to_be64(ino_num);
> -	}
> -
> -	if (platform_uuid_compare(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid)) {
> -		__dirty_no_modify_ret(dirty);
> -		platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
> -	}
> -
> -	for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) {
> -		if (dinoc->di_pad2[i] != 0) {
> -			__dirty_no_modify_ret(dirty);
> -			memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2));
> -			break;
> -		}
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_flags2) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_flags2 = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_lsn) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_lsn = 0;
> -	}
> -
> -	if (be64_to_cpu(dinoc->di_changecount) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_changecount = 0;
> -	}
> -
> -	if (be32_to_cpu(dinoc->di_cowextsize) != 0)  {
> -		__dirty_no_modify_ret(dirty);
> -		dinoc->di_cowextsize = 0;
> -	}
> -
> -	return dirty;
> +		return;
> +	dinoc->di_ino = cpu_to_be64(ino_num);
> +	platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
> +	return;
>  }
>  
>  static int
> @@ -268,21 +155,15 @@ clear_dinode_unlinked(xfs_mount_t *mp, xfs_dinode_t *dino)
>   * until after the agi unlinked lists are walked in phase 3.
>   * returns > zero if the inode has been altered while being cleared
>   */
> -static int
> +static void
>  clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
>  {
> -	int dirty;
> -
> -	dirty = clear_dinode_core(mp, dino, ino_num);
> -	dirty += clear_dinode_unlinked(mp, dino);
> +	clear_dinode_core(mp, dino, ino_num);
> +	clear_dinode_unlinked(mp, dino);
>  
>  	/* and clear the forks */
> -
> -	if (dirty && !no_modify)
> -		memset(XFS_DFORK_DPTR(dino), 0,
> -		       XFS_LITINO(mp, dino->di_version));
> -
> -	return(dirty);
> +	memset(XFS_DFORK_DPTR(dino), 0, XFS_LITINO(mp, dino->di_version));
> +	return;
>  }
>  
>  
> @@ -2140,8 +2021,8 @@ process_inode_data_fork(
>  	if (err)  {
>  		do_warn(_("bad data fork in inode %" PRIu64 "\n"), lino);
>  		if (!no_modify)  {
> -			*dirty += clear_dinode(mp, dino, lino);
> -			ASSERT(*dirty > 0);
> +			clear_dinode(mp, dino, lino);
> +			*dirty += 1;
>  		}
>  		return 1;
>  	}
> @@ -2551,7 +2432,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	}
>  
>  	/*
> -	 * if not in verify mode, check to sii if the inode and imap
> +	 * if not in verify mode, check to see if the inode and imap
>  	 * agree that the inode is free
>  	 */
>  	if (!verify_mode && di_mode == 0) {
> @@ -2568,8 +2449,9 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  				do_warn(
>   _("free inode %" PRIu64 " contains errors, "), lino);
>  				if (!no_modify) {
> -					*dirty += clear_dinode(mp, dino, lino);
> +					clear_dinode(mp, dino, lino);
>  					do_warn(_("corrected\n"));
> +					*dirty += 1;
>  				} else {
>  					do_warn(_("would correct\n"));
>  				}
> @@ -2586,7 +2468,8 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	_("imap claims a free inode %" PRIu64 " is in use, "), lino);
>  		if (!no_modify)  {
>  			do_warn(_("correcting imap and clearing inode\n"));
> -			*dirty += clear_dinode(mp, dino, lino);
> +			clear_dinode(mp, dino, lino);
> +			*dirty += 1;
>  			retval = 1;
>  		} else
>  			do_warn(_("would correct imap and clear inode\n"));
> @@ -2804,8 +2687,10 @@ _("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)
> -		*dirty += clear_dinode_unlinked(mp, dino);
> +	if (check_dups && !no_modify) {
> +		clear_dinode_unlinked(mp, dino);
> +		*dirty += 1;
> +	}
>  
>  	/* set type and map type info */
>  
> @@ -3024,8 +2909,8 @@ _("Cannot have CoW extent size of zero on cowextsize inode %" PRIu64 ", "),
>  
>  clear_bad_out:
>  	if (!no_modify)  {
> -		*dirty += clear_dinode(mp, dino, lino);
> -		ASSERT(*dirty > 0);
> +		clear_dinode(mp, dino, lino);
> +		*dirty += 1;
>  	}
>  bad_out:
>  	*used = is_free;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html