[v2,1/2] xfs: always init fdblocks in mount

Commit Message

Zheng Bin March 17, 2020, 7:05 a.m. UTC

Use fuzz(hydra) to test XFS and automatically generate
tmp.img(XFS v5 format, but some metadata is wrong)

xfs_repair information(just one AG):
agf_freeblks 0, counted 3224 in ag 0
agf_longest 0, counted 3224 in ag 0
sb_fdblocks 3228, counted 3224

Test as follows:
mount tmp.img tmpdir
cp file1M tmpdir
sync

In 4.19-stable, sync will stuck, while in linux-next, sync not stuck.
The reason is same to commit d0c7feaf8767
("xfs: add agf freeblocks verify in xfs_agf_verify"), cause agf_longest
is 0, we can not block this in xfs_agf_verify.

Make sure fdblocks is always inited in mount(also init ifree, icount).

xfs_mountfs
  xfs_check_summary_counts
    xfs_initialize_perag_data

Signed-off-by: Zheng Bin <[e-mail]>
---
 fs/xfs/libxfs/xfs_ag_resv.c | 11 +++++++----
 fs/xfs/xfs_mount.c          | 33 ---------------------------------
 2 files changed, 7 insertions(+), 37 deletions(-)

--
2.7.4

Comments

Christoph Hellwig March 17, 2020, 7:01 p.m. UTC | #1

So if we can't trust the sb counters on your fuzzed file system,
how can we trust the AG counters?

Also if we decide that we always want to rebuild and thus always read
in the pagf all the pagf_init checks can go away..

Patch

diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index fdfe6dc..487961b 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -303,10 +303,13 @@  xfs_ag_resv_init(
 	}

 #ifdef DEBUG
-	/* need to read in the AGF for the ASSERT below to work */
-	error = xfs_alloc_pagf_init(pag->pag_mount, tp, pag->pag_agno, 0);
-	if (error)
-		return error;
+	if (!pag->pagf_init) {
+		/* need to read in the AGF for the ASSERT below to work */
+		error = xfs_alloc_pagf_init(pag->pag_mount, tp,
+					    pag->pag_agno, 0);
+		if (error)
+			return error;
+	}

 	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
 	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index c5513e5..dc41801 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -594,39 +594,6 @@  xfs_check_summary_counts(
 		return -EFSCORRUPTED;
 	}

-	/*
-	 * Now the log is mounted, we know if it was an unclean shutdown or
-	 * not. If it was, with the first phase of recovery has completed, we
-	 * have consistent AG blocks on disk. We have not recovered EFIs yet,
-	 * but they are recovered transactionally in the second recovery phase
-	 * later.
-	 *
-	 * If the log was clean when we mounted, we can check the summary
-	 * counters.  If any of them are obviously incorrect, we can recompute
-	 * them from the AGF headers in the next step.
-	 */
-	if (XFS_LAST_UNMOUNT_WAS_CLEAN(mp) &&
-	    (mp->m_sb.sb_fdblocks > mp->m_sb.sb_dblocks ||
-	     !xfs_verify_icount(mp, mp->m_sb.sb_icount) ||
-	     mp->m_sb.sb_ifree > mp->m_sb.sb_icount))
-		xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
-
-	/*
-	 * We can safely re-initialise incore superblock counters from the
-	 * per-ag data. These may not be correct if the filesystem was not
-	 * cleanly unmounted, so we waited for recovery to finish before doing
-	 * this.
-	 *
-	 * If the filesystem was cleanly unmounted or the previous check did
-	 * not flag anything weird, then we can trust the values in the
-	 * superblock to be correct and we don't need to do anything here.
-	 * Otherwise, recalculate the summary counters.
-	 */
-	if ((!xfs_sb_version_haslazysbcount(&mp->m_sb) ||
-	     XFS_LAST_UNMOUNT_WAS_CLEAN(mp)) &&
-	    !xfs_fs_has_sickness(mp, XFS_SICK_FS_COUNTERS))
-		return 0;
-
 	return xfs_initialize_perag_data(mp, mp->m_sb.sb_agcount);
 }