Re: [PATCH 1/3] xfs: add a xfs_valid_startblock helper

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

On Sun, Sep 01, 2019 at 09:36:34AM +0200, Christoph Hellwig wrote:
> On Fri, Aug 30, 2019 at 05:32:53PM +0200, Christoph Hellwig wrote:
> > On Fri, Aug 30, 2019 at 08:06:50AM -0700, Darrick J. Wong wrote:
> > > > --- a/fs/xfs/libxfs/xfs_bmap.h
> > > > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > > > @@ -171,6 +171,9 @@ static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec)
> > > >  		!isnullstartblock(irec->br_startblock);
> > > >  }
> > > >  
> > > > +#define xfs_valid_startblock(ip, startblock) \
> > > > +	((startblock) != 0 || XFS_IS_REALTIME_INODE(ip))
> > > 
> > > We have more robust validators for data/rtdev fsblock_t, so why not:
> > > 
> > > #define xfs_valid_startblock(ip, startblock) \
> > > 	(XFS_IS_REALTIME_INODE(ip) ? xfs_verify_rtbno(startblock) : \
> > > 				     xfs_verify_fsbno(startblock))
> > > 
> > > and why not make it a static inline function too?
> > 
> > I tried an inline function, but I could not find a header to place
> > it that would actually easily compile everywhere...  Maybe we should
> > just make that a xfs_verify_bno(mp, startblock) and move that out of
> > line such in a way that a smart compiler avoids the function call
> > overhead for xfs_verify_rtbno / xfs_verify_fsbno.  I'll take another
> > stab at this.
> 
> So I looked into your suggestion, but xfs_verify_rtbno / xfs_verify_fsbno
> do a lot of validity checking, but they don't actually contain the
> check that was in the existing code.  The bmap code just checks that
> there is a startblock of 0 for non-rt devices, probably this was added
> to find some old bug where a irec structure that was zeroed was returned.
> 
> So replacing it with xfs_verify_rtbno / xfs_verify_fsbno would not help
> in any way.  But the big question is if keeping the 0 check is even
> worth it.

It's been mildly helpful for noticing when my online/offline repair
prototype code totally screws up, but at that point so much magic smoke
is already pouring out everywhere that it's hard not to notice. :)

--D