Re: [PATCH 5/6] xfs: move the fork format fields into struct xfs_ifork

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, May 16, 2020 at 10:01:43AM -0700, Darrick J. Wong wrote:
> On Sat, May 16, 2020 at 03:58:08PM +0200, Christoph Hellwig wrote:
> > On Thu, May 14, 2020 at 02:25:41PM -0700, Darrick J. Wong wrote:
> > 
> > [~1000 lines of fullquote deleted until I hit the first comment, sigh..]
> > 
> > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > > > index 157f72efec5e9..dfa1533b4edfc 100644
> > > > --- a/fs/xfs/scrub/bmap.c
> > > > +++ b/fs/xfs/scrub/bmap.c
> > > > @@ -598,7 +598,7 @@ xchk_bmap_check_rmaps(
> > > >  		size = 0;
> > > >  		break;
> > > >  	}
> > > > -	if (XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> > > > +	if (ifp->if_format != XFS_DINODE_FMT_BTREE &&
> > > 
> > > ifp can be null here if bmapbt scrub is called on a file that has no
> > > xattrs; this crashed my test vm immediately...
> > 
> > What tests is that?  And xfstests auto run did not hit it, even if a
> > NULL check here seems sensible.
> 
> In my case it was the xfs_scrub run after generic/001 that did it.
> 
> I think we're covered against null *ifp in most cases because they're
> guarded by an if(XFS_IFORK_Q()); it's jut here where I went around
> shortcutting.  Maybe I should just fix this function for you... :)

Hmm, that sounded meaner than I intended it to be. :/

Also, it turns out that it's pretty easy to fix this as part of fixing
the contorted logic in patch 1 (aka xchk_bmap_check_rmaps) so I'll do
that there.

> --D