| Message ID | 20240430124926.1775355-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [01/16] xfs: allow non-empty forks in xfs_bmap_local_to_extents_empty | expand |
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 > >
On Tue, Apr 30, 2024 at 08:51:31AM -0700, Darrick J. Wong wrote: > > 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? Even before this patch, xfs_bmap_local_to_extents_empty never frees any memory, it just asserts the the local fork size has been changed to 0, which implies that the caller already freed the memory. With this patch the caller can free the memory after calling xfs_bmap_local_to_extents_empty instead of before it, which the callers (all but one anyway) will make use of in the following patches. I thought the commit log made that clear, but if you think it needs to improved feel free to suggest edits.
On Wed, May 01, 2024 at 06:37:34AM +0200, Christoph Hellwig wrote: > On Tue, Apr 30, 2024 at 08:51:31AM -0700, Darrick J. Wong wrote: > > > 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? > > Even before this patch, xfs_bmap_local_to_extents_empty never frees any > memory, it just asserts the the local fork size has been changed to 0, > which implies that the caller already freed the memory. With this > patch the caller can free the memory after calling > xfs_bmap_local_to_extents_empty instead of before it, which the callers > (all but one anyway) will make use of in the following patches. > > I thought the commit log made that clear, but if you think it needs to > improved feel free to suggest edits. Ah, I see, currently all the callers /do/ free ifp->if_data, having snapshotted the contents before doing so. I guess I needed the words "all callers do that". How about: "Currently, xfs_bmap_local_to_extents_empty expects the caller to set the fork size to 0 and free if_data. All callers do that, but they also allocate a temporary shadow buffer because they need the contents of the fork so they can copy it to the newly allocated extent after the transition to extents format. This is highly suboptimal." ? With some wordsmithing like that, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
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,
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 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(-)