On Mon, Feb 19, 2024 at 07:34:43AM +0100, Christoph Hellwig wrote:
> __xfs_bunmapi is a bit of an odd place to lock the rtbitmap and rtsummary
> inodes given that it is very high level code. While this only looks ugly
> right now, it will become a problem when supporting delayed allocations
> for RT inodes as __xfs_bunmapi might end up deleting only delalloc extents
> and thus never unlock the rt inodes.
>
> Move the locking into xfs_rtfree_blocks instead (where it will also be
> helpful once we support extfree items for RT allocations), and use a new
> flag in the transaction to ensure they aren't locked twice.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 10 ----------
> fs/xfs/libxfs/xfs_rtbitmap.c | 14 ++++++++++++++
> fs/xfs/libxfs/xfs_shared.h | 3 +++
> 3 files changed, 17 insertions(+), 10 deletions(-)
Ok, nice cleanup.
I'd also like to see the rest of the rt-only code in __xfs_bunmapi()
lifted out of the function into a helper. It's a big chunk of code
inside the loop, and the code structure is:
loop() {
/* common stuff */
if (!rt)
goto delete;
/* lots of rt only stuff */
delete:
/* common stuff */
}
I think this would be much better as
loop() {
/* common stuff */
if (rt) {
error = xfs_bunmapi_rtext()
if (error)
goto error0;
}
/* common stuff */
}
Separate cleanup though, not necessary for this patchset...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
