Re: [PATCH] xfs: stop exchanging reflink flags during mapping exchanges

[Darrick J. Wong]

On Fri, Oct 02, 2026 at 03:38:22PM +0200, Norbert Szetei wrote:
> When an exchange covers the whole of both files,
> xmi_can_exchange_reflink_flags() moves the reflink inode flag from the
> file that has it to the other one, deciding that from req->blockcount
> against XFS_B_TO_FSB(mp, i_disk_size) on each inode.
> 
> i_disk_size does not bound an inode's mappings, and
> XFS_EXCHMAPS_SET_SIZES creates that state by assigning both i_disk_size
> values from the sizes sampled before the operation without unmapping
> anything above the new size. An exchange over [0, i_disk_size) can
> therefore pass every test in the function while an inode still owns
> shared mappings above EOF, and the post-operation cleanup clears its

How do you end up with shared mappings above EOF?  You write a /lot/ of
sentences here but weirdly none of it explains how this key assumption
is violated.

> reflink flag anyway. Later writes to those mappings take the non-reflink
> write path and update blocks that should still have been protected by
> CoW, which shows up as data corruption between reflink-related files and
> as an rmap overlap that xfs_rmap_convert() rejects.
> 
> Commit a23eca88448e ("xfs: fix exchange-range reflink flag clearing
> issue with INO1_WRITTEN") disabled the flag exchange for
> XFS_EXCHMAPS_INO1_WRITTEN requests. Fix this variant by not exchanging
> the flags at all, which subsumes that guard. Whether an inode still owns
> shared blocks outside the exchanged range is not recorded in the data
> fork, so answering it takes a refcount btree lookup, and
> xfs_exchmaps_init_intent() has no transaction to do that with, cannot
> report an error, and runs with both ILOCKs dropped in recovery. Deciding
> it correctly means doing that lookup in xfs_exchrange_mappings(), which
> holds both ILOCKs and a transaction and can return an error. The
> conservative outcome here is that both inodes keep the reflink flag.

Or you could cross-reference the refcount btree with any mappings you
find beyond i_disk_size.

> XFS_EXCHMAPS_CLEAR_INO{1,2}_REFLINK were only ever set by the decision
> this patch removes, so xfs_exchmaps_clear_reflink() and the code that
> consumed them are unreachable and go too. An intent logged by an older
> kernel does carry the bits, but xfs_xmi_item_recover_intent() has always
> masked them off with XFS_EXCHMAPS_PARAMS, and the clearing only survived
> recovery because init_intent re-derived the decision. Recovering such an
> intent now completes the exchange and leaves both reflink flags set.
> 
> The flag that xfs_exchmaps_ensure_reflink() copies to the peer inode is
> now permanent until a FALLOC_FL_UNSHARE_RANGE, an xfs_scrub run or a
> truncate to empty clears it. Until then that inode keeps the paths
> xfs_is_cow_inode() selects, ILOCK_EXCL for buffered writes and the
> -ENOTBLK bounce for unaligned direct writes, and
> xchk_inode_check_reflink_iflag() preens it on every scrub.
> 
> Fixes: 966ceafc7a43 ("xfs: create deferred log items for file mapping exchanges")
> Cc: stable@xxxxxxxxxxxxxxx # v6.10
> Assisted-by: LLM

Oh, this was all slop?  Wonderful.

> Signed-off-by: Norbert Szetei <norbert@xxxxxxxxxxxx>
> ---
> A reproducer is available on request.

POC || GTFO.  I'm not going to play 20 questions here.

--D

>  fs/xfs/libxfs/xfs_exchmaps.c | 86 +++++-------------------------------
>  1 file changed, 10 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_exchmaps.c b/fs/xfs/libxfs/xfs_exchmaps.c
> index 6a66b6075e0a..07288f4163b3 100644
> --- a/fs/xfs/libxfs/xfs_exchmaps.c
> +++ b/fs/xfs/libxfs/xfs_exchmaps.c
> @@ -127,9 +127,7 @@ xmi_has_more_exchange_work(const struct xfs_exchmaps_intent *xmi)
>  static inline bool
>  xmi_has_postop_work(const struct xfs_exchmaps_intent *xmi)
>  {
> -	return xmi->xmi_flags & (XFS_EXCHMAPS_CLEAR_INO1_REFLINK |
> -				 XFS_EXCHMAPS_CLEAR_INO2_REFLINK |
> -				 __XFS_EXCHMAPS_INO2_SHORTFORM);
> +	return xmi->xmi_flags & __XFS_EXCHMAPS_INO2_SHORTFORM;
>  }
>  
>  /* Check all mappings to make sure we can actually exchange them. */
> @@ -525,18 +523,6 @@ xfs_exchmaps_link_to_sf(
>  	return error;
>  }
>  
> -/* Clear the reflink flag after an exchange. */
> -static inline void
> -xfs_exchmaps_clear_reflink(
> -	struct xfs_trans	*tp,
> -	struct xfs_inode	*ip)
> -{
> -	trace_xfs_reflink_unset_inode_flag(ip);
> -
> -	ip->i_diflags2 &= ~XFS_DIFLAG2_REFLINK;
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -}
> -
>  /* Finish whatever work might come after an exchange operation. */
>  static int
>  xfs_exchmaps_do_postop_work(
> @@ -557,16 +543,6 @@ xfs_exchmaps_do_postop_work(
>  			return error;
>  	}
>  
> -	if (xmi->xmi_flags & XFS_EXCHMAPS_CLEAR_INO1_REFLINK) {
> -		xfs_exchmaps_clear_reflink(tp, xmi->xmi_ip1);
> -		xmi->xmi_flags &= ~XFS_EXCHMAPS_CLEAR_INO1_REFLINK;
> -	}
> -
> -	if (xmi->xmi_flags & XFS_EXCHMAPS_CLEAR_INO2_REFLINK) {
> -		xfs_exchmaps_clear_reflink(tp, xmi->xmi_ip2);
> -		xmi->xmi_flags &= ~XFS_EXCHMAPS_CLEAR_INO2_REFLINK;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -948,46 +924,21 @@ xfs_exchmaps_intent_destroy_cache(void)
>  }
>  
>  /*
> - * Decide if we will exchange the reflink flags between the two files after the
> - * exchange.  The only time we want to do this is if we're exchanging all
> - * mappings under EOF and the inode reflink flags have different states.
> + * Allocate and initialize a new incore intent item from a request.
> + *
> + * Note that this does not decide anything about the two inodes' reflink flags.
> + * Clearing XFS_DIFLAG2_REFLINK off an inode needs to know whether it still owns
> + * shared blocks outside the exchanged range, which is not recorded in the data
> + * fork and takes a refcount btree lookup.  This function has no transaction to
> + * do that with and no way to report a failure, so both flags are left as they
> + * are.  xfs_exchmaps_ensure_reflink() copies the flag to the peer inode when the
> + * exchange runs.
>   */
> -static inline bool
> -xmi_can_exchange_reflink_flags(
> -	const struct xfs_exchmaps_req	*req,
> -	unsigned int			reflink_state)
> -{
> -	struct xfs_mount		*mp = req->ip1->i_mount;
> -
> -	/*
> -	 * The INO1_WRITTEN optimization can skip exchanging hole and
> -	 * unwritten mappings, which means we cannot guarantee that all
> -	 * shared extents actually moved to the other file.  Clearing the
> -	 * reflink flag of an inode that still holds shared extents breaks
> -	 * the CoW write path, so refuse to exchange the flags in that case.
> -	 */
> -	if (req->flags & XFS_EXCHMAPS_INO1_WRITTEN)
> -		return false;
> -
> -	if (hweight32(reflink_state) != 1)
> -		return false;
> -	if (req->startoff1 != 0 || req->startoff2 != 0)
> -		return false;
> -	if (req->blockcount != XFS_B_TO_FSB(mp, req->ip1->i_disk_size))
> -		return false;
> -	if (req->blockcount != XFS_B_TO_FSB(mp, req->ip2->i_disk_size))
> -		return false;
> -	return true;
> -}
> -
> -
> -/* Allocate and initialize a new incore intent item from a request. */
>  struct xfs_exchmaps_intent *
>  xfs_exchmaps_init_intent(
>  	const struct xfs_exchmaps_req	*req)
>  {
>  	struct xfs_exchmaps_intent	*xmi;
> -	unsigned int			rs = 0;
>  
>  	xmi = kmem_cache_zalloc(xfs_exchmaps_intent_cache,
>  			GFP_NOFS | __GFP_NOFAIL);
> @@ -1011,23 +962,6 @@ xfs_exchmaps_init_intent(
>  		xmi->xmi_isize2 = req->ip1->i_disk_size;
>  	}
>  
> -	/* Record the state of each inode's reflink flag before the op. */
> -	if (xfs_is_reflink_inode(req->ip1))
> -		rs |= 1;
> -	if (xfs_is_reflink_inode(req->ip2))
> -		rs |= 2;
> -
> -	/*
> -	 * Figure out if we're clearing the reflink flags (which effectively
> -	 * exchanges them) after the operation.
> -	 */
> -	if (xmi_can_exchange_reflink_flags(req, rs)) {
> -		if (rs & 1)
> -			xmi->xmi_flags |= XFS_EXCHMAPS_CLEAR_INO1_REFLINK;
> -		if (rs & 2)
> -			xmi->xmi_flags |= XFS_EXCHMAPS_CLEAR_INO2_REFLINK;
> -	}
> -
>  	if (S_ISDIR(VFS_I(xmi->xmi_ip2)->i_mode) ||
>  	    S_ISLNK(VFS_I(xmi->xmi_ip2)->i_mode))
>  		xmi->xmi_flags |= __XFS_EXCHMAPS_INO2_SHORTFORM;
> -- 
> 2.55.0
> 
>