| Message ID | 20240223071506.3968029-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/10] xfs: make XFS_TRANS_LOWMODE match the other XFS_TRANS_ definitions | expand |
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@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(-) > > 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 > >
On Fri, Feb 23, 2024 at 08:34:48AM -0800, Darrick J. Wong wrote: > 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? Urgg, that would be pretty ugly, and not work for the new locking in the extfree item once you add that, which has the same weird layering violation. The only "sane" way out would be to always use a deferred item, which we should be doing for anything using new RT features, but we can't really do that for legacy file systems without forcing a log incompat flag. So while I don't particularly like the transaction flag it seems like the least evil solution.
On Fri, Feb 23, 2024 at 05:37:37PM +0100, Christoph Hellwig wrote: > On Fri, Feb 23, 2024 at 08:34:48AM -0800, Darrick J. Wong wrote: > > 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? > > Urgg, that would be pretty ugly, and not work for the new locking in the > extfree item once you add that, which has the same weird layering > violation. <nod> Dave occasionally talks about turning allocation into a log intent so that we could decouple both of these things. I think the difficulty there is that you need a way for the allocator to notice that a particular "free" extent is actually "busy". That's trivial for the bnobt allocator, but nonexistent for rtalloc. > The only "sane" way out would be to always use a deferred item, which we > should be doing for anything using new RT features, but we can't really > do that for legacy file systems without forcing a log incompat flag. > So while I don't particularly like the transaction flag it seems like > the least evil solution. I had thought about doing that for rtgroups=1 filesystems. :) --D
On Fri, Feb 23, 2024 at 08:46:55AM -0800, Darrick J. Wong wrote: > > The only "sane" way out would be to always use a deferred item, which we > > should be doing for anything using new RT features, but we can't really > > do that for legacy file systems without forcing a log incompat flag. > > So while I don't particularly like the transaction flag it seems like > > the least evil solution. > > I had thought about doing that for rtgroups=1 filesystems. :) I actually have a patch doing that in my "not quite finished" queue. Given that your patch queue already enables it for rmap and reflink it's pretty trivial.
On Fri, Feb 23, 2024 at 05:49:16PM +0100, Christoph Hellwig wrote: > On Fri, Feb 23, 2024 at 08:46:55AM -0800, Darrick J. Wong wrote: > > > The only "sane" way out would be to always use a deferred item, which we > > > should be doing for anything using new RT features, but we can't really > > > do that for legacy file systems without forcing a log incompat flag. > > > So while I don't particularly like the transaction flag it seems like > > > the least evil solution. > > > > I had thought about doing that for rtgroups=1 filesystems. :) > > I actually have a patch doing that in my "not quite finished" queue. > Given that your patch queue already enables it for rmap and reflink > it's pretty trivial. <nod> In that case I'm fine with this: Reviewed-by: Darrick J. Wong <djwong@kernel.org> Some day we can deprecate !rtgroups filesystems and this will go away. Assuming one of us doesn't figure out a better way to do this. :P --D
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(-)