Re: [PATCH 6.17.y 2/2] xfs: fix various problems in xfs_atomic_write_cow_iomap_begin

[Sasha Levin <sashal@kernel.org>]

On Fri, Nov 21, 2025 at 10:47:50AM +0100, Greg KH wrote:
>On Sun, Nov 09, 2025 at 06:20:57PM -0500, Sasha Levin wrote:
>> From: "Darrick J. Wong" <djwong@kernel.org>
>>
>> [ Upstream commit 8d7bba1e8314013ecc817a91624104ceb9352ddc ]
>>
>> I think there are several things wrong with this function:
>>
>> A) xfs_bmapi_write can return a much larger unwritten mapping than what
>>    the caller asked for.  We convert part of that range to written, but
>>    return the entire written mapping to iomap even though that's
>>    inaccurate.
>>
>> B) The arguments to xfs_reflink_convert_cow_locked are wrong -- an
>>    unwritten mapping could be *smaller* than the write range (or even
>>    the hole range).  In this case, we convert too much file range to
>>    written state because we then return a smaller mapping to iomap.
>>
>> C) It doesn't handle delalloc mappings.  This I covered in the patch
>>    that I already sent to the list.
>>
>> D) Reassigning count_fsb to handle the hole means that if the second
>>    cmap lookup attempt succeeds (due to racing with someone else) we
>>    trim the mapping more than is strictly necessary.  The changing
>>    meaning of count_fsb makes this harder to notice.
>>
>> E) The tracepoint is kinda wrong because @length is mutated.  That makes
>>    it harder to chase the data flows through this function because you
>>    can't just grep on the pos/bytecount strings.
>>
>> F) We don't actually check that the br_state = XFS_EXT_NORM assignment
>>    is accurate, i.e that the cow fork actually contains a written
>>    mapping for the range we're interested in
>>
>> G) Somewhat inadequate documentation of why we need to xfs_trim_extent
>>    so aggressively in this function.
>>
>> H) Not sure why xfs_iomap_end_fsb is used here, the vfs already clamped
>>    the write range to s_maxbytes.
>>
>> Fix these issues, and then the atomic writes regressions in generic/760,
>> generic/617, generic/091, generic/263, and generic/521 all go away for
>> me.
>>
>> Cc: stable@vger.kernel.org # v6.16
>> Fixes: bd1d2c21d5d249 ("xfs: add xfs_atomic_write_cow_iomap_begin()")
>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>> Reviewed-by: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Carlos Maiolino <cem@kernel.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>  fs/xfs/xfs_iomap.c | 61 +++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 50 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index a4a22975c7cc9..0cf84b25d6567 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -1082,6 +1082,29 @@ const struct iomap_ops xfs_zoned_direct_write_iomap_ops = {
>>  };
>>  #endif /* CONFIG_XFS_RT */
>>
>> +#ifdef DEBUG
>> +static void
>> +xfs_check_atomic_cow_conversion(
>> +	struct xfs_inode		*ip,
>> +	xfs_fileoff_t			offset_fsb,
>> +	xfs_filblks_t			count_fsb,
>> +	const struct xfs_bmbt_irec	*cmap)
>> +{
>> +	struct xfs_iext_cursor		icur;
>> +	struct xfs_bmbt_irec		cmap2 = { };
>> +
>> +	if (xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &icur, &cmap2))
>> +		xfs_trim_extent(&cmap2, offset_fsb, count_fsb);
>> +
>> +	ASSERT(cmap2.br_startoff == cmap->br_startoff);
>> +	ASSERT(cmap2.br_blockcount == cmap->br_blockcount);
>> +	ASSERT(cmap2.br_startblock == cmap->br_startblock);
>> +	ASSERT(cmap2.br_state == cmap->br_state);
>> +}
>> +#else
>> +# define xfs_check_atomic_cow_conversion(...)	((void)0)
>> +#endif
>> +
>>  static int
>>  xfs_atomic_write_cow_iomap_begin(
>>  	struct inode		*inode,
>> @@ -1093,9 +1116,10 @@ xfs_atomic_write_cow_iomap_begin(
>>  {
>>  	struct xfs_inode	*ip = XFS_I(inode);
>>  	struct xfs_mount	*mp = ip->i_mount;
>> -	const xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> -	xfs_fileoff_t		end_fsb = xfs_iomap_end_fsb(mp, offset, length);
>> -	xfs_filblks_t		count_fsb = end_fsb - offset_fsb;
>> +	const xfs_fileoff_t	offset_fsb = XFS_B_TO_FSBT(mp, offset);
>> +	const xfs_fileoff_t	end_fsb = XFS_B_TO_FSB(mp, offset + length);
>> +	const xfs_filblks_t	count_fsb = end_fsb - offset_fsb;
>> +	xfs_filblks_t		hole_count_fsb;
>>  	int			nmaps = 1;
>>  	xfs_filblks_t		resaligned;
>>  	struct xfs_bmbt_irec	cmap;
>> @@ -1134,14 +1158,20 @@ xfs_atomic_write_cow_iomap_begin(
>>  	if (cmap.br_startoff <= offset_fsb) {
>>  		if (isnullstartblock(cmap.br_startblock))
>>  			goto convert_delay;
>> +
>> +		/*
>> +		 * cmap could extend outside the write range due to previous
>> +		 * speculative preallocations.  We must trim cmap to the write
>> +		 * range because the cow fork treats written mappings to mean
>> +		 * "write in progress".
>> +		 */
>>  		xfs_trim_extent(&cmap, offset_fsb, count_fsb);
>>  		goto found;
>>  	}
>>
>> -	end_fsb = cmap.br_startoff;
>> -	count_fsb = end_fsb - offset_fsb;
>> +	hole_count_fsb = cmap.br_startoff - offset_fsb;
>>
>> -	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
>> +	resaligned = xfs_aligned_fsb_count(offset_fsb, hole_count_fsb,
>>  			xfs_get_cowextsz_hint(ip));
>>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>
>> @@ -1177,7 +1207,7 @@ xfs_atomic_write_cow_iomap_begin(
>>  	 * atomic writes to that same range will be aligned (and don't require
>>  	 * this COW-based method).
>>  	 */
>> -	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
>> +	error = xfs_bmapi_write(tp, ip, offset_fsb, hole_count_fsb,
>>  			XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC |
>>  			XFS_BMAPI_EXTSZALIGN, 0, &cmap, &nmaps);
>>  	if (error) {
>> @@ -1190,17 +1220,26 @@ xfs_atomic_write_cow_iomap_begin(
>>  	if (error)
>>  		goto out_unlock;
>>
>> +	/*
>> +	 * cmap could map more blocks than the range we passed into bmapi_write
>> +	 * because of EXTSZALIGN or adjacent pre-existing unwritten mappings
>> +	 * that were merged.  Trim cmap to the original write range so that we
>> +	 * don't convert more than we were asked to do for this write.
>> +	 */
>> +	xfs_trim_extent(&cmap, offset_fsb, count_fsb);
>> +
>>  found:
>>  	if (cmap.br_state != XFS_EXT_NORM) {
>> -		error = xfs_reflink_convert_cow_locked(ip, offset_fsb,
>> -				count_fsb);
>> +		error = xfs_reflink_convert_cow_locked(ip, cmap.br_startoff,
>> +				cmap.br_blockcount);
>>  		if (error)
>>  			goto out_unlock;
>>  		cmap.br_state = XFS_EXT_NORM;
>> +		xfs_check_atomic_cow_conversion(ip, offset_fsb, count_fsb,
>> +				&cmap);
>>  	}
>>
>> -	length = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
>> -	trace_xfs_iomap_found(ip, offset, length - offset, XFS_COW_FORK, &cmap);
>> +	trace_xfs_iomap_found(ip, offset, length, XFS_COW_FORK, &cmap);
>>  	seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED);
>>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>  	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
>> --
>> 2.51.0
>>
>>
>
>Also did not apply to 6.17.y :(

You pulled these two for the previous release, so they're already in tree :)

-- 
Thanks,
Sasha