Message-ID: <20240807161944.GJ6051@frogsfrogsfrogs>
Date: Wed, 7 Aug 2024 09:19:44 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: John Garry <john.g.garry@...cle.com>
Cc: chandan.babu@...cle.com, dchinner@...hat.com, hch@....de,
	viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, catherine.hoang@...cle.com,
	martin.petersen@...cle.com
Subject: Re: [PATCH v3 12/14] xfs: Unmap blocks according to forcealign

On Wed, Aug 07, 2024 at 02:40:36PM +0100, John Garry wrote:
> On 06/08/2024 21:14, Darrick J. Wong wrote:
> > On Thu, Aug 01, 2024 at 04:30:55PM +0000, John Garry wrote:
> > > For when forcealign is enabled, blocks in an inode need to be unmapped
> > > according to extent alignment, like what is already done for rtvol.
> > > 
> > > Change variable isrt in __xfs_bunmapi() to a bool, as that is really what
> > > it is.
> > > 
> > > Signed-off-by: John Garry <john.g.garry@...cle.com>
> > > ---
> > >   fs/xfs/libxfs/xfs_bmap.c | 48 +++++++++++++++++++++++++++++-----------
> > >   fs/xfs/xfs_inode.c       | 16 ++++++++++++++
> > >   fs/xfs/xfs_inode.h       |  2 ++
> > >   3 files changed, 53 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 0c3df8c71c6d..d6ae344a17fc 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -5409,6 +5409,25 @@ xfs_bmap_del_extent_real(
> > >   	return 0;
> > >   }
> > > +static xfs_extlen_t
> > > +xfs_bunmapi_align(
> > > +	struct xfs_inode	*ip,
> > > +	xfs_fsblock_t		fsbno,
> > > +	xfs_extlen_t *off)
> > > +{
> > > +	struct xfs_mount	*mp = ip->i_mount;
> > > +
> > > +	if (XFS_IS_REALTIME_INODE(ip))
> > > +		return xfs_inode_alloc_fsbsize_align(ip, fsbno, off);
> > > +	/*
> > > +	 * The agbno for the fsbno is aligned to extsize, but the fsbno itself
> > > +	 * is not necessarily aligned (to extsize), so use agbno to determine
> > > +	 * mod+offset to the alloc unit boundary.
> > > +	 */
> > > +	return xfs_inode_alloc_fsbsize_align(ip, XFS_FSB_TO_AGBNO(mp, fsbno),
> > > +					off);
> > > +}
> > > +
> > >   /*
> > >    * Unmap (remove) blocks from a file.
> > >    * If nexts is nonzero then the number of extents to remove is limited to
> > > @@ -5430,7 +5449,8 @@ __xfs_bunmapi(
> > >   	xfs_extnum_t		extno;		/* extent number in list */
> > >   	struct xfs_bmbt_irec	got;		/* current extent record */
> > >   	struct xfs_ifork	*ifp;		/* inode fork pointer */
> > > -	int			isrt;		/* freeing in rt area */
> > > +	bool			isrt;		/* freeing in rt area */
> > > +	bool			isforcealign;	/* forcealign inode */
> > >   	int			logflags;	/* transaction logging flags */
> > >   	xfs_extlen_t		mod;		/* rt extent offset */
> > >   	struct xfs_mount	*mp = ip->i_mount;
> > > @@ -5468,6 +5488,8 @@ __xfs_bunmapi(
> > >   	}
> > >   	XFS_STATS_INC(mp, xs_blk_unmap);
> > >   	isrt = xfs_ifork_is_realtime(ip, whichfork);
> > > +	isforcealign = (whichfork != XFS_ATTR_FORK) &&
> > > +			xfs_inode_has_forcealign(ip);
> > >   	end = start + len;
> > >   	if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
> > > @@ -5486,6 +5508,8 @@ __xfs_bunmapi(
> > >   	extno = 0;
> > >   	while (end != (xfs_fileoff_t)-1 && end >= start &&
> > >   	       (nexts == 0 || extno < nexts)) {
> > > +		xfs_extlen_t off;
> > 
> > I got really confused because I thought this was a file block offset and
> > only after more digging realized that this is a sometimes dummy
> > adjustment variable.
> 
> yeah, I put it here to use the common helper in both callsites
> 
> > 
> > > +
> > >   		/*
> > >   		 * Is the found extent after a hole in which end lives?
> > >   		 * Just back up to the previous extent, if so.
> > > @@ -5519,18 +5543,18 @@ __xfs_bunmapi(
> > >   		if (del.br_startoff + del.br_blockcount > end + 1)
> > >   			del.br_blockcount = end + 1 - del.br_startoff;
> > > -		if (!isrt || (flags & XFS_BMAPI_REMAP))
> > > +		if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
> > >   			goto delete;
> > > -		mod = xfs_rtb_to_rtxoff(mp,
> > > -				del.br_startblock + del.br_blockcount);
> > > +		mod = xfs_bunmapi_align(ip,
> > > +				del.br_startblock + del.br_blockcount, &off);
> > >   		if (mod) {
> > 
> > Oof.  I don't like how this loop body has the rtx adjustment code
> > inlined into it.  We only use the isrt flag for the one test above.
> > I tried hoisting this into something less gross involving separate
> > adjustment functions but then you have to pass in so many outer
> > variables that it becomes a mess.
> > 
> > The best I can come up with for now is:
> > 
> > 	unsigned int		alloc_fsb = xfs_inode_alloc_fsbsize(ip);
> > 	/* no more isrt/isforcealign bools */
> > 
> > ...
> > 
> > 		if (alloc_fsb == 1 || (flags & XFS_BMAPI_REMAP))
> > 			goto delete;
> 
> ok, good
> 
> > 
> > 		mod = do_div(del.br_startblock + del.br_blockcount,
> > 				alloc_fsb);
> 
> Note that xfs_bunmapi_align() uses agbno for !rt

Right, I forgot about that.  Can we make the name say that we're
computing the offset within an allocunit?  It's not actually aligning
any of its parameters.

static inline unsigned int
xfs_bmap_alloc_unit_offset(
	struct xfs_inode	*ip,
	xfs_fsblock_t		fsbno,
	unsigned int		alloc_fsb)
{
	xfs_agblock_t		agbno;

	if (XFS_IS_REALTIME_INODE(ip))
		return do_div(fsbno, alloc_fsb);

	agbno = XFS_FSB_TO_AGBNO(ip->i_mount, fsbno);
	return agbno % alloc_fsb;
}

		mod = xfs_bmap_alloc_unit_offset(ip,
				del.br_startblock + del.br_blockcount,
				alloc_fsb);

and

		mod = xfs_bmap_alloc_unit_offset(ip, del.br_startblock,
				alloc_fsb);

--D

> 
> > 		if (mod) {
> > 
> > >   			/*
> > > -			 * Realtime extent not lined up at the end.
> > > +			 * Not aligned to allocation unit on the end.
> > >   			 * The extent could have been split into written
> > >   			 * and unwritten pieces, or we could just be
> > >   			 * unmapping part of it.  But we can't really
> > > -			 * get rid of part of a realtime extent.
> > > +			 * get rid of part of an extent.
> > >   			 */
> > >   			if (del.br_state == XFS_EXT_UNWRITTEN) {
> > >   				/*
> > > @@ -5554,7 +5578,7 @@ __xfs_bunmapi(
> > >   			ASSERT(del.br_state == XFS_EXT_NORM);
> > >   			ASSERT(tp->t_blk_res > 0);
> > >   			/*
> > > -			 * If this spans a realtime extent boundary,
> > > +			 * If this spans an extent boundary,
> > >   			 * chop it back to the start of the one we end at.
> > >   			 */
> > >   			if (del.br_blockcount > mod) {
> > > @@ -5571,14 +5595,12 @@ __xfs_bunmapi(
> > >   			goto nodelete;
> > >   		}
> > > -		mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
> > > +		mod = xfs_bunmapi_align(ip, del.br_startblock, &off);
> > >   		if (mod) {
> > > -			xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
> > 
> > 		mod = do_div(del.br_startblock, alloc_fsb);
> > 		if (mod) {
> > 			xfs_extlen_t off = alloc_fsb - mod;
> > 
> > At least then you don't need this weird xfs_inode_alloc_fsbsize_align
> > that passes back two xfs_extlen_t arguments.
> 
> Sure, but same point about xfs_bunmapi_align() using agbno. I suppose I can
> just make the change to not have xfs_bunmapi_align() be passed the off
> address pointer, and do the calcation here for off here (as you suggest).
> 
> Thanks,
> John
> 

Powered by blists - more mailing lists