Re: [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Oct 25, 2019 at 05:03:29PM +0200, Christoph Hellwig wrote:
> By open coding xfs_bmap_last_extent instead of calling it through a
> double indirection we don't need to handle an error return that
> can't happen given that we are guaranteed to have the extent list in
> memory already.  Also simplify the calling conventions a little and
> move the extent list assert from the only caller into the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 23 ---------------------
>  fs/xfs/xfs_bmap_util.h |  2 --
>  fs/xfs/xfs_iomap.c     | 47 ++++++++++++++++++++----------------------
>  3 files changed, 22 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 11658da40640..44d6b6469303 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -179,29 +179,6 @@ xfs_bmap_rtalloc(
>  }
>  #endif /* CONFIG_XFS_RT */
>  
> -/*
> - * Check if the endoff is outside the last extent. If so the caller will grow
> - * the allocation to a stripe unit boundary.  All offsets are considered outside
> - * the end of file for an empty fork, so 1 is returned in *eof in that case.
> - */
> -int
> -xfs_bmap_eof(
> -	struct xfs_inode	*ip,
> -	xfs_fileoff_t		endoff,
> -	int			whichfork,
> -	int			*eof)
> -{
> -	struct xfs_bmbt_irec	rec;
> -	int			error;
> -
> -	error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, eof);
> -	if (error || *eof)
> -		return error;
> -
> -	*eof = endoff >= rec.br_startoff + rec.br_blockcount;
> -	return 0;
> -}
> -
>  /*
>   * Extent tree block counting routines.
>   */
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 3e0fa0d363d1..9f993168b55b 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -30,8 +30,6 @@ xfs_bmap_rtalloc(struct xfs_bmalloca *ap)
>  }
>  #endif /* CONFIG_XFS_RT */
>  
> -int	xfs_bmap_eof(struct xfs_inode *ip, xfs_fileoff_t endoff,
> -		     int whichfork, int *eof);
>  int	xfs_bmap_punch_delalloc_range(struct xfs_inode *ip,
>  		xfs_fileoff_t start_fsb, xfs_fileoff_t length);
>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c1063507e5fd..49fbc99c18ff 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -156,25 +156,33 @@ xfs_eof_alignment(
>  	return align;
>  }
>  
> -STATIC int
> +/*
> + * Check if last_fsb is outside the last extent, and if so grow it to the next
> + * stripe unit boundary.
> + */
> +static xfs_fileoff_t
>  xfs_iomap_eof_align_last_fsb(
>  	struct xfs_inode	*ip,
> -	xfs_extlen_t		extsize,
> -	xfs_fileoff_t		*last_fsb)
> +	xfs_fileoff_t		end_fsb)
>  {
> -	xfs_extlen_t		align = xfs_eof_alignment(ip, extsize);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	xfs_extlen_t		extsz = xfs_get_extsz_hint(ip);
> +	xfs_extlen_t		align = xfs_eof_alignment(ip, extsz);
> +	struct xfs_bmbt_irec	irec;
> +	struct xfs_iext_cursor	icur;
> +
> +	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
>  
>  	if (align) {
> -		xfs_fileoff_t	new_last_fsb = roundup_64(*last_fsb, align);
> -		int		eof, error;
> +		xfs_fileoff_t	aligned_end_fsb = roundup_64(end_fsb, align);
>  
> -		error = xfs_bmap_eof(ip, new_last_fsb, XFS_DATA_FORK, &eof);
> -		if (error)
> -			return error;
> -		if (eof)
> -			*last_fsb = new_last_fsb;
> +		xfs_iext_last(ifp, &icur);
> +		if (!xfs_iext_get_extent(ifp, &icur, &irec) ||
> +		    aligned_end_fsb >= irec.br_startoff + irec.br_blockcount)
> +			return aligned_end_fsb;
>  	}
> -	return 0;
> +
> +	return end_fsb;
>  }
>  
>  int
> @@ -206,19 +214,8 @@ xfs_iomap_write_direct(
>  
>  	ASSERT(xfs_isilocked(ip, lockmode));
>  
> -	if ((offset + count) > XFS_ISIZE(ip)) {
> -		/*
> -		 * Assert that the in-core extent list is present since this can
> -		 * call xfs_iread_extents() and we only have the ilock shared.
> -		 * This should be safe because the lock was held around a bmapi
> -		 * call in the caller and we only need it to access the in-core
> -		 * list.
> -		 */
> -		ASSERT(XFS_IFORK_PTR(ip, XFS_DATA_FORK)->if_flags &
> -								XFS_IFEXTENTS);
> -		error = xfs_iomap_eof_align_last_fsb(ip, extsz, &last_fsb);
> -		if (error)
> -			goto out_unlock;
> +	if (offset + count > XFS_ISIZE(ip)) {
> +		last_fsb = xfs_iomap_eof_align_last_fsb(ip, last_fsb);
>  	} else {
>  		if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
>  			last_fsb = min(last_fsb, (xfs_fileoff_t)
> -- 
> 2.20.1
>