Re: [RFC PATCH v3 05/11] xfs: Add RWF_WRITETHROUGH support to xfs

[Ojaswin Mujoo]

On Wed, Aug 05, 2026 at 11:58:11AM +0530, Ojaswin Mujoo wrote:
> Add the boilerplate needed to start supporting RWF_WRITETHROUGH in XFS.
> We use the direct write ->iomap_begin() functions to ensure the range
> under write through always has a real non-delalloc extent. We reuse the xfs
> dio's end IO function to perform extent conversion and i_size handling
> for us.
> 
> *Note on COW extent over DATA hole case*
> 
> In case of an unmapped COW extent over a DATA hole
> (due to COW preallocations), leave the extent unmapped until we are just
> about to send IO. At that time, use the ->writethrough_submit() call
> back to convert the COW extent to written.
> 
> We initially tried converting during iomap begin() time (like dio does)
> but that results in a stale data exposure as follows:
> 
> 1. iomap begin() - converts COW extent over DATA hole to written and
> marks IOMAP_F_NEW to handle zeroing.
> 2. During iomap_write_begin() -> realise extent is stale and return back
> without zeroing.
> 3. iomap begin() - Again sees the same COW extent but it's written
> this time so we don't mark IOMAP_F_NEW
> 4. Since IOMAP_F_NEW is unmarked, we never zeroout and hence expose
> stale data.
> 
> To avoid the above, take the buffered IO approach of converting the
> extent just before IO, when we are sure to have zeroed out the folio.
> 
> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
> Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> ---
>  fs/xfs/xfs_file.c | 83 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 768cabf6250b..4b45ceacd461 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -702,6 +702,36 @@ static const struct iomap_dio_ops xfs_dio_write_ops = {
>  	.end_io		= xfs_dio_write_end_io,
>  };
>  
> +static int
> +xfs_writethrough_end_io(
> +	struct iomap_writethrough_ctx	*wt_ctx,
> +	ssize_t				size,
> +	int				error,
> +	unsigned int			flags)
> +{
> +	struct xfs_inode *ip = XFS_I(wt_ctx->inode);
> +	xfs_off_t offset = wt_ctx->iocb->ki_pos;
> +
> +	if (unlikely(error)) {
> +		if (wt_ctx->flags & IOMAP_DIO_COW)
> +			xfs_reflink_cancel_cow_range(ip, offset, size, true);
> +
> +		return error;
> +	}
> +
> +	/*
> +	 * writethrough completions are handled same as dio with the exception
> +	 * that we need to explicitly change the i_disk_size. This is because
> +	 * unlike dio, we have already updated the i_size and hence the
> +	 * (i_disk_size < i_size) check will fail in dio code
> +	 */
> +	xfs_dio_write_end_io(wt_ctx->iocb, size, error, flags);
> +	if (offset + size > ip->i_disk_size)

Okay so mostly the Sashiko reviews are straight forward but I would like
to discuss one of it here:

  Is it safe to read ip->i_disk_size locklessly here?

  On 32-bit systems, a lockless read of a 64-bit value can result in a torn
  read. If another thread is concurrently updating i_disk_size via
  xfs_setfilesize(), a torn read might yield a spuriously large value.

  Could this cause the offset + size > ip->i_disk_size condition to evaluate
  to false, skipping the on-disk size update entirely?

Although this seems like a valid concern and we can end up tearing
i_disk_size read on 32 bit machines which can lead to a skipped disksize
update, I see that same sort of unsafe checks for i_disk_size is also
present in the endio code (xfs_ioend_is_append() call)

Shouldnt we ideally be fixing those sites as well or is there something
I'm missing in xfs that makes this acceptable?

Regards,
ojaswin

> +		return xfs_setfilesize(ip, offset, size);
> +
> +	return 0;
> +}
> +
>  static void
>  xfs_dio_zoned_submit_io(
>  	const struct iomap_iter	*iter,
> @@ -1033,6 +1063,39 @@ xfs_file_dax_write(
>  	return ret;
>  }
>  
> +static int
> +xfs_writethrough_submit(
> +	struct inode		*inode,
> +	struct iomap		*iomap,
> +	loff_t			offset,
> +	u64			count)
> +{
> +	int error = 0;
> +	unsigned int		nofs_flag;
> +
> +	/*
> +	 * Convert CoW extents to regular.
> +	 *
> +	 * We are under writethrough context with folio lock possibly held. To
> +	 * avoid memory allocation deadlocks, set the task-wide nofs context.
> +	 */
> +	if (iomap->flags & IOMAP_F_SHARED) {
> +		nofs_flag = memalloc_nofs_save();
> +		error = xfs_reflink_convert_cow(XFS_I(inode), offset, count);
> +		memalloc_nofs_restore(nofs_flag);
> +	}
> +
> +	return error;
> +}
> +
> +const struct iomap_writethrough_ops xfs_writethrough_ops = {
> +	.ops			= &xfs_direct_write_iomap_ops,
> +	.write_ops		= &xfs_iomap_write_ops,
> +	.end_io		= xfs_writethrough_end_io,
> +	.writethrough_submit	= &xfs_writethrough_submit
> +};
> +
> +
>  STATIC ssize_t
>  xfs_file_buffered_write(
>  	struct kiocb		*iocb,
> @@ -1055,9 +1118,13 @@ xfs_file_buffered_write(
>  		goto out;
>  
>  	trace_xfs_file_buffered_write(iocb, from);
> -	ret = iomap_file_buffered_write(iocb, from,
> -			&xfs_buffered_write_iomap_ops, &xfs_iomap_write_ops,
> -			NULL);
> +	if (iocb->ki_flags & IOCB_WRITETHROUGH) {
> +		ret = iomap_file_writethrough_write(iocb, from,
> +						    &xfs_writethrough_ops, NULL);
> +	} else
> +		ret = iomap_file_buffered_write(iocb, from,
> +						&xfs_buffered_write_iomap_ops,
> +						&xfs_iomap_write_ops, NULL);
>  
>  	/*
>  	 * If we hit a space limit, try to free up some lingering preallocated
> @@ -1092,8 +1159,12 @@ xfs_file_buffered_write(
>  
>  	if (ret > 0) {
>  		XFS_STATS_ADD(ip->i_mount, xs_write_bytes, ret);
> -		/* Handle various SYNC-type writes */
> -		ret = generic_write_sync(iocb, ret);
> +		/*
> +		 * Handle various SYNC-type writes.
> +		 * For writethrough, we handle sync during completion.
> +		 */
> +		if (!(iocb->ki_flags & IOCB_WRITETHROUGH))
> +			ret = generic_write_sync(iocb, ret);
>  	}
>  	return ret;
>  }
> @@ -2104,7 +2175,7 @@ const struct file_operations xfs_file_operations = {
>  	.remap_file_range = xfs_file_remap_range,
>  	.fop_flags	= FOP_MMAP_SYNC | FOP_BUFFER_RASYNC |
>  			  FOP_BUFFER_WASYNC | FOP_DIO_PARALLEL_WRITE |
> -			  FOP_DONTCACHE,
> +			  FOP_DONTCACHE | FOP_WRITETHROUGH,
>  	.setlease	= generic_setlease,
>  };
>  
> -- 
> 2.55.0
> 
>