Re: [PATCH 4/4] xfs: remove xfs_page_mkwrite_iomap_ops

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Oct 29, 2024 at 04:12:00PM +0100, Christoph Hellwig wrote:
> Shared the regular buffered write iomap_ops with the page fault path
> and just check for the IOMAP_FAULT flag to skip delalloc punching.
> 
> This keeps the delalloc punching checks in one place, and will make it
> easier to convert iomap to an iter model where the begin and end
> handlers are merged into a single callback.

"merged into a single callback"?  What plans are these? :)

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Code changes here look ok to me, so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c  |  2 +-
>  fs/xfs/xfs_iomap.c | 17 ++++++++---------
>  fs/xfs/xfs_iomap.h |  1 -
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 7464d874e766..c6de6b865ef1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1474,7 +1474,7 @@ xfs_write_fault(
>  	if (IS_DAX(inode))
>  		ret = xfs_dax_fault_locked(vmf, order, true);
>  	else
> -		ret = iomap_page_mkwrite(vmf, &xfs_page_mkwrite_iomap_ops);
> +		ret = iomap_page_mkwrite(vmf, &xfs_buffered_write_iomap_ops);
>  	xfs_iunlock(ip, lock_mode);
>  
>  	sb_end_pagefault(inode->i_sb);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 916531d9f83c..bfc5b0a4d633 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1234,6 +1234,14 @@ xfs_buffered_write_iomap_end(
>  	if (iomap->type != IOMAP_DELALLOC || !(iomap->flags & IOMAP_F_NEW))
>  		return 0;
>  
> +	/*
> +	 * iomap_page_mkwrite() will never fail in a way that requires delalloc
> +	 * extents that it allocated to be revoked.  Hence never try to release
> +	 * them here.
> +	 */
> +	if (flags & IOMAP_FAULT)
> +		return 0;
> +
>  	/* Nothing to do if we've written the entire delalloc extent */
>  	start_byte = iomap_last_written_block(inode, offset, written);
>  	end_byte = round_up(offset + length, i_blocksize(inode));
> @@ -1260,15 +1268,6 @@ const struct iomap_ops xfs_buffered_write_iomap_ops = {
>  	.iomap_end		= xfs_buffered_write_iomap_end,
>  };
>  
> -/*
> - * iomap_page_mkwrite() will never fail in a way that requires delalloc extents
> - * that it allocated to be revoked. Hence we do not need an .iomap_end method
> - * for this operation.
> - */
> -const struct iomap_ops xfs_page_mkwrite_iomap_ops = {
> -	.iomap_begin		= xfs_buffered_write_iomap_begin,
> -};
> -
>  static int
>  xfs_read_iomap_begin(
>  	struct inode		*inode,
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index 4da13440bae9..8347268af727 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -48,7 +48,6 @@ xfs_aligned_fsb_count(
>  }
>  
>  extern const struct iomap_ops xfs_buffered_write_iomap_ops;
> -extern const struct iomap_ops xfs_page_mkwrite_iomap_ops;
>  extern const struct iomap_ops xfs_direct_write_iomap_ops;
>  extern const struct iomap_ops xfs_read_iomap_ops;
>  extern const struct iomap_ops xfs_seek_iomap_ops;
> -- 
> 2.45.2
> 
>