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@xxxxxxxxxx> --D
