Re: [PATCH v2 08/13] xfs: Do not free EOF blocks for forcealign

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

On 08/07/2024 12:12, Dave Chinner wrote:
> On Mon, Jul 08, 2024 at 08:36:52AM +0100, John Garry wrote:
>> On 08/07/2024 02:44, Dave Chinner wrote:
>>> On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote:
>>>> On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote:
>>>>> -	if (xfs_inode_has_bigrtalloc(ip))
>>>>> +
>>>>> +	/* Only try to free beyond the allocation unit that crosses EOF */
>>>>> +	if (xfs_inode_has_forcealign(ip))
>>>>> +		end_fsb = roundup_64(end_fsb, ip->i_extsize);
>>>>> +	else if (xfs_inode_has_bigrtalloc(ip))
>>>>>    		end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb);
>>>>
>>>> Shouldn't we have a common helper to align things the right way?
>>>
>>> Yes, that's what I keep saying.
>>
>> Such a change was introduced in
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/20240501235310.GP360919@frogsfrogsfrogs/__;!!ACWV5N9M2RV99hQ!LU97IJHrwX9otpItjJI_ewbNt-T-Lgyt5ulyz0yGKc5Dmms4jqhwZv5NregBEK_dTEtEDCYCwcA43RxQFnc$
>>
>> and, as you can see, Darrick was less than happy with it. That is why I kept
>> this method which removed recently added RT code.
> 
> I know.
> 
> However, "This is pointless busywork!" is not a technical argument
> against the observation that rtbigalloc and forcealign are exactly
> the same thing from the BMBT management POV and so should be
> combined.
> 
> Arguing that "but doing the right thing makes more work for me"
> doesn't hold any weight. It never has. Shouting and ranting
> irrationally is a great way to shut down any further conversation,
> though.
> 
> Our normal process is to factor the code and add the extra condition
> for the new feature. That's all I'm asking to be done. It's not
> technically difficult. It makes the code better. it makes the code
> easier to test, too, because there are now two entries in the test
> matrix taht exercise that code path. It's simpler to understand
> months down the track, makes new alignment features easier to add in
> future, etc.
> 
> Put simply: if we just do what we have always done, then we end up
> with better code.  Hence I just don't see why people are trying to
> make a mountain out of this...

I tend to agree with what you said; and the conversation was halted, so 
left me in an awkward position.

> 
>> Darrick, can we find a better method to factor this code out, like below?
>>
>>> The common way to do this is:
>>>
>>> 	align = xfs_inode_alloc_unitsize(ip);
>>> 	if (align > mp->m_blocksize)
>>> 		end_fsb = roundup_64(end_fsb, align);
>>>
>>> Wrapping that into a helper might be appropriate, though we'd need
>>> wrappers for aligning both the start (down) and end (up).
>>>
>>> To make this work, the xfs_inode_alloc_unitsize() code needs to grow
>>> a forcealign check. That overrides the RT rextsize value (force
>>> align on RT should work the same as it does on data devs) and needs
>>> to look like this:
>>>
>>> 	unsigned int		blocks = 1;
>>>
>>> +	if (xfs_inode_has_forcealign(ip)
>>> +		blocks = ip->i_extsize;
>>> -	if (XFS_IS_REALTIME_INODE(ip))
>>> +	else if (XFS_IS_REALTIME_INODE(ip))
>>>                   blocks = ip->i_mount->m_sb.sb_rextsize;
>>
>> That's in 09/13
> 
> Thanks, I thought it was somewhere in this patch series, I just
> wanted to point out (once again) that rtbigalloc and forcealign are
> basically the same thing.
> 
> And, in case it isn't obvious to everyone, setting forcealign on a
> rt inode is basically the equivalent of turning on "rtbigalloc" for
> just that inode....

sure

> 
>>>           return XFS_FSB_TO_B(ip->i_mount, blocks);
>>>
>>>> But more importantly shouldn't this also cover hole punching if we
>>>> really want force aligned boundaries?
>>
>> so doesn't the xfs_file_fallocate(PUNCH_HOLES) -> xfs_flush_unmap_range() ->
>> rounding with xfs_inode_alloc_unitsize() do the required job?
> 
> No, xfs_flush_unmap_range() should be flushing to *outwards*
> block/page size boundaries because it is cleaning and invalidating
> the page cache over the punch range, not manipulating the physical
> extents underlying the data.
> 
> It's only once we go to punch out the extents in
> xfs_free_file_space() that we need to use xfs_inode_alloc_unitsize()
> to determine the *inwards* rounding for the extent punch vs writing
> physical zeroes....
> 

ok, well we are covered for forcealign in both xfs_flush_unmap_range() 
and xfs_free_file_space().