From 971ee3a6706abf1074349c124922e4e4d513fa45 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:23 -0700 Subject: [PATCH 1/6] xfs: change bmap scrubber to store the previous mapping Convert the inode data/attr/cow fork scrubber to remember the entire previous mapping, not just the next expected offset. No behavior changes here, but this will enable some better checking in subsequent patches. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/bmap.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index e485a546a758dd..7b4d0e2736a2a5 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -96,11 +96,23 @@ xchk_setup_inode_bmap( struct xchk_bmap_info { struct xfs_scrub *sc; + + /* Incore extent tree cursor */ struct xfs_iext_cursor icur; - xfs_fileoff_t lastoff; + + /* Previous fork mapping that we examined */ + struct xfs_bmbt_irec prev_rec; + + /* Is this a realtime fork? */ bool is_rt; + + /* May mappings point to shared space? */ bool is_shared; + + /* Was the incore extent tree loaded? */ bool was_loaded; + + /* Which inode fork are we checking? */ int whichfork; }; @@ -405,7 +417,8 @@ xchk_bmap_iextent( * Check for out-of-order extents. This record could have come * from the incore list, for which there is no ordering check. */ - if (irec->br_startoff < info->lastoff) + if (irec->br_startoff < info->prev_rec.br_startoff + + info->prev_rec.br_blockcount) xchk_fblock_set_corrupt(info->sc, info->whichfork, irec->br_startoff); @@ -712,7 +725,8 @@ xchk_bmap_iextent_delalloc( * Check for out-of-order extents. This record could have come * from the incore list, for which there is no ordering check. */ - if (irec->br_startoff < info->lastoff) + if (irec->br_startoff < info->prev_rec.br_startoff + + info->prev_rec.br_blockcount) xchk_fblock_set_corrupt(info->sc, info->whichfork, irec->br_startoff); @@ -806,7 +820,6 @@ xchk_bmap( goto out; /* Scrub extent records. */ - info.lastoff = 0; ifp = xfs_ifork_ptr(ip, whichfork); for_each_xfs_iext(ifp, &info.icur, &irec) { if (xchk_should_terminate(sc, &error) || @@ -823,7 +836,7 @@ xchk_bmap( xchk_bmap_iextent_delalloc(ip, &info, &irec); else xchk_bmap_iextent(ip, &info, &irec); - info.lastoff = irec.br_startoff + irec.br_blockcount; + memcpy(&info.prev_rec, &irec, sizeof(struct xfs_bmbt_irec)); } error = xchk_bmap_check_rmaps(sc, whichfork); From 634d4a79e76691020ba73f50416da37a30779e9e Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:24 -0700 Subject: [PATCH 2/6] xfs: accumulate iextent records when checking bmap Currently, the bmap scrubber checks file fork mappings individually. In the case that the file uses multiple mappings to a single contiguous piece of space, the scrubber repeatedly locks the AG to check the existence of a reverse mapping that overlaps this file mapping. If the reverse mapping starts before or ends after the mapping we're checking, it will also crawl around in the bmbt checking correspondence for adjacent extents. This is not very time efficient because it does the crawling while holding the AGF buffer, and checks the middle mappings multiple times. Instead, create a custom iextent record iterator function that combines multiple adjacent allocated mappings into one large incore bmbt record. This is feasible because the incore bmbt record length is 64-bits wide. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/libxfs/xfs_bmap.h | 2 +- fs/xfs/scrub/bmap.c | 183 +++++++++++++++++++++++---------------- 2 files changed, 107 insertions(+), 78 deletions(-) diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 9ff030d1298157..e33470e39728d5 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -145,7 +145,7 @@ static inline int xfs_bmapi_whichfork(uint32_t bmapi_flags) { BMAP_COWFORK, "COW" } /* Return true if the extent is an allocated extent, written or not. */ -static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec) +static inline bool xfs_bmap_is_real_extent(const struct xfs_bmbt_irec *irec) { return irec->br_startblock != HOLESTARTBLOCK && irec->br_startblock != DELAYSTARTBLOCK && diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index 7b4d0e2736a2a5..4bda1b0ee12233 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -165,48 +165,6 @@ xchk_bmap_get_rmap( return has_rmap; } -static inline bool -xchk_bmap_has_prev( - struct xchk_bmap_info *info, - struct xfs_bmbt_irec *irec) -{ - struct xfs_bmbt_irec got; - struct xfs_ifork *ifp; - - ifp = xfs_ifork_ptr(info->sc->ip, info->whichfork); - - if (!xfs_iext_peek_prev_extent(ifp, &info->icur, &got)) - return false; - if (got.br_startoff + got.br_blockcount != irec->br_startoff) - return false; - if (got.br_startblock + got.br_blockcount != irec->br_startblock) - return false; - if (got.br_state != irec->br_state) - return false; - return true; -} - -static inline bool -xchk_bmap_has_next( - struct xchk_bmap_info *info, - struct xfs_bmbt_irec *irec) -{ - struct xfs_bmbt_irec got; - struct xfs_ifork *ifp; - - ifp = xfs_ifork_ptr(info->sc->ip, info->whichfork); - - if (!xfs_iext_peek_next_extent(ifp, &info->icur, &got)) - return false; - if (irec->br_startoff + irec->br_blockcount != got.br_startoff) - return false; - if (irec->br_startblock + irec->br_blockcount != got.br_startblock) - return false; - if (got.br_state != irec->br_state) - return false; - return true; -} - /* Make sure that we have rmapbt records for this extent. */ STATIC void xchk_bmap_xref_rmap( @@ -277,31 +235,20 @@ xchk_bmap_xref_rmap( irec->br_startoff); /* - * If the rmap starts before this bmbt record, make sure there's a bmbt - * record for the previous offset that is contiguous with this mapping. - * Skip this for CoW fork extents because the refcount btree (and not - * the inode) is the ondisk owner for those extents. - */ - if (info->whichfork != XFS_COW_FORK && rmap.rm_startblock < agbno && - !xchk_bmap_has_prev(info, irec)) { - xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); - return; - } - - /* - * If the rmap ends after this bmbt record, make sure there's a bmbt - * record for the next offset that is contiguous with this mapping. - * Skip this for CoW fork extents because the refcount btree (and not - * the inode) is the ondisk owner for those extents. + * The rmap must correspond exactly with this bmbt record. Skip this + * for CoW fork extents because the refcount btree (and not the inode) + * is the ondisk owner for those extents. */ - rmap_end = (unsigned long long)rmap.rm_startblock + rmap.rm_blockcount; - if (info->whichfork != XFS_COW_FORK && - rmap_end > agbno + irec->br_blockcount && - !xchk_bmap_has_next(info, irec)) { - xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); - return; + if (info->whichfork != XFS_COW_FORK) { + if (rmap.rm_startblock != agbno) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + + rmap_end = (unsigned long long)rmap.rm_startblock + + rmap.rm_blockcount; + if (rmap_end != agbno + irec->br_blockcount) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); } } @@ -428,15 +375,7 @@ xchk_bmap_iextent( xchk_bmap_dirattr_extent(ip, info, irec); - /* There should never be a "hole" extent in either extent list. */ - if (irec->br_startblock == HOLESTARTBLOCK) - xchk_fblock_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); - /* Make sure the extent points to a valid place. */ - if (irec->br_blockcount > XFS_MAX_BMBT_EXTLEN) - xchk_fblock_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); if (info->is_rt && !xfs_verify_rtext(mp, irec->br_startblock, irec->br_blockcount)) xchk_fblock_set_corrupt(info->sc, info->whichfork, @@ -740,6 +679,90 @@ xchk_bmap_iextent_delalloc( irec->br_startoff); } +/* Decide if this individual fork mapping is ok. */ +static bool +xchk_bmap_iext_mapping( + struct xchk_bmap_info *info, + const struct xfs_bmbt_irec *irec) +{ + /* There should never be a "hole" extent in either extent list. */ + if (irec->br_startblock == HOLESTARTBLOCK) + return false; + if (irec->br_blockcount > XFS_MAX_BMBT_EXTLEN) + return false; + return true; +} + +/* Are these two mappings contiguous with each other? */ +static inline bool +xchk_are_bmaps_contiguous( + const struct xfs_bmbt_irec *b1, + const struct xfs_bmbt_irec *b2) +{ + /* Don't try to combine unallocated mappings. */ + if (!xfs_bmap_is_real_extent(b1)) + return false; + if (!xfs_bmap_is_real_extent(b2)) + return false; + + /* Does b2 come right after b1 in the logical and physical range? */ + if (b1->br_startoff + b1->br_blockcount != b2->br_startoff) + return false; + if (b1->br_startblock + b1->br_blockcount != b2->br_startblock) + return false; + if (b1->br_state != b2->br_state) + return false; + return true; +} + +/* + * Walk the incore extent records, accumulating consecutive contiguous records + * into a single incore mapping. Returns true if @irec has been set to a + * mapping or false if there are no more mappings. Caller must ensure that + * @info.icur is zeroed before the first call. + */ +static int +xchk_bmap_iext_iter( + struct xchk_bmap_info *info, + struct xfs_bmbt_irec *irec) +{ + struct xfs_bmbt_irec got; + struct xfs_ifork *ifp; + + ifp = xfs_ifork_ptr(info->sc->ip, info->whichfork); + + /* Advance to the next iextent record and check the mapping. */ + xfs_iext_next(ifp, &info->icur); + if (!xfs_iext_get_extent(ifp, &info->icur, irec)) + return false; + + if (!xchk_bmap_iext_mapping(info, irec)) { + xchk_fblock_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + return false; + } + + /* + * Iterate subsequent iextent records and merge them with the one + * that we just read, if possible. + */ + while (xfs_iext_peek_next_extent(ifp, &info->icur, &got)) { + if (!xchk_are_bmaps_contiguous(irec, &got)) + break; + + if (!xchk_bmap_iext_mapping(info, &got)) { + xchk_fblock_set_corrupt(info->sc, info->whichfork, + got.br_startoff); + return false; + } + + irec->br_blockcount += got.br_blockcount; + xfs_iext_next(ifp, &info->icur); + } + + return true; +} + /* * Scrub an inode fork's block mappings. * @@ -819,9 +842,15 @@ xchk_bmap( if (!xchk_fblock_process_error(sc, whichfork, 0, &error)) goto out; - /* Scrub extent records. */ - ifp = xfs_ifork_ptr(ip, whichfork); - for_each_xfs_iext(ifp, &info.icur, &irec) { + /* + * Scrub extent records. We use a special iterator function here that + * combines adjacent mappings if they are logically and physically + * contiguous. For large allocations that require multiple bmbt + * records, this reduces the number of cross-referencing calls, which + * reduces runtime. Cross referencing with the rmap is simpler because + * the rmap must match the combined mapping exactly. + */ + while (xchk_bmap_iext_iter(&info, &irec)) { if (xchk_should_terminate(sc, &error) || (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) goto out; From c0d5a92f7aaf41b1ab70869358d534757b569a1f Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:24 -0700 Subject: [PATCH 3/6] xfs: split xchk_bmap_xref_rmap into two functions There's more special-cased functionality than not in this function. Split it into two so that each can be far more cohesive. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/bmap.c | 116 +++++++++++++++++++++++++++++--------------- 1 file changed, 76 insertions(+), 40 deletions(-) diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index 4bda1b0ee12233..912b67d2321b7b 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -165,7 +165,7 @@ xchk_bmap_get_rmap( return has_rmap; } -/* Make sure that we have rmapbt records for this extent. */ +/* Make sure that we have rmapbt records for this data/attr fork extent. */ STATIC void xchk_bmap_xref_rmap( struct xchk_bmap_info *info, @@ -174,41 +174,39 @@ xchk_bmap_xref_rmap( { struct xfs_rmap_irec rmap; unsigned long long rmap_end; - uint64_t owner; + uint64_t owner = info->sc->ip->i_ino; if (!info->sc->sa.rmap_cur || xchk_skip_xref(info->sc->sm)) return; - if (info->whichfork == XFS_COW_FORK) - owner = XFS_RMAP_OWN_COW; - else - owner = info->sc->ip->i_ino; - /* Find the rmap record for this irec. */ if (!xchk_bmap_get_rmap(info, irec, agbno, owner, &rmap)) return; - /* Check the rmap. */ + /* + * The rmap must be an exact match for this incore file mapping record, + * which may have arisen from multiple ondisk records. + */ + if (rmap.rm_startblock != agbno) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + rmap_end = (unsigned long long)rmap.rm_startblock + rmap.rm_blockcount; - if (rmap.rm_startblock > agbno || - agbno + irec->br_blockcount > rmap_end) + if (rmap_end != agbno + irec->br_blockcount) xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, irec->br_startoff); - /* - * Check the logical offsets if applicable. CoW staging extents - * don't track logical offsets since the mappings only exist in - * memory. - */ - if (info->whichfork != XFS_COW_FORK) { - rmap_end = (unsigned long long)rmap.rm_offset + - rmap.rm_blockcount; - if (rmap.rm_offset > irec->br_startoff || - irec->br_startoff + irec->br_blockcount > rmap_end) - xchk_fblock_xref_set_corrupt(info->sc, - info->whichfork, irec->br_startoff); - } + /* Check the logical offsets. */ + if (rmap.rm_offset != irec->br_startoff) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + + rmap_end = (unsigned long long)rmap.rm_offset + rmap.rm_blockcount; + if (rmap_end != irec->br_startoff + irec->br_blockcount) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + /* Check the owner */ if (rmap.rm_owner != owner) xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, irec->br_startoff); @@ -220,8 +218,7 @@ xchk_bmap_xref_rmap( * records because the blocks are owned (on-disk) by the refcountbt, * which doesn't track unwritten state. */ - if (owner != XFS_RMAP_OWN_COW && - !!(irec->br_state == XFS_EXT_UNWRITTEN) != + if (!!(irec->br_state == XFS_EXT_UNWRITTEN) != !!(rmap.rm_flags & XFS_RMAP_UNWRITTEN)) xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, irec->br_startoff); @@ -233,23 +230,60 @@ xchk_bmap_xref_rmap( if (rmap.rm_flags & XFS_RMAP_BMBT_BLOCK) xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, irec->br_startoff); +} + +/* Make sure that we have rmapbt records for this COW fork extent. */ +STATIC void +xchk_bmap_xref_rmap_cow( + struct xchk_bmap_info *info, + struct xfs_bmbt_irec *irec, + xfs_agblock_t agbno) +{ + struct xfs_rmap_irec rmap; + unsigned long long rmap_end; + uint64_t owner = XFS_RMAP_OWN_COW; + + if (!info->sc->sa.rmap_cur || xchk_skip_xref(info->sc->sm)) + return; + + /* Find the rmap record for this irec. */ + if (!xchk_bmap_get_rmap(info, irec, agbno, owner, &rmap)) + return; /* - * The rmap must correspond exactly with this bmbt record. Skip this - * for CoW fork extents because the refcount btree (and not the inode) - * is the ondisk owner for those extents. + * CoW staging extents are owned by the refcount btree, so the rmap + * can start before and end after the physical space allocated to this + * mapping. There are no offsets to check. */ - if (info->whichfork != XFS_COW_FORK) { - if (rmap.rm_startblock != agbno) - xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); - - rmap_end = (unsigned long long)rmap.rm_startblock + - rmap.rm_blockcount; - if (rmap_end != agbno + irec->br_blockcount) - xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, - irec->br_startoff); - } + if (rmap.rm_startblock > agbno) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + + rmap_end = (unsigned long long)rmap.rm_startblock + rmap.rm_blockcount; + if (rmap_end < agbno + irec->br_blockcount) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + + /* Check the owner */ + if (rmap.rm_owner != owner) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + + /* + * No flags allowed. Note that the (in-memory) CoW fork distinguishes + * between unwritten and written extents, but we don't track that in + * the rmap records because the blocks are owned (on-disk) by the + * refcountbt, which doesn't track unwritten state. + */ + if (rmap.rm_flags & XFS_RMAP_ATTR_FORK) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + if (rmap.rm_flags & XFS_RMAP_BMBT_BLOCK) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); + if (rmap.rm_flags & XFS_RMAP_UNWRITTEN) + xchk_fblock_xref_set_corrupt(info->sc, info->whichfork, + irec->br_startoff); } /* Cross-reference a single rtdev extent record. */ @@ -288,9 +322,9 @@ xchk_bmap_iextent_xref( xchk_xref_is_used_space(info->sc, agbno, len); xchk_xref_is_not_inode_chunk(info->sc, agbno, len); - xchk_bmap_xref_rmap(info, irec, agbno); switch (info->whichfork) { case XFS_DATA_FORK: + xchk_bmap_xref_rmap(info, irec, agbno); if (!xfs_is_reflink_inode(info->sc->ip)) { xfs_rmap_ino_owner(&oinfo, info->sc->ip->i_ino, info->whichfork, irec->br_startoff); @@ -303,6 +337,7 @@ xchk_bmap_iextent_xref( irec->br_blockcount); break; case XFS_ATTR_FORK: + xchk_bmap_xref_rmap(info, irec, agbno); xfs_rmap_ino_owner(&oinfo, info->sc->ip->i_ino, info->whichfork, irec->br_startoff); xchk_xref_is_only_owned_by(info->sc, agbno, irec->br_blockcount, @@ -313,6 +348,7 @@ xchk_bmap_iextent_xref( irec->br_blockcount); break; case XFS_COW_FORK: + xchk_bmap_xref_rmap_cow(info, irec, agbno); xchk_xref_is_only_owned_by(info->sc, agbno, irec->br_blockcount, &XFS_RMAP_OINFO_COW); xchk_xref_is_cow_staging(info->sc, agbno, From 336642f79283715e4535bfaa05f5593dd91da6e8 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:25 -0700 Subject: [PATCH 4/6] xfs: alert the user about data/attr fork mappings that could be merged If the data or attr forks have mappings that could be merged, let the user know that the structure could be optimized. This isn't a filesystem corruption since the regular filesystem does not try to be smart about merging bmbt records. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/bmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index 912b67d2321b7b..a2edcf8cc779d4 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -764,6 +764,7 @@ xchk_bmap_iext_iter( { struct xfs_bmbt_irec got; struct xfs_ifork *ifp; + xfs_filblks_t prev_len; ifp = xfs_ifork_ptr(info->sc->ip, info->whichfork); @@ -782,6 +783,7 @@ xchk_bmap_iext_iter( * Iterate subsequent iextent records and merge them with the one * that we just read, if possible. */ + prev_len = irec->br_blockcount; while (xfs_iext_peek_next_extent(ifp, &info->icur, &got)) { if (!xchk_are_bmaps_contiguous(irec, &got)) break; @@ -792,7 +794,16 @@ xchk_bmap_iext_iter( return false; } + /* + * Notify the user of mergeable records in the data or attr + * forks. CoW forks only exist in memory so we ignore them. + */ + if (info->whichfork != XFS_COW_FORK && + prev_len + got.br_blockcount > BMBT_BLOCKCOUNT_MASK) + xchk_ino_set_preen(info->sc, info->sc->ip->i_ino); + irec->br_blockcount += got.br_blockcount; + prev_len = got.br_blockcount; xfs_iext_next(ifp, &info->icur); } From e8882f69b941b20704ea509ebfca2d8a123ad6e3 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:25 -0700 Subject: [PATCH 5/6] xfs: split the xchk_bmap_check_rmaps into a predicate This function has two parts: the second part scans every reverse mapping record for this file fork to make sure that there's a corresponding mapping in the fork, and the first part decides if we even want to do that. Split the first part into a separate predicate so that we can make more changes to it in the next patch. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/bmap.c | 60 ++++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index a2edcf8cc779d4..64ed5b6585d5f1 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -635,28 +635,28 @@ xchk_bmap_check_ag_rmaps( return error; } -/* Make sure each rmap has a corresponding bmbt entry. */ -STATIC int -xchk_bmap_check_rmaps( - struct xfs_scrub *sc, - int whichfork) +/* + * Decide if we want to walk every rmap btree in the fs to make sure that each + * rmap for this file fork has corresponding bmbt entries. + */ +static bool +xchk_bmap_want_check_rmaps( + struct xchk_bmap_info *info) { - struct xfs_ifork *ifp = xfs_ifork_ptr(sc->ip, whichfork); - struct xfs_perag *pag; - xfs_agnumber_t agno; + struct xfs_scrub *sc = info->sc; + struct xfs_ifork *ifp; bool zero_size; - int error; - if (!xfs_has_rmapbt(sc->mp) || - whichfork == XFS_COW_FORK || - (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)) - return 0; + if (!xfs_has_rmapbt(sc->mp)) + return false; + if (info->whichfork == XFS_COW_FORK) + return false; + if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT) + return false; /* Don't support realtime rmap checks yet. */ - if (XFS_IS_REALTIME_INODE(sc->ip) && whichfork == XFS_DATA_FORK) - return 0; - - ASSERT(xfs_ifork_ptr(sc->ip, whichfork) != NULL); + if (info->is_rt) + return false; /* * Only do this for complex maps that are in btree format, or for @@ -666,14 +666,28 @@ xchk_bmap_check_rmaps( * reattached. */ - if (whichfork == XFS_DATA_FORK) + if (info->whichfork == XFS_DATA_FORK) zero_size = i_size_read(VFS_I(sc->ip)) == 0; else zero_size = false; + ifp = xfs_ifork_ptr(sc->ip, info->whichfork); if (ifp->if_format != XFS_DINODE_FMT_BTREE && (zero_size || ifp->if_nextents > 0)) - return 0; + return false; + + return true; +} + +/* Make sure each rmap has a corresponding bmbt entry. */ +STATIC int +xchk_bmap_check_rmaps( + struct xfs_scrub *sc, + int whichfork) +{ + struct xfs_perag *pag; + xfs_agnumber_t agno; + int error; for_each_perag(sc->mp, agno, pag) { error = xchk_bmap_check_ag_rmaps(sc, whichfork, pag); @@ -915,9 +929,11 @@ xchk_bmap( memcpy(&info.prev_rec, &irec, sizeof(struct xfs_bmbt_irec)); } - error = xchk_bmap_check_rmaps(sc, whichfork); - if (!xchk_fblock_xref_process_error(sc, whichfork, 0, &error)) - goto out; + if (xchk_bmap_want_check_rmaps(&info)) { + error = xchk_bmap_check_rmaps(sc, whichfork); + if (!xchk_fblock_xref_process_error(sc, whichfork, 0, &error)) + goto out; + } out: return error; } From 1e59fdb7d6157ff685a250e0873a015a2b16a4f2 Mon Sep 17 00:00:00 2001 From: "Darrick J. Wong" Date: Tue, 11 Apr 2023 19:00:26 -0700 Subject: [PATCH 6/6] xfs: don't call xchk_bmap_check_rmaps for btree-format file forks The logic at the end of xchk_bmap_want_check_rmaps tries to detect a file fork that has been zapped by what will become the online inode repair code. Zapped forks are in FMT_EXTENTS with zero extents, and some sort of hint that there's supposed to be data somewhere in the filesystem. Unfortunately, the inverted logic here is confusing and has the effect that we always call xchk_bmap_check_rmaps for FMT_BTREE forks. This is horribly inefficient and unnecessary, so invert the logic to get rid of this performance problem. This has caused 8h delays in generic/333 and generic/334. Signed-off-by: Darrick J. Wong Reviewed-by: Dave Chinner --- fs/xfs/scrub/bmap.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c index 64ed5b6585d5f1..87ab9f95a487ad 100644 --- a/fs/xfs/scrub/bmap.c +++ b/fs/xfs/scrub/bmap.c @@ -645,7 +645,6 @@ xchk_bmap_want_check_rmaps( { struct xfs_scrub *sc = info->sc; struct xfs_ifork *ifp; - bool zero_size; if (!xfs_has_rmapbt(sc->mp)) return false; @@ -659,24 +658,23 @@ xchk_bmap_want_check_rmaps( return false; /* - * Only do this for complex maps that are in btree format, or for - * situations where we would seem to have a size but zero extents. - * The inode repair code can zap broken iforks, which means we have - * to flag this bmap as corrupt if there are rmaps that need to be - * reattached. + * The inode repair code zaps broken inode forks by resetting them back + * to EXTENTS format and zero extent records. If we encounter a fork + * in this state along with evidence that the fork isn't supposed to be + * empty, we need to scan the reverse mappings to decide if we're going + * to rebuild the fork. Data forks with nonzero file size are scanned. + * xattr forks are never empty of content, so they are always scanned. */ - - if (info->whichfork == XFS_DATA_FORK) - zero_size = i_size_read(VFS_I(sc->ip)) == 0; - else - zero_size = false; - ifp = xfs_ifork_ptr(sc->ip, info->whichfork); - if (ifp->if_format != XFS_DINODE_FMT_BTREE && - (zero_size || ifp->if_nextents > 0)) - return false; + if (ifp->if_format == XFS_DINODE_FMT_EXTENTS && ifp->if_nextents == 0) { + if (info->whichfork == XFS_DATA_FORK && + i_size_read(VFS_I(sc->ip)) == 0) + return false; - return true; + return true; + } + + return false; } /* Make sure each rmap has a corresponding bmbt entry. */