| Message ID | 20240219063450.3032254-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/9] xfs: make XFS_TRANS_LOWMODE match the other XFS_TRANS_ definitions | expand |
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@lst.de> > --- > 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.
On Tue, Feb 20, 2024 at 10:55:59AM +1100, Dave Chinner wrote: > 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: Yes, this code has been bothering me forever, and I've started at least three attempts at cleaning it up and for now given them up due to the really compliated exit conditions. I think it really should be done eventually, but I don't plan to add it to this series. It will probably end up changing some of the existing loop logic as some of that is pretty questionable and predates the nice iext iterators.
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; + + 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. */
__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@lst.de> --- 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(-)