On Mon, May 18, 2020 at 09:33:53AM +0200, Christoph Hellwig wrote:
> From: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
>
> XFS_IFORK_Q is supposed to be a predicate, not a function returning a
> value. Its usage is in xchk_bmap_check_rmaps is incorrect, but that
> function only cares about whether or not the "size" of the data is zero
> or not. Convert that logic to use a proper boolean, and teach the
> caller to skip the call entirely if the end result would be that we'd do
> nothing anyway. This avoids a crash later in this series.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> [hch: generalized the NULL ifor check]
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
I'm still rather confused about the i_size logic, but this seems like a
good cleanup regardless:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> fs/xfs/scrub/bmap.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index add8598eacd5d..d0daf3de9fde5 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -566,8 +566,8 @@ xchk_bmap_check_rmaps(
> struct xfs_scrub *sc,
> int whichfork)
> {
> - loff_t size;
> xfs_agnumber_t agno;
> + bool zero_size;
> int error;
>
> if (!xfs_sb_version_hasrmapbt(&sc->mp->m_sb) ||
> @@ -579,6 +579,8 @@ xchk_bmap_check_rmaps(
> if (XFS_IS_REALTIME_INODE(sc->ip) && whichfork == XFS_DATA_FORK)
> return 0;
>
> + ASSERT(XFS_IFORK_PTR(sc->ip, whichfork) != NULL);
> +
> /*
> * 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.
> @@ -586,19 +588,13 @@ xchk_bmap_check_rmaps(
> * to flag this bmap as corrupt if there are rmaps that need to be
> * reattached.
> */
> - switch (whichfork) {
> - case XFS_DATA_FORK:
> - size = i_size_read(VFS_I(sc->ip));
> - break;
> - case XFS_ATTR_FORK:
> - size = XFS_IFORK_Q(sc->ip);
> - break;
> - default:
> - size = 0;
> - break;
> - }
> + if (whichfork == XFS_DATA_FORK)
> + zero_size = i_size_read(VFS_I(sc->ip)) == 0;
> + else
> + zero_size = false;
> +
> if (XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> - (size == 0 || XFS_IFORK_NEXTENTS(sc->ip, whichfork) > 0))
> + (zero_size || XFS_IFORK_NEXTENTS(sc->ip, whichfork) > 0))
> return 0;
>
> for (agno = 0; agno < sc->mp->m_sb.sb_agcount; agno++) {
> @@ -627,12 +623,14 @@ xchk_bmap(
> struct xchk_bmap_info info = { NULL };
> struct xfs_mount *mp = sc->mp;
> struct xfs_inode *ip = sc->ip;
> - struct xfs_ifork *ifp;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> xfs_fileoff_t endoff;
> struct xfs_iext_cursor icur;
> int error = 0;
>
> - ifp = XFS_IFORK_PTR(ip, whichfork);
> + /* Non-existent forks can be ignored. */
> + if (!ifp)
> + goto out;
>
> info.is_rt = whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip);
> info.whichfork = whichfork;
> @@ -641,9 +639,6 @@ xchk_bmap(
>
> switch (whichfork) {
> case XFS_COW_FORK:
> - /* Non-existent CoW forks are ignorable. */
> - if (!ifp)
> - goto out;
> /* No CoW forks on non-reflink inodes/filesystems. */
> if (!xfs_is_reflink_inode(ip)) {
> xchk_ino_set_corrupt(sc, sc->ip->i_ino);
> @@ -651,8 +646,6 @@ xchk_bmap(
> }
> break;
> case XFS_ATTR_FORK:
> - if (!ifp)
> - goto out_check_rmap;
> if (!xfs_sb_version_hasattr(&mp->m_sb) &&
> !xfs_sb_version_hasattr2(&mp->m_sb))
> xchk_ino_set_corrupt(sc, sc->ip->i_ino);
> @@ -700,7 +693,6 @@ xchk_bmap(
>
> /* Scrub extent records. */
> info.lastoff = 0;
> - ifp = XFS_IFORK_PTR(ip, whichfork);
> for_each_xfs_iext(ifp, &icur, &irec) {
> if (xchk_should_terminate(sc, &error) ||
> (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> @@ -717,7 +709,6 @@ xchk_bmap(
> goto out;
> }
>
> -out_check_rmap:
> error = xchk_bmap_check_rmaps(sc, whichfork);
> if (!xchk_fblock_xref_process_error(sc, whichfork, 0, &error))
> goto out;
> --
> 2.26.2
>
