| Message ID | 20240327110318.2776850-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/13] xfs: make XFS_TRANS_LOWMODE match the other XFS_TRANS_ definitions | expand |
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@lst.de> > --- > 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 > >
On Wed, Mar 27, 2024 at 08:07:55AM -0700, Darrick J. Wong wrote: > 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? Yes. > 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? Probably. Not that I really want to rock that boat now when we'll also have the extent free item / defer ops based path soon.
On Wed, Mar 27, 2024 at 06:06:32PM +0100, Christoph Hellwig wrote: > On Wed, Mar 27, 2024 at 08:07:55AM -0700, Darrick J. Wong wrote: > > 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? > > Yes. > > > 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? > > Probably. Not that I really want to rock that boat now when we'll > also have the extent free item / defer ops based path soon. <nod> I think once we get to the rtgroups patchset then all we have to do in __xfs_bunmapi is: if (XFS_IS_REALTIME_INODE(ip) && whichfork == XFS_DATA_FORK && !xfs_has_rtgroups(mp)) nexts = 1; and then the XFS_TRANS_RTBITMAP_LOCKED flag can go away? If so, then Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D
On Wed, Mar 27, 2024 at 11:06:00AM -0700, Darrick J. Wong wrote: > <nod> I think once we get to the rtgroups patchset then all we have to > do in __xfs_bunmapi is: > > if (XFS_IS_REALTIME_INODE(ip) && whichfork == XFS_DATA_FORK && > !xfs_has_rtgroups(mp)) > nexts = 1; > > and then the XFS_TRANS_RTBITMAP_LOCKED flag can go away? Probably. We could also try to do it now, but that's just yet another variable changing..
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@lst.de> Not a great fan of using transaction flags for object state, but I don't have a better way of doing this. Reviewed-by: Dave Chinner <dchinner@redhat.com>
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; + 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. */
__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@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 15 ++++++++------- fs/xfs/libxfs/xfs_shared.h | 3 +++ 2 files changed, 11 insertions(+), 7 deletions(-)