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

[John Garry <john.g.garry@oracle.com>]

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

>