[PATCH] xfs_repair: notify user if free inodes contain errors

* [PATCH] xfs_repair: notify user if free inodes contain errors
@ 2018-04-14  1:00 Eric Sandeen
  2018-04-17 10:07 ` Carlos Maiolino
  0 siblings, 1 reply; 2+ messages in thread
From: Eric Sandeen @ 2018-04-14  1:00 UTC (permalink / raw)
  To: linux-xfs

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.

This also requires tweaking or removing some tests for free inode
state to match what the kernel does on either initial allocation or
eventual free after re-use.

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

diff --git a/repair/dinode.c b/repair/dinode.c
index 9af4f05..1d7659f 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -117,6 +117,15 @@ _("would have cleared inode %" PRIu64 " attributes\n"), ino_num);
 	return(1);
 }
 
+/*
+ * Clear the on-disk inode, and also test for inconsistencies along the way.
+ * In some cases this is to wipe out a bad inode, in other cases it is
+ * validating an unused but allocated inode on disk.
+ * The kernel leaves unused inodes on disk in slightly different states
+ * depending on whether it is freshly allocated or used and then freed,
+ * so this function only validates deterministic fields set by the
+ * kernel in i.e. xfs_ialloc_inode_init or  xfs_ifree.
+ */
 static int
 clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 {
@@ -159,17 +168,21 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 		dinoc->di_forkoff = 0;
 	}
 
-	if (dinoc->di_format != XFS_DINODE_FMT_EXTENTS)  {
+	/* Brand new allocated, never used inodes have format 0 */
+	if (dinoc->di_format &&
+	    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)  {
+	if (dinoc->di_aformat &&
+	    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)  {
+	if (S_ISREG(be16_to_cpu(dinoc->di_mode)) &&
+	    be64_to_cpu(dinoc->di_size) != 0)  {
 		__dirty_no_modify_ret(dirty);
 		dinoc->di_size = 0;
 	}
@@ -227,15 +240,7 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num)
 		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;
-	}
+	/* We leave di_lsn, di_changecount alone, as does the kernel */
 
 	return dirty;
 }
@@ -2358,7 +2363,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) {
@@ -2369,10 +2374,17 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 			/*
 			 * easy case, inode free -- inode and map agree, clear
 			 * it just in case to ensure that format, etc. are
-			 * set correctly
+			 * set correctly.  no_modify is tested in clear_dinode.
 			 */
-			if (!no_modify)
-				*dirty += clear_dinode(mp, dino, lino);
+			if (clear_dinode(mp, dino, lino) != 0) {
+				do_warn(
+	_("free inode %" PRIu64 " contains errors, "), lino);
+				if (!no_modify) {
+					do_warn(_("corrected\n"));
+					*dirty += 1;
+				} else
+					do_warn(_("would correct\n"));
+			}
 			*used = is_free;
 			return 0;
 		}



^ permalink raw reply related	[flat|nested] 2+ messages in thread