Re: [PATCH v5 09/10] xfs: Allow block allocator to take an alignment hint

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

On 12/03/2025 16:00, Darrick J. Wong wrote:
> On Wed, Mar 12, 2025 at 06:45:12AM -0700, Christoph Hellwig wrote:
>> On Wed, Mar 12, 2025 at 08:05:14AM +0000, John Garry wrote:
>>>> Shouldn't we be doing this by default for any extent size hint
>>>> based allocations?
>>>
>>> I'm not sure.
>>>
>>> I think that currently users just expect extszhint to hint at the
>>> granularity only.
> 
> Yes, the current behavior is that extszhint only affects the granularity
> of the file range that's passed into the allocator.  To align the actual
> space, you have to set the raid stripe parameters.
> 
> I can see how that sorta made sense in the old days -- the fs could get
> moved between raid arrays (or the raid array gets reconfigured), so you
> want the actual allocations to be aligned to whatever the current
> hardware config advertises.  The extent size hint is merely a means to
> amortize the cost of allocation/second-guess the delalloc machinery.
> 
>>> Maybe users don't require alignment and adding an alignment requirement just
>>> leads to more fragmentation.
>>
>> But does it?  Once an extsize hint is set I'd expect that we keep
>> getting more allocation with it.  And keeping the aligned is the concept
>> of a buddy allocator which reduces fragmentation.  Because of that I
>> wonder why we aren't doing that by default.
> 
> Histerical raisins?
> 
> We /could/ let extszhint influence allocation alignment by default, but
> then anyone who had (say) a 8k hint on a 32k raid stripe might be
> surprised when the allocator behavior changes.
> 
> What do you say about logic like this?
> 
> 	if (software_atomic) {
> 		/*
> 		 * align things so we can use hw atomic on the next
> 		 * overwrite, no matter what hw says
> 		 */
> 		args->alignment = ip->i_extsize;
 > 	} else if (raid_stripe) {> 		/* otherwise try to align for better 
raid performance */
> 		args->alignment = mp->m_dalign;
> 	} else if (ip->i_extsize) {
> 		/* if no raid, align to the hint provided */
> 		args->alignment = ip->i_extsize;
> 	} else {
> 		args->alignment = 1;
> 	}
> 
> Hm?  (I'm probably forgetting something...)
> 

note that for forcealign support, there was a prep patch to do something 
similar to this:
https://lore.kernel.org/linux-xfs/20240429174746.2132161-5-john.g.garry@oracle.com/


> --D