On Wed, Mar 27, 2024 at 12:03:09PM +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_bmap_del_extent_real just before the call to
> xfs_rtfree_blocks instead and use a new flag in the transaction to ensure
> that the locking happens only once.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 15 ++++++++-------
> fs/xfs/libxfs/xfs_shared.h | 3 +++
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 282b44deb9f864..e5e199d325982f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5305,6 +5305,14 @@ xfs_bmap_del_extent_real(
> if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
> xfs_refcount_decrease_extent(tp, del);
> } else if (xfs_ifork_is_realtime(ip, whichfork)) {
> + /*
> + * Ensure the bitmap and summary inodes are locked
> + * and joined to the transaction before modifying them.
> + */
> + if (!(tp->t_flags & XFS_TRANS_RTBITMAP_LOCKED)) {
> + tp->t_flags |= XFS_TRANS_RTBITMAP_LOCKED;
How does it happen that xfs_rtfree_blocks gets called more than once in
the same transaction? Is that simply the effect of xfs_bunmapi_range
and xfs_unmap_exten calling __xfs_bunmapi with
nextents == XFS_ITRUNC_MAX_EXTENTS==2?
What if we simply didn't unmap multiple extents per bunmapi call for
realtime files? Would that eliminate the need for
XFS_TRANS_RTBITMAP_LOCKED?
--D
> + xfs_rtbitmap_lock(tp, mp);
> + }
> error = xfs_rtfree_blocks(tp, del->br_startblock,
> del->br_blockcount);
> } else {
> @@ -5406,13 +5414,6 @@ __xfs_bunmapi(
> } else
> cur = NULL;
>
> - if (isrt) {
> - /*
> - * Synchronize by locking the realtime bitmap.
> - */
> - xfs_rtbitmap_lock(tp, mp);
> - }
> -
> extno = 0;
> while (end != (xfs_fileoff_t)-1 && end >= start &&
> (nexts == 0 || extno < nexts)) {
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index f35640ad3e7fe4..34f104ed372c09 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -137,6 +137,9 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
> */
> #define XFS_TRANS_LOWMODE (1u << 8)
>
> +/* Transaction has locked the rtbitmap and rtsum inodes */
> +#define XFS_TRANS_RTBITMAP_LOCKED (1u << 9)
> +
> /*
> * Field values for xfs_trans_mod_sb.
> */
> --
> 2.39.2
>
>
