Re: [PATCH 01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty

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

On Tue, Apr 30, 2024 at 02:49:11PM +0200, Christoph Hellwig wrote:
> Currently xfs_bmap_local_to_extents_empty expects the caller to set the
> for size to 0, which implies freeing if_data.  That is highly suboptimal
> because the callers need the data in the local fork so that they can
> copy it to the newly allocated extent.
> 
> Change xfs_bmap_local_to_extents_empty to return the old fork data and
> clear if_bytes to zero instead and let the callers free the memory for

But I don't see any changes in the callsites to do that freeing, is this
a memory leak?

--D

> the local fork, which allows them to access the data directly while
> formatting the extent format data.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 13 +++++++------
>  fs/xfs/libxfs/xfs_bmap.h |  2 +-
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6053f5e5c71eec..eb826fae8fd878 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -754,31 +754,32 @@ xfs_bmap_extents_to_btree(
>  
>  /*
>   * Convert a local file to an extents file.
> - * This code is out of bounds for data forks of regular files,
> - * since the file data needs to get logged so things will stay consistent.
> - * (The bmap-level manipulations are ok, though).
> + *
> + * Returns the content of the old data fork, which needs to be freed by the
> + * caller.
>   */
> -void
> +void *
>  xfs_bmap_local_to_extents_empty(
>  	struct xfs_trans	*tp,
>  	struct xfs_inode	*ip,
>  	int			whichfork)
>  {
>  	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
> +	void			*old_data = ifp->if_data;
>  
>  	ASSERT(whichfork != XFS_COW_FORK);
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(ifp->if_bytes == 0);
>  	ASSERT(ifp->if_nextents == 0);
>  
>  	xfs_bmap_forkoff_reset(ip, whichfork);
>  	ifp->if_data = NULL;
> +	ifp->if_bytes = 0;
>  	ifp->if_height = 0;
>  	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	return old_data;
>  }
>  
> -
>  int					/* error */
>  xfs_bmap_local_to_extents(
>  	xfs_trans_t	*tp,		/* transaction pointer */
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index e98849eb9bbae3..6e0b6081bf3aa5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -178,7 +178,7 @@ void	xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
>  unsigned int xfs_bmap_compute_attr_offset(struct xfs_mount *mp);
>  int	xfs_bmap_add_attrfork(struct xfs_trans *tp, struct xfs_inode *ip,
>  		int size, int rsvd);
> -void	xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
> +void	*xfs_bmap_local_to_extents_empty(struct xfs_trans *tp,
>  		struct xfs_inode *ip, int whichfork);
>  int xfs_bmap_local_to_extents(struct xfs_trans *tp, struct xfs_inode *ip,
>  		xfs_extlen_t total, int *logflagsp, int whichfork,
> -- 
> 2.39.2
> 
>