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

John Garry
2024-07-08T14:48:42Z

On 06/07/2024 08:58, Christoph Hellwig wrote:
>> +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?

Right, it is not really. Apart from the ASSERT to ensure that we are not
calling from a stray context.

>
>> @@ -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..

I can change both to a bool - would that be better?

Using isfa (instead of isforcealign) might be interpreted as something
else :)

>>
>> + 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.

noted

>
> 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.

ok, I'll see if can come up with something

>
>> + 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.

sure

>