Re: [PATCH] xfs: Use xarray to track SB UUIDs instead of plain array.

[Lukas Herbolt <lukas@herbolt.com>]

On 2026-02-02 08:37, Christoph Hellwig wrote:
>> +STATIC int
> 
> We're phasing out the magic STATIC, so pleae just use plain static
> here.
> 
>> +xfs_uuid_insert(uuid_t *uuid)
> 
> The somewhat unique XFS coding style uses separate lines for each
> argument:
> 
> static int
> xfs_uuid_insert(
> 	uuid_t			*uuid)
> 
>> +{
>> +	uint32_t index = 0;
> 
> .. and aligns the variables the same way:
> 
> {
> 	uint32_t		index = 0;
> 
> Although this one will go away with Darrick's suggestion anyway.
> 
>> +
>> +	return xa_alloc(&xfs_uuid_table, &index, uuid,
>> +			xa_limit_32b, GFP_KERNEL);
>> +}
>> +
>> +STATIC uuid_t
>> +*xfs_uuid_search(uuid_t *new_uuid)
> 
> The * for pointers goes with the type:
> 
> static uuid_t *
> xfs_uuid_search(
> 	uuid_t			*new_uuid)
> 
>> +	uuid_t *uuid = NULL;
> 
> no need to initialize the iterator to NULL before xa_for_each.
> 
>> +STATIC void
>> +xfs_uuid_delete(uuid_t *uuid)
>> +{
>> +	unsigned long index = 0;
>> +
>> +	xa_for_each(&xfs_uuid_table, index, uuid) {
>> +		xa_erase(&xfs_uuid_table, index);
>> +	}
> 
> I don't think this works as expected, as it just erases all uuids in 
> the
> table.
> 
>> +}
>> 
>>  void
>> -xfs_uuid_table_free(void)
>> +xfs_uuid_table_destroy(void)
> 
> I'd drop this rename.  Free works just fine here as a name.
> 
>> +	if (!xfs_uuid_search(uuid))
>> +		return xfs_uuid_insert(uuid);
>> 
>>  	xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount", 
>> uuid);
>>  	return -EINVAL;
> 
> Just return an error here if xfs_uuid_search finds something, and then
> open code the insert in the straight line path.
> 
>> @@ -110,22 +119,12 @@ xfs_uuid_unmount(
>>  	struct xfs_mount	*mp)
>>  {
>>  	uuid_t			*uuid = &mp->m_sb.sb_uuid;
>> 
>>  	if (xfs_has_nouuid(mp))
>>  		return;
>> +	xfs_uuid_delete(uuid);
>> +	return;
> 
> No need for the last return.  Also I think you can just open code
> xfs_uuid_delete here.

Thank you for all the comments! I will do all the changes.
-- 
-lhe