Re: [PATCH 3/7] xfs: use byte ranges for write cleanup ranges

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

On Tue, Nov 01, 2022 at 11:34:08AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_buffered_write_iomap_end() currently converts the byte ranges
> passed to it to filesystem blocks to pass them to the bmap code to
> punch out delalloc blocks, but then has to convert filesytem
> blocks back to byte ranges for page cache truncate.
> 
> We're about to make the page cache truncate go away and replace it
> with a page cache walk, so having to convert everything to/from/to
> filesystem blocks is messy and error-prone. It is much easier to
> pass around byte ranges and convert to page indexes and/or
> filesystem blocks only where those units are needed.
> 
> In preparation for the page cache walk being added, add a helper
> that converts byte ranges to filesystem blocks and calls
> xfs_bmap_punch_delalloc_range() and convert
> xfs_buffered_write_iomap_end() to calculate limits in byte ranges.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_iomap.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index a2e45ea1b0cb..7bb55dbc19d3 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1120,6 +1120,20 @@ xfs_buffered_write_iomap_begin(
>  	return error;
>  }
>  
> +static int
> +xfs_buffered_write_delalloc_punch(
> +	struct inode		*inode,
> +	loff_t			start_byte,
> +	loff_t			end_byte)
> +{
> +	struct xfs_mount	*mp = XFS_M(inode->i_sb);
> +	xfs_fileoff_t		start_fsb = XFS_B_TO_FSBT(mp, start_byte);
> +	xfs_fileoff_t		end_fsb = XFS_B_TO_FSB(mp, end_byte);
> +
> +	return xfs_bmap_punch_delalloc_range(XFS_I(inode), start_fsb,
> +				end_fsb - start_fsb);
> +}

/me echoes hch's comment that the other callers of
xfs_bmap_punch_delalloc_range do this byte->block conversion too.

> +
>  static int
>  xfs_buffered_write_iomap_end(
>  	struct inode		*inode,
> @@ -1129,10 +1143,9 @@ xfs_buffered_write_iomap_end(
>  	unsigned		flags,
>  	struct iomap		*iomap)
>  {
> -	struct xfs_inode	*ip = XFS_I(inode);
> -	struct xfs_mount	*mp = ip->i_mount;
> -	xfs_fileoff_t		start_fsb;
> -	xfs_fileoff_t		end_fsb;
> +	struct xfs_mount	*mp = XFS_M(inode->i_sb);
> +	loff_t			start_byte;
> +	loff_t			end_byte;
>  	int			error = 0;
>  
>  	if (iomap->type != IOMAP_DELALLOC)
> @@ -1157,13 +1170,13 @@ xfs_buffered_write_iomap_end(
>  	 * the range.
>  	 */
>  	if (unlikely(!written))
> -		start_fsb = XFS_B_TO_FSBT(mp, offset);
> +		start_byte = round_down(offset, mp->m_sb.sb_blocksize);
>  	else
 -		start_fsb = XFS_B_TO_FSB(mp, offset + written);
> -	end_fsb = XFS_B_TO_FSB(mp, offset + length);
> +		start_byte = round_up(offset + written, mp->m_sb.sb_blocksize);
> +	end_byte = round_up(offset + length, mp->m_sb.sb_blocksize);

Technically this is the byte where we should *stop* processing, right?

If we are told to write 1000 bytes at pos 0 and the whole thing fails,
the end pos of the range is 1023 and we must stop at pos 1024.  Right?

(The only reason I ask is that Linus ranted about XFS naming these
variables incorrectly in the iomap code and the (at the time only) user
of it.)

"stop" itself isn't a great name either since one could say "stop after
pos 1023" so I guess that's why I've been naming these things "next_fsb"
and "next_pos"...

>  
>  	/* Nothing to do if we've written the entire delalloc extent */
> -	if (start_fsb >= end_fsb)
> +	if (start_byte >= end_byte)
>  		return 0;
>  
>  	/*
> @@ -1173,15 +1186,12 @@ xfs_buffered_write_iomap_end(
>  	 * leave dirty pages with no space reservation in the cache.
>  	 */
>  	filemap_invalidate_lock(inode->i_mapping);
> -	truncate_pagecache_range(VFS_I(ip), XFS_FSB_TO_B(mp, start_fsb),
> -				 XFS_FSB_TO_B(mp, end_fsb) - 1);
> -
> -	error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
> -				       end_fsb - start_fsb);
> +	truncate_pagecache_range(inode, start_byte, end_byte - 1);

...because the expression "end_byte - 1" looks a little funny when it's
used to compute the "lend" argument to truncate_pagecache_range.

> +	error = xfs_buffered_write_delalloc_punch(inode, start_byte, end_byte);
>  	filemap_invalidate_unlock(inode->i_mapping);
>  	if (error && !xfs_is_shutdown(mp)) {
> -		xfs_alert(mp, "%s: unable to clean up ino %lld",
> -			__func__, ip->i_ino);
> +		xfs_alert(mp, "%s: unable to clean up ino 0x%llx",
> +			__func__, XFS_I(inode)->i_ino);

Oh, you did fix the ino 0x%llx format thing.  Previous comment
withdrawn.

With s/end_byte/next_byte/ and the delalloc punch thing sorted out,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  		return error;
>  	}
>  	return 0;
> -- 
> 2.37.2
>