Re: [PATCH RFC v3 12/21] xfs: Only free full extents for forcealign

[Dave Chinner]

On Wed, May 01, 2024 at 04:53:10PM -0700, Darrick J. Wong wrote:
> On Wed, May 01, 2024 at 10:53:28AM +1000, Dave Chinner wrote:
> > On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote:
> > > Like we already do for rtvol, only free full extents for forcealign in
> > > xfs_free_file_space().
> > > 
> > > Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> > > ---
> > >  fs/xfs/xfs_bmap_util.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > > index f26d1570b9bd..1dd45dfb2811 100644
> > > --- a/fs/xfs/xfs_bmap_util.c
> > > +++ b/fs/xfs/xfs_bmap_util.c
> > > @@ -847,8 +847,11 @@ xfs_free_file_space(
> > >  	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
> > >  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
> > >  
> > > -	/* We can only free complete realtime extents. */
> > > -	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
> > > +	/* Free only complete extents. */
> > > +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
> > > +		startoffset_fsb = roundup_64(startoffset_fsb, ip->i_extsize);
> > > +		endoffset_fsb = rounddown_64(endoffset_fsb, ip->i_extsize);
> > > +	} else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
> > >  		startoffset_fsb = xfs_rtb_roundup_rtx(mp, startoffset_fsb);
> > >  		endoffset_fsb = xfs_rtb_rounddown_rtx(mp, endoffset_fsb);
> > >  	}
> > 
> > When you look at xfs_rtb_roundup_rtx() you'll find it's just a one
> > line wrapper around roundup_64().
> 
> I added this a couple of cycles ago to get ready for realtime
> modernization.

Yes, I know. I'm not suggesting that there's anything wrong with
this code, just pointing out that the RT wrappers are doing the
exact same conversion as the force-align code is doing. And from
that observation, a common implementation makes a lot of sense
because that same logic is repeated in quite a few places....

> That will create a bunch *more* churn in my tree just to
> convert everything *back*.

This doesn't change anything significant in your tree, nor do you
need to "convert everything back". The RT wrappers are unchanged,
and the only material difference in your tree vs the upstream
xfs_free_file_space() this patchset is based on is this:

-	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
+	if (xfs_inode_has_bigrtalloc(ip)) {

That's it.

All the suggestion I made does is change where you need to make this
one line change. It would also remove the need to do this one line
change in multiple other places, so it would actually -reduce- your
ongoing rebase pain, not make it worse.

That's a net win for everyone, and it's most definitely not a reason
to shout at people and threaten to revert any changes they might
make in this area of the code.

> Where the hell were you when that was being reviewed?!!!

How is this sort of unhelpful statement in any way relevant to
improving the forcealign functionality to the point where we can
actually merge it and start making use of it for atomic writes?

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx