Re: [PATCH v2 10/13] xfs: Unmap blocks according to forcealign

Christoph Hellwig
2024-07-06T07:59:02Z

> +static xfs_extlen_t
> +xfs_bunmapi_align(
> + struct xfs_inode *ip,
> + xfs_fsblock_t bno)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + xfs_agblock_t agbno;
> +
> + if (xfs_inode_has_forcealign(ip)) {
> + if (is_power_of_2(ip->i_extsize))
> + return bno & (ip->i_extsize - 1);
> +
> + agbno = XFS_FSB_TO_AGBNO(mp, bno);
> + return agbno % ip->i_extsize;
> + }
> + ASSERT(XFS_IS_REALTIME_INODE(ip));
> + return xfs_rtb_to_rtxoff(ip->i_mount, bno);

This helper isn't really bunmapi sepcific, is it?

> @@ -5425,6 +5444,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 inode with forcealign */

This is really a bool. And while it matches the code around it the
code feels a bit too verbose..
>
> + 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);

Overly long line.

We've been long wanting to split the whole align / convert unwritten /
etc code into a helper outside the main bumapi flow. And when adding
new logic to it this might indeed be a good time.

> + if (isforcealign) {
> + off = ip->i_extsize - mod;
> + } else {
> + ASSERT(isrt);
> + off = mp->m_sb.sb_rextsize - mod;
> + }

And we'll really need proper helpers so that we don't have to
open code the i_extsize vs sb_rextsize logic all over.