[PATCHSET v23.1 0/2] xfs: scrub inode core when checking metadata files

[PATCHSET v23.1 0/2] xfs: scrub inode core when checking metadata files

[Darrick J. Wong]

Hi all,

Running the online fsck QA fuzz tests, I noticed that we were
consistently missing fuzzed records in the inode cores of the realtime
freespace files and the quota files.  This patch adds the ability to
check inode cores in xchk_metadata_inode_forks.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=scrub-check-metadata-inode-records
---
 fs/xfs/scrub/common.c |   40 ++++++++++++++++++++++++++++++++++------
 fs/xfs/scrub/inode.c  |    2 +-
 2 files changed, 35 insertions(+), 7 deletions(-)

[PATCH 1/2] xfs: don't warn about files that are exactly s_maxbytes long

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

We can handle files that are exactly s_maxbytes bytes long; we just
can't handle anything larger than that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index dd2bf4cbd68f..3b272c86d0ad 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -368,7 +368,7 @@ xchk_dinode(
 	 * pagecache can't cache all the blocks in this file due to
 	 * overly large offsets, flag the inode for admin review.
 	 */
-	if (isize >= mp->m_super->s_maxbytes)
+	if (isize > mp->m_super->s_maxbytes)
 		xchk_ino_set_warning(sc, ino);
 
 	/* di_nblocks */

[PATCH 2/2] xfs: check inode core when scrubbing metadata files

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Metadata files (e.g. realtime bitmaps and quota files) do not show up in
the bulkstat output, which means that scrub-by-handle does not work;
they can only be checked through a specific scrub type.  Therefore, each
scrub type calls xchk_metadata_inode_forks to check the metadata for
whatever's in the file.

Unfortunately, that function doesn't actually check the inode record
itself.  Refactor the function a bit to make that happen.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/common.c |   40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)


diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 60c4d33fe6f5..73ac38c1126e 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -866,6 +866,33 @@ xchk_buffer_recheck(
 	trace_xchk_block_error(sc, xfs_buf_daddr(bp), fa);
 }
 
+static inline int
+xchk_metadata_inode_subtype(
+	struct xfs_scrub	*sc,
+	unsigned int		scrub_type)
+{
+	__u32			smtype = sc->sm->sm_type;
+	int			error;
+
+	sc->sm->sm_type = scrub_type;
+
+	switch (scrub_type) {
+	case XFS_SCRUB_TYPE_INODE:
+		error = xchk_inode(sc);
+		break;
+	case XFS_SCRUB_TYPE_BMBTD:
+		error = xchk_bmap_data(sc);
+		break;
+	default:
+		ASSERT(0);
+		error = -EFSCORRUPTED;
+		break;
+	}
+
+	sc->sm->sm_type = smtype;
+	return error;
+}
+
 /*
  * Scrub the attr/data forks of a metadata inode.  The metadata inode must be
  * pointed to by sc->ip and the ILOCK must be held.
@@ -874,13 +901,17 @@ int
 xchk_metadata_inode_forks(
 	struct xfs_scrub	*sc)
 {
-	__u32			smtype;
 	bool			shared;
 	int			error;
 
 	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
 		return 0;
 
+	/* Check the inode record. */
+	error = xchk_metadata_inode_subtype(sc, XFS_SCRUB_TYPE_INODE);
+	if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+		return error;
+
 	/* Metadata inodes don't live on the rt device. */
 	if (sc->ip->i_diflags & XFS_DIFLAG_REALTIME) {
 		xchk_ino_set_corrupt(sc, sc->ip->i_ino);
@@ -900,10 +931,7 @@ xchk_metadata_inode_forks(
 	}
 
 	/* Invoke the data fork scrubber. */
-	smtype = sc->sm->sm_type;
-	sc->sm->sm_type = XFS_SCRUB_TYPE_BMBTD;
-	error = xchk_bmap_data(sc);
-	sc->sm->sm_type = smtype;
+	error = xchk_metadata_inode_subtype(sc, XFS_SCRUB_TYPE_BMBTD);
 	if (error || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
 		return error;
 
@@ -918,7 +946,7 @@ xchk_metadata_inode_forks(
 			xchk_ino_set_corrupt(sc, sc->ip->i_ino);
 	}
 
-	return error;
+	return 0;
 }
 
 /*

Re: [PATCH 1/2] xfs: don't warn about files that are exactly s_maxbytes long

[Dave Chinner]

On Sun, Oct 02, 2022 at 11:20:26AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> We can handle files that are exactly s_maxbytes bytes long; we just
> can't handle anything larger than that.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/inode.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
LGTM.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: check inode core when scrubbing metadata files

[Dave Chinner]

On Sun, Oct 02, 2022 at 11:20:26AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Metadata files (e.g. realtime bitmaps and quota files) do not show up in
> the bulkstat output, which means that scrub-by-handle does not work;
> they can only be checked through a specific scrub type.  Therefore, each
> scrub type calls xchk_metadata_inode_forks to check the metadata for
> whatever's in the file.
> 
> Unfortunately, that function doesn't actually check the inode record
> itself.  Refactor the function a bit to make that happen.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/common.c |   40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)

Looks reasonable. Will there be more metadata inode types to scrub
in future?

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: check inode core when scrubbing metadata files

[Darrick J. Wong]

On Tue, Nov 15, 2022 at 01:58:54PM +1100, Dave Chinner wrote:
> On Sun, Oct 02, 2022 at 11:20:26AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Metadata files (e.g. realtime bitmaps and quota files) do not show up in
> > the bulkstat output, which means that scrub-by-handle does not work;
> > they can only be checked through a specific scrub type.  Therefore, each
> > scrub type calls xchk_metadata_inode_forks to check the metadata for
> > whatever's in the file.
> > 
> > Unfortunately, that function doesn't actually check the inode record
> > itself.  Refactor the function a bit to make that happen.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/scrub/common.c |   40 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> Looks reasonable. Will there be more metadata inode types to scrub
> in future?

For the realtime modernisation project, I am planning to shard the rt
volume into allocation groups and give each rtgroup its own rmap and
refcount btree.

> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks for the review!

--D

> 
> -- 
> Dave Chinner
> david@fromorbit.com