Re: [PATCH] xfs: fix write failures in software-provided atomic writes

[John Garry <john.g.garry@oracle.com>]

On 31/10/2025 04:30, Darrick J. Wong wrote:
>> @@ -1215,6 +1216,7 @@ xfs_atomic_write_cow_iomap_begin(
>>   	return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq);
>>
>> I think that the problem may be that we were converting an inappropriate
>> number of blocks from unwritten to real allocations (but never writing to
>> the excess blocks). Does it look ok?
> That looks like a good correction to me; I'll run that on my test fleet
> overnight and we'll see what happens.  Thanks for putting this together!

Cool, but I am not confident that it is a completely correct. Here's the 
updated code:

  	int			error;
  	u64			seq;
+	xfs_filblks_t		count_fsb_orig = count_fsb;

  	ASSERT(flags & IOMAP_WRITE);
  	ASSERT(flags & IOMAP_DIRECT);
@@ -1202,7 +1203,7 @@ xfs_atomic_write_cow_iomap_begin(
  found:
  	if (cmap.br_state != XFS_EXT_NORM) {
  		error = xfs_reflink_convert_cow_locked(ip, offset_fsb,
-				count_fsb);
+				count_fsb_orig);
  		if (error)
  			goto out_unlock;
  		cmap.br_state = XFS_EXT_NORM;

cmap may be longer than count_fsb_orig (which was the failing scenario). 
In that case, after calling xfs_reflink_convert_cow_locked(), we would 
have partially converted cmap, so it is proper to set cmap.br_state = 
XFS_EXT_NORM? We should trim cmap to count_fsb_orig also, right?

I don't think that it makes much of a difference, but it seems the 
proper thing to do. Maybe the subsequent traces length values would be 
inconsistent with other path to @found label if we don't trim.

Thank,
John