On Tue, Feb 23, 2021 at 01:56:29PM +0530, Chandan Babu R wrote:
> This commit now makes it possible for scrub to check if an inode's extents are
> maximally sized i.e. it checks if an inode's extent is contiguous (in terms of
> both file offset and disk offset) with neighbouring extents and the total
> length of both the extents is less than the maximum allowed extent
> length (i.e. MAXEXTLEN).
>
> Signed-off-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> ---
> fs/xfs/scrub/inode.c | 11 +++++++----
> fs/xfs/xfs_bmap_util.c | 36 +++++++++++++++++++++++++++++++-----
> fs/xfs/xfs_bmap_util.h | 5 +++--
> fs/xfs/xfs_qm.c | 2 +-
> 4 files changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index faf65eb5bd31..33b530785e36 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -478,23 +478,26 @@ xchk_inode_xref_bmap(
> xfs_filblks_t count;
> xfs_filblks_t acount;
> int error;
> + bool maximal_sized_exts;
>
> if (xchk_skip_xref(sc->sm))
> return;
>
> /* Walk all the extents to check nextents/naextents/nblocks. */
> + maximal_sized_exts = true;
Shouldn't this be set by xfs_bmap_count_blocks()? It's purely an out
parameter.
> error = xfs_bmap_count_blocks(sc->tp, sc->ip, XFS_DATA_FORK,
> - &nextents, &count);
> + &nextents, &count, &maximal_sized_exts);
> if (!xchk_should_check_xref(sc, &error, NULL))
> return;
> - if (nextents < be32_to_cpu(dip->di_nextents))
> + if (nextents < be32_to_cpu(dip->di_nextents) || !maximal_sized_exts)
> xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
>
> + maximal_sized_exts = true;
> error = xfs_bmap_count_blocks(sc->tp, sc->ip, XFS_ATTR_FORK,
> - &nextents, &acount);
> + &nextents, &acount, &maximal_sized_exts);
> if (!xchk_should_check_xref(sc, &error, NULL))
> return;
> - if (nextents != be16_to_cpu(dip->di_anextents))
> + if (nextents != be16_to_cpu(dip->di_anextents) || !maximal_sized_exts)
> xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
>
> /* Check nblocks against the inode. */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index e7d68318e6a5..eca39677a46f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -176,6 +176,25 @@ xfs_bmap_rtalloc(
> * Extent tree block counting routines.
> */
>
> +STATIC bool
> +is_maximal_extent(
> + struct xfs_ifork *ifp,
> + struct xfs_iext_cursor *icur,
> + struct xfs_bmbt_irec *got)
> +{
> + struct xfs_bmbt_irec left;
> +
> + if (xfs_iext_peek_prev_extent(ifp, icur, &left) &&
> + !isnullstartblock(left.br_startblock) &&
> + left.br_startoff + left.br_blockcount == got->br_startoff &&
> + left.br_startblock + left.br_blockcount == got->br_startblock &&
> + left.br_state == got->br_state &&
> + left.br_blockcount + got->br_blockcount <= MAXEXTLEN)
> + return false;
Please don't align the if test code with the statement body.
> + else
No need for the else here.
> + return true;
> +}
> +
> /*
> * Count leaf blocks given a range of extent records. Delayed allocation
> * extents are not counted towards the totals.
> @@ -183,7 +202,8 @@ xfs_bmap_rtalloc(
> xfs_extnum_t
> xfs_bmap_count_leaves(
> struct xfs_ifork *ifp,
> - xfs_filblks_t *count)
> + xfs_filblks_t *count,
> + bool *maximal_sized_exts)
> {
> struct xfs_iext_cursor icur;
> struct xfs_bmbt_irec got;
> @@ -191,6 +211,10 @@ xfs_bmap_count_leaves(
>
> for_each_xfs_iext(ifp, &icur, &got) {
> if (!isnullstartblock(got.br_startblock)) {
> + if (maximal_sized_exts)
> + *maximal_sized_exts =
> + is_maximal_extent(ifp, &icur, &got);
Shouldn't this be *maximal_sized_exts &= is_maximal_extent(); ?
Otherwise this only tells us if the last extent in the fork is maximal.
I also wonder if it's really worth the extra complexity to make the
boolean an optional argument, since there are callers that pass in a
count pointer that's a dummy value.
--D
> +
> *count += got.br_blockcount;
> numrecs++;
> }
> @@ -209,7 +233,8 @@ xfs_bmap_count_blocks(
> struct xfs_inode *ip,
> int whichfork,
> xfs_extnum_t *nextents,
> - xfs_filblks_t *count)
> + xfs_filblks_t *count,
> + bool *maximal_sized_exts)
> {
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> @@ -246,7 +271,8 @@ xfs_bmap_count_blocks(
>
> /* fall through */
> case XFS_DINODE_FMT_EXTENTS:
> - *nextents = xfs_bmap_count_leaves(ifp, count);
> + *nextents = xfs_bmap_count_leaves(ifp, count,
> + maximal_sized_exts);
> break;
> }
>
> @@ -1442,14 +1468,14 @@ xfs_swap_extent_forks(
> if (XFS_IFORK_Q(ip) && ip->i_afp->if_nextents > 0 &&
> ip->i_afp->if_format != XFS_DINODE_FMT_LOCAL) {
> error = xfs_bmap_count_blocks(tp, ip, XFS_ATTR_FORK, &junk,
> - &aforkblks);
> + &aforkblks, NULL);
> if (error)
> return error;
> }
> if (XFS_IFORK_Q(tip) && tip->i_afp->if_nextents > 0 &&
> tip->i_afp->if_format != XFS_DINODE_FMT_LOCAL) {
> error = xfs_bmap_count_blocks(tp, tip, XFS_ATTR_FORK, &junk,
> - &taforkblks);
> + &taforkblks, NULL);
> if (error)
> return error;
> }
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 9f993168b55b..7b0ce227c7bd 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -71,10 +71,11 @@ int xfs_swap_extents(struct xfs_inode *ip, struct xfs_inode *tip,
>
> xfs_daddr_t xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb);
>
> -xfs_extnum_t xfs_bmap_count_leaves(struct xfs_ifork *ifp, xfs_filblks_t *count);
> +xfs_extnum_t xfs_bmap_count_leaves(struct xfs_ifork *ifp, xfs_filblks_t *count,
> + bool *maximal_sized_exts);
> int xfs_bmap_count_blocks(struct xfs_trans *tp, struct xfs_inode *ip,
> int whichfork, xfs_extnum_t *nextents,
> - xfs_filblks_t *count);
> + xfs_filblks_t *count, bool *maximal_sized_exts);
>
> int xfs_flush_unmap_range(struct xfs_inode *ip, xfs_off_t offset,
> xfs_off_t len);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 742d1413e2d0..04fcca5583b3 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1170,7 +1170,7 @@ xfs_qm_dqusage_adjust(
> goto error0;
> }
>
> - xfs_bmap_count_leaves(ifp, &rtblks);
> + xfs_bmap_count_leaves(ifp, &rtblks, NULL);
> }
>
> nblks = (xfs_qcnt_t)ip->i_d.di_nblocks - rtblks;
> --
> 2.29.2
>
