Re: [PATCH 6/9] xfs: reflink can skip remap existing mappings

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

On Thu, Jun 25, 2020 at 08:28:18AM -0400, Brian Foster wrote:
> On Wed, Jun 24, 2020 at 06:18:18PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If the source and destination map are identical, we can skip the remap
> > step to save some time.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_reflink.c |   17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index 72de7179399d..f1156f121b7d 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -1031,6 +1031,23 @@ xfs_reflink_remap_extent(
> >  
> >  	trace_xfs_reflink_remap_extent_dest(ip, &smap);
> >  
> > +	/*
> > +	 * Two extents mapped to the same physical block must not have
> > +	 * different states; that's filesystem corruption.  Move on to the next
> > +	 * extent if they're both holes or both the same physical extent.
> > +	 */
> > +	if (dmap->br_startblock == smap.br_startblock) {
> > +		ASSERT(dmap->br_startblock == smap.br_startblock);
> 
> That assert duplicates the logic in the if statement. Was this intended
> to be the length check I asked for? If so it looks like that was added
> previously so perhaps this can just drop off. With that fixed up:

Yep, I got hopelessly distracted while trying to figure out why Dave's
inode flush series keep crashing here. Will fix. :/

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > +		if (dmap->br_state != smap.br_state)
> > +			error = -EFSCORRUPTED;
> > +		goto out_cancel;
> > +	}
> > +
> > +	/* If both extents are unwritten, leave them alone. */
> > +	if (dmap->br_state == XFS_EXT_UNWRITTEN &&
> > +	    smap.br_state == XFS_EXT_UNWRITTEN)
> > +		goto out_cancel;
> > +
> >  	/* No reflinking if the AG of the dest mapping is low on space. */
> >  	if (dmap_written) {
> >  		error = xfs_reflink_ag_has_free_space(mp,
> > 
>