* [PATCH 1/2] xfs_repair: clear_dinode should only clear, not check contents
2018-05-30 15:49 [PATCH 0/2] xfs_repair: rework inode clearing and free inode validation Eric Sandeen
@ 2018-05-30 15:51 ` Eric Sandeen
2018-05-30 15:51 ` [PATCH 2/2] xfs_repair: notify user if free inodes contain errors Eric Sandeen
1 sibling, 0 replies; 3+ messages in thread
From: Eric Sandeen @ 2018-05-30 15:51 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs
clear_dinode is a strange thing; it's called primarily when other code
detects errors and wishes to clear the inode, but it also re-checks each
piece of the inode before clearing it. For one, this means that all the
checks that call it need to be re-performed in the clearing function, or
it may do nothing at all.
It's also used to check and clear free inodes; for that purpose going
forward we'll need to first validate a free inode, and then clear_dinode
if it fails validation.
With this commit we unconditionally clear & dirty all free inodes, but
the next commit remedies this and clears only corrupt free inodes.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/repair/dinode.c b/repair/dinode.c
index 49d5d05..92130db 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -117,127 +117,27 @@ _("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 (dinoc->di_version > 1 &&
- be32_to_cpu(dinoc->di_nlink) != 0) {
- __dirty_no_modify_ret(dirty);
- dinoc->di_nlink = 0;
- }
-
- /* 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;
- }
- }
+ 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;
- if (be64_to_cpu(dinoc->di_flags2) != 0) {
- __dirty_no_modify_ret(dirty);
- dinoc->di_flags2 = 0;
- }
+ dinoc->di_gen = cpu_to_be32(random());
+ dinoc->di_format = XFS_DINODE_FMT_EXTENTS;
+ dinoc->di_aformat = XFS_DINODE_FMT_EXTENTS;
- if (be64_to_cpu(dinoc->di_lsn) != 0) {
- __dirty_no_modify_ret(dirty);
- dinoc->di_lsn = 0;
- }
+ /* we are done for version 1/2 inodes */
+ if (dinoc->di_version < 3)
+ return;
- if (be64_to_cpu(dinoc->di_changecount) != 0) {
- __dirty_no_modify_ret(dirty);
- dinoc->di_changecount = 0;
- }
-
- return dirty;
+ dinoc->di_ino = cpu_to_be64(ino_num);
+ platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_meta_uuid);
+ return;
}
static int
@@ -258,21 +158,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;
}
@@ -2130,8 +2024,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;
}
@@ -2541,7 +2435,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) {
@@ -2554,8 +2448,10 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
* it just in case to ensure that format, etc. are
* set correctly
*/
- if (!no_modify)
- *dirty += clear_dinode(mp, dino, lino);
+ if (!no_modify) {
+ clear_dinode(mp, dino, lino);
+ *dirty += 1;
+ }
*used = is_free;
return 0;
}
@@ -2568,7 +2464,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"));
@@ -3006,8 +2903,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;
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] xfs_repair: notify user if free inodes contain errors
2018-05-30 15:49 [PATCH 0/2] xfs_repair: rework inode clearing and free inode validation Eric Sandeen
2018-05-30 15:51 ` [PATCH 1/2] xfs_repair: clear_dinode should only clear, not check contents Eric Sandeen
@ 2018-05-30 15:51 ` Eric Sandeen
1 sibling, 0 replies; 3+ messages in thread
From: Eric Sandeen @ 2018-05-30 15:51 UTC (permalink / raw)
To: Eric Sandeen, 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 shamelessly re-uses xfs_dinode_verify for this purpose.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 63bf27e..0f68e58 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 92130db..a4e3c51 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2448,9 +2448,15 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
* it just in case to ensure that format, etc. are
* set correctly
*/
- if (!no_modify) {
- clear_dinode(mp, dino, lino);
- *dirty += 1;
+ if (libxfs_dinode_verify(mp, lino, dino) != NULL) {
+ do_warn(
+ _("free inode %" PRIu64 " contains errors, "), lino);
+ if (!no_modify) {
+ clear_dinode(mp, dino, lino);
+ do_warn(_("corrected\n"));
+ *dirty += 1;
+ } else
+ do_warn(_("would correct\n"));
}
*used = is_free;
return 0;
^ permalink raw reply related [flat|nested] 3+ messages in thread