On Fri, Feb 23, 2024 at 08:14:58AM +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(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b525524a2da4ef..f8cc7c510d7bd5 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5321,16 +5321,6 @@ __xfs_bunmapi(
> } else
> cur = NULL;
>
> - if (isrt) {
> - /*
> - * Synchronize by locking the bitmap inode.
> - */
> - xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> - xfs_trans_ijoin(tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> - xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> - xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> - }
> -
> extno = 0;
> while (end != (xfs_fileoff_t)-1 && end >= start &&
> (nexts == 0 || extno < nexts)) {
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index e31663cb7b4349..2759c48390241d 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1001,6 +1001,20 @@ xfs_rtfree_blocks(
> return -EIO;
> }
>
> + /*
> + * Ensure the bitmap and summary inodes are locked before modifying
> + * them. We can get called multiples times per transaction, so record
> + * the fact that they are locked in the transaction.
> + */
> + if (!(tp->t_flags & XFS_TRANS_RTBITMAP_LOCKED)) {
> + tp->t_flags |= XFS_TRANS_RTBITMAP_LOCKED;
I don't really like using transaction flags to record lock state.
Would this be cleaner if we tracked this in struct xfs_rtalloc_args, and
had an xfs_rtalloc_acquire(mp, &args, XFS_ILOCK_{SHARED,EXCL}) method
that would set that up for us?
--D
> +
> + xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> + xfs_trans_ijoin(tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> + xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> + xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> + }
> +
> return xfs_rtfree_extent(tp, start, len);
> }
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 6f1cedb850eb39..1598ff00f6805f 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -83,6 +83,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
>
>
