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@xxxxxx>
> ---
> 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
>
>
