Re: [PATCH RFC v3 11/21] xfs: Unmap blocks according to forcealign

[John Garry]

On 01/05/2024 01:10, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:36PM +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.
> >
> > Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_bmap.c | 39 +++++++++++++++++++++++++++++++++------
> >   fs/xfs/xfs_inode.h       |  5 +++++
> >   2 files changed, 38 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 4f39a43d78a7..4a78ab193753 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -5339,6 +5339,15 @@ xfs_bmap_del_extent_real(
> >   	return 0;
> >   }
> >   
> > +/* Return the offset of an block number within an extent for forcealign. */
> > +static xfs_extlen_t
> > +xfs_forcealign_extent_offset(
> > +	struct xfs_inode	*ip,
> > +	xfs_fsblock_t		bno)
> > +{
> > +	return bno & (ip->i_extsize - 1);
> > +}
> > +
> >   /*
> >    * Unmap (remove) blocks from a file.
> >    * If nexts is nonzero then the number of extents to remove is limited to
> > @@ -5361,6 +5370,7 @@ __xfs_bunmapi(
> >   	struct xfs_bmbt_irec	got;		/* current extent record */
> >   	struct xfs_ifork	*ifp;		/* inode fork pointer */
> >   	int			isrt;		/* freeing in rt area */
> > +	int			isforcealign;	/* freeing for file inode with forcealign */
> >   	int			logflags;	/* transaction logging flags */
> >   	xfs_extlen_t		mod;		/* rt extent offset */
> >   	struct xfs_mount	*mp = ip->i_mount;
> > @@ -5397,7 +5407,10 @@ __xfs_bunmapi(
> >   		return 0;
> >   	}
> >   	XFS_STATS_INC(mp, xs_blk_unmap);
> > -	isrt = xfs_ifork_is_realtime(ip, whichfork);
> > +	isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
>
> Why did you change this check? What's wrong with
> xfs_ifork_is_realtime(), and if there is something wrong, why
> shouldn't xfs_ifork_is_relatime() get fixed?

oops, I should have not made that change. I must have changed it when 
debugging and not reverted it.

> > +	isforcealign = (whichfork == XFS_DATA_FORK) &&
> > +			xfs_inode_has_forcealign(ip) &&
> > +			xfs_inode_has_extsize(ip) && ip->i_extsize > 1;
>
> This is one of the reasons why I said xfs_inode_has_forcealign()
> should be checking that extent size hints should be checked in that
> helper....

Right. In this particular case, I found that directories may be 
considered as well if we don't check for xfs_inode_has_extsize() (which 
we don't want).

> >   	end = start + len;
> >   
> >   	if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
> > @@ -5459,11 +5472,15 @@ __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);
> > +		if (isrt)
> > +			mod = xfs_rtb_to_rtxoff(mp,
> > +					del.br_startblock + del.br_blockcount);
> > +		else if (isforcealign)
> > +			mod = xfs_forcealign_extent_offset(ip,
> > +					del.br_startblock + del.br_blockcount);
>
> There's got to be a cleaner way to do this.
>
> We already know that either isrt or isforcealign must be set here,
> so there's no need for the "else if" construct.

right

> Also, forcealign should take precedence over realtime, so that
> forcealign will work on realtime devices as well. I'd change this
> code to call a wrapper like:
>
> 		mod = xfs_bunmapi_align(ip, del.br_startblock + del.br_blockcount);
>
> static xfs_extlen_t
> xfs_bunmapi_align(
> 	struct xfs_inode	*ip,
> 	xfs_fsblock_t		bno)
> {
> 	if (!XFS_INODE_IS_REALTIME(ip)) {
> 		ASSERT(xfs_inode_has_forcealign(ip))
> 		if (is_power_of_2(ip->i_extsize))
> 			return bno & (ip->i_extsize - 1);
> 		return do_div(bno, ip->i_extsize);
> 	}
> 	return xfs_rtb_to_rtxoff(ip->i_mount, bno);
> }

ok, that's neater

Thanks,
John