On Sat, May 16, 2020 at 10:01:06PM +0200, Christoph Hellwig wrote:
> > for (agno = 0; agno < sc->mp->m_sb.sb_agcount; agno++) {
> > @@ -651,8 +647,9 @@ xchk_bmap(
> > }
> > break;
> > case XFS_ATTR_FORK:
> > + /* No fork means no attr data at all. */
> > if (!ifp)
> > - goto out_check_rmap;
> > + goto out;
>
> Maybe lift the !ifp to before the switch statement, or even to just after
> assigning the value to ifp? For the data fork it obviously won't be
> true, but it still looks simple than duplicating it for the attr and
> cow fork.
>
> Otherwise looks fine:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
This is what I ended up with for my tree:
---
>From a98ef6f4bbfe186a83d3384cbd0a0727c0e1ddee Mon Sep 17 00:00:00 2001
From: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
Date: Sat, 16 May 2020 11:42:59 -0700
Subject: xfs: clean up xchk_bmap_check_rmaps usage of XFS_IFORK_Q
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>
---
fs/xfs/scrub/bmap.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index add8598eacd5d..93d5b8a9d7f74 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);
@@ -717,7 +710,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
