| Message ID | 20240223071506.3968029-7-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [01/10] xfs: make XFS_TRANS_LOWMODE match the other XFS_TRANS_ definitions | expand |
On Fri, Feb 23, 2024 at 08:15:02AM +0100, Christoph Hellwig wrote: > The code to account fdblocks and frextents in xfs_bmap_del_extent_delay > is a bit weird in that it accounts frextents before the iext tree > manipulations and fdblocks after it. Given that the iext tree > manipulations can fail currently that's not really a problem, but cannot? If my correction ^^^ is correct, then: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > still odd. Move the frextent manipulation to the end, and use a > fdblocks variable to account of the unconditional indirect blocks and > the data blocks only freed for !RT. This prepares for following > updates in the area and already makes the code more readable. > > Also remove the !isrt assert given that this code clearly handles > rt extents correctly, and we'll soon reinstate delalloc support for > RT inodes. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_bmap.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 95e93534cd1264..074d833e845af3 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -4833,6 +4833,7 @@ xfs_bmap_del_extent_delay( > xfs_fileoff_t del_endoff, got_endoff; > xfs_filblks_t got_indlen, new_indlen, stolen; > uint32_t state = xfs_bmap_fork_to_state(whichfork); > + uint64_t fdblocks; > int error = 0; > bool isrt; > > @@ -4848,15 +4849,11 @@ xfs_bmap_del_extent_delay( > ASSERT(got->br_startoff <= del->br_startoff); > ASSERT(got_endoff >= del_endoff); > > - if (isrt) > - xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount)); > - > /* > * Update the inode delalloc counter now and wait to update the > * sb counters as we might have to borrow some blocks for the > * indirect block accounting. > */ > - ASSERT(!isrt); > error = xfs_quota_unreserve_blkres(ip, del->br_blockcount); > if (error) > return error; > @@ -4933,12 +4930,15 @@ xfs_bmap_del_extent_delay( > > ASSERT(da_old >= da_new); > da_diff = da_old - da_new; > - if (!isrt) > - da_diff += del->br_blockcount; > - if (da_diff) { > - xfs_add_fdblocks(mp, da_diff); > - xfs_mod_delalloc(mp, -da_diff); > - } > + fdblocks = da_diff; > + > + if (isrt) > + xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount)); > + else > + fdblocks += del->br_blockcount; > + > + xfs_add_fdblocks(mp, fdblocks); > + xfs_mod_delalloc(mp, -(int64_t)fdblocks); > return error; > } > > -- > 2.39.2 > >
On Fri, Feb 23, 2024 at 09:13:24AM -0800, Darrick J. Wong wrote: > On Fri, Feb 23, 2024 at 08:15:02AM +0100, Christoph Hellwig wrote: > > The code to account fdblocks and frextents in xfs_bmap_del_extent_delay > > is a bit weird in that it accounts frextents before the iext tree > > manipulations and fdblocks after it. Given that the iext tree > > manipulations can fail currently that's not really a problem, but > > cannot? > > If my correction ^^^ is correct, then: Yes.
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 95e93534cd1264..074d833e845af3 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -4833,6 +4833,7 @@ xfs_bmap_del_extent_delay( xfs_fileoff_t del_endoff, got_endoff; xfs_filblks_t got_indlen, new_indlen, stolen; uint32_t state = xfs_bmap_fork_to_state(whichfork); + uint64_t fdblocks; int error = 0; bool isrt; @@ -4848,15 +4849,11 @@ xfs_bmap_del_extent_delay( ASSERT(got->br_startoff <= del->br_startoff); ASSERT(got_endoff >= del_endoff); - if (isrt) - xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount)); - /* * Update the inode delalloc counter now and wait to update the * sb counters as we might have to borrow some blocks for the * indirect block accounting. */ - ASSERT(!isrt); error = xfs_quota_unreserve_blkres(ip, del->br_blockcount); if (error) return error; @@ -4933,12 +4930,15 @@ xfs_bmap_del_extent_delay( ASSERT(da_old >= da_new); da_diff = da_old - da_new; - if (!isrt) - da_diff += del->br_blockcount; - if (da_diff) { - xfs_add_fdblocks(mp, da_diff); - xfs_mod_delalloc(mp, -da_diff); - } + fdblocks = da_diff; + + if (isrt) + xfs_add_frextents(mp, xfs_rtb_to_rtx(mp, del->br_blockcount)); + else + fdblocks += del->br_blockcount; + + xfs_add_fdblocks(mp, fdblocks); + xfs_mod_delalloc(mp, -(int64_t)fdblocks); return error; }
The code to account fdblocks and frextents in xfs_bmap_del_extent_delay is a bit weird in that it accounts frextents before the iext tree manipulations and fdblocks after it. Given that the iext tree manipulations can fail currently that's not really a problem, but still odd. Move the frextent manipulation to the end, and use a fdblocks variable to account of the unconditional indirect blocks and the data blocks only freed for !RT. This prepares for following updates in the area and already makes the code more readable. Also remove the !isrt assert given that this code clearly handles rt extents correctly, and we'll soon reinstate delalloc support for RT inodes. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_bmap.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)