Re: [PATCH 2/3] xfs: clean up the metadata validation in xfs_swap_extent_rmap

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

On Wed, May 06, 2020 at 07:56:33AM -0700, Christoph Hellwig wrote:
> On Mon, May 04, 2020 at 06:10:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Bail out if there's something not right with either file's fork
> > mappings.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_util.c |   31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index cc23a3e23e2d..2774939e176d 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1342,8 +1342,16 @@ xfs_swap_extent_rmap(
> >  				&nimaps, 0);
> >  		if (error)
> >  			goto out;
> > -		ASSERT(nimaps == 1);
> > -		ASSERT(tirec.br_startblock != DELAYSTARTBLOCK);
> > +		if (nimaps != 1 || tirec.br_startblock == DELAYSTARTBLOCK) {
> > +			/*
> > +			 * We should never get no mapping or a delalloc extent
> > +			 * since the donor file should have been flushed by the
> > +			 * caller.
> > +			 */
> > +			ASSERT(0);
> > +			error = -EINVAL;
> > +			goto out;
> > +		}
> 
> I'm not even sure the !nimaps case still exists.  Usually this will
> return a hole extent, which we don't seem to handle here?

xfs_bmap_unmap_extent and xfs_bmap_map_extent are NOPs if you pass
them a hole.

But yeah, the !nimaps case doesn't seem to exist anymore.

> In general I think this code would be improved quite a bit by using
> xfs_iext_lookup_extent instead of xfs_bmapi_read.
> 
> Same for the second hunk.

I'd rather not make major changes to this code because my long range
plan is to replace all this with a much cleaner implementation in the
atomic file update series[1].  This patchset bandages the wtfs that I
found while writing that series, projecting that review of atomic file
updates is going to take a while...

--D

[1] https://lwn.net/ml/linux-fsdevel/158812825316.168506.932540609191384366.stgit@magnolia/