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

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

[Lukas Herbolt]

Removing the plain array to track the UUIDs and switch
xarray makes it more readable.

Signed-off-by: Lukas Herbolt <lukas@herbolt.com>
---
changes v2:
	add mutex to protect changes in xarray
	add m_uuid_table_index into xfs_mount to tract xarray index 

 fs/xfs/xfs_mount.c | 88 +++++++++++++++++++++++++---------------------
 fs/xfs/xfs_mount.h |  3 ++
 2 files changed, 51 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 0953f6ae94ab..165cc101d9fd 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -43,17 +43,48 @@
 #include "xfs_zone_alloc.h"
 
 static DEFINE_MUTEX(xfs_uuid_table_mutex);
-static int xfs_uuid_table_size;
-static uuid_t *xfs_uuid_table;
+static DEFINE_XARRAY_ALLOC(xfs_uuid_table);
+
+/*
+ * Helper fucntions to store UUID in xarray.
+ */
+static int
+xfs_uuid_insert(
+	uuid_t		*uuid,
+	unsigned int	*index)
+{
+	return xa_alloc(&xfs_uuid_table, index, uuid,
+			xa_limit_32b, GFP_KERNEL);
+}
+
+static uuid_t *
+xfs_uuid_search(
+	uuid_t		*new_uuid)
+{
+	unsigned long	index = 0;
+	uuid_t		*uuid;
+
+	xa_for_each(&xfs_uuid_table, index, uuid) {
+		if (uuid_equal(uuid, new_uuid))
+			return uuid;
+	}
+	return NULL;
+}
+
+static void
+xfs_uuid_delete(
+	uuid_t		*uuid,
+	unsigned int	index)
+{
+	ASSERT(uuid_equal(xa_load(&xfs_uuid_table, index), uuid));
+	xa_erase(&xfs_uuid_table, index);
+}
 
 void
 xfs_uuid_table_free(void)
 {
-	if (xfs_uuid_table_size == 0)
-		return;
-	kfree(xfs_uuid_table);
-	xfs_uuid_table = NULL;
-	xfs_uuid_table_size = 0;
+	ASSERT(xa_empty(&xfs_uuid_table));
+	xa_destroy(&xfs_uuid_table);
 }
 
 /*
@@ -65,7 +96,7 @@ xfs_uuid_mount(
 	struct xfs_mount	*mp)
 {
 	uuid_t			*uuid = &mp->m_sb.sb_uuid;
-	int			hole, i;
+	int			ret;
 
 	/* Publish UUID in struct super_block */
 	super_set_uuid(mp->m_super, uuid->b, sizeof(*uuid));
@@ -78,31 +109,16 @@ xfs_uuid_mount(
 		return -EINVAL;
 	}
 
-	mutex_lock(&xfs_uuid_table_mutex);
-	for (i = 0, hole = -1; i < xfs_uuid_table_size; i++) {
-		if (uuid_is_null(&xfs_uuid_table[i])) {
-			hole = i;
-			continue;
-		}
-		if (uuid_equal(uuid, &xfs_uuid_table[i]))
-			goto out_duplicate;
-	}
-
-	if (hole < 0) {
-		xfs_uuid_table = krealloc(xfs_uuid_table,
-			(xfs_uuid_table_size + 1) * sizeof(*xfs_uuid_table),
-			GFP_KERNEL | __GFP_NOFAIL);
-		hole = xfs_uuid_table_size++;
-	}
-	xfs_uuid_table[hole] = *uuid;
 	mutex_unlock(&xfs_uuid_table_mutex);
+	if (unlikely(xfs_uuid_search(uuid))) {
+		xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount",
+				uuid);
+		return -EINVAL;
+	}
 
-	return 0;
-
- out_duplicate:
+	ret = xfs_uuid_insert(uuid, &mp->m_uuid_table_index);
 	mutex_unlock(&xfs_uuid_table_mutex);
-	xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount", uuid);
-	return -EINVAL;
+	return ret;
 }
 
 STATIC void
@@ -110,22 +126,14 @@ xfs_uuid_unmount(
 	struct xfs_mount	*mp)
 {
 	uuid_t			*uuid = &mp->m_sb.sb_uuid;
-	int			i;
 
 	if (xfs_has_nouuid(mp))
 		return;
 
 	mutex_lock(&xfs_uuid_table_mutex);
-	for (i = 0; i < xfs_uuid_table_size; i++) {
-		if (uuid_is_null(&xfs_uuid_table[i]))
-			continue;
-		if (!uuid_equal(uuid, &xfs_uuid_table[i]))
-			continue;
-		memset(&xfs_uuid_table[i], 0, sizeof(uuid_t));
-		break;
-	}
-	ASSERT(i < xfs_uuid_table_size);
+	xfs_uuid_delete(uuid, mp->m_uuid_table_index);
 	mutex_unlock(&xfs_uuid_table_mutex);
+	return;
 }
 
 /*
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index b871dfde372b..4a2f2a30fbff 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -342,6 +342,9 @@ typedef struct xfs_mount {
 
 	/* Hook to feed dirent updates to an active online repair. */
 	struct xfs_hooks	m_dir_update_hooks;
+
+	/* Index of uuid record in the uuid xarray. */
+	unsigned int            m_uuid_table_index;
 } xfs_mount_t;
 
 #define M_IGEO(mp)		(&(mp)->m_ino_geo)
-- 
2.53.0

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

[Christoph Hellwig]

This doens't apply to current mainline or the XFS tree, as it
doesn't have the healthmon pointer in the mount structure yet.

> +/*
> + * Helper fucntions to store UUID in xarray.

s/fucntions/functions/

But I'd just drop the comment.

> + */
> +static int
> +xfs_uuid_insert(
> +	uuid_t		*uuid,
> +	unsigned int	*index)
> +{
> +	return xa_alloc(&xfs_uuid_table, index, uuid,
> +			xa_limit_32b, GFP_KERNEL);
> +}

... and open code this in the only caller.

> +static void
> +xfs_uuid_delete(
> +	uuid_t		*uuid,
> +	unsigned int	index)
> +{
> +	ASSERT(uuid_equal(xa_load(&xfs_uuid_table, index), uuid));
> +	xa_erase(&xfs_uuid_table, index);
> +}

>  	mutex_unlock(&xfs_uuid_table_mutex);

This looks like it should be a lock and not unlock?

> +	if (unlikely(xfs_uuid_search(uuid))) {
> +		xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount",
> +				uuid);
> +		return -EINVAL;

And this is missing an unlock?

Just curious how this was tested?  xfs/045 should cover mounting with
duplicate uuids.

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

[Lukas Herbolt]

On 2026-02-25 15:33, Christoph Hellwig wrote:
> This doens't apply to current mainline or the XFS tree, as it
> doesn't have the healthmon pointer in the mount structure yet.
> 
>> +/*
>> + * Helper fucntions to store UUID in xarray.
> 
> s/fucntions/functions/
> 
> But I'd just drop the comment.
> 
>> + */
>> +static int
>> +xfs_uuid_insert(
>> +	uuid_t		*uuid,
>> +	unsigned int	*index)
>> +{
>> +	return xa_alloc(&xfs_uuid_table, index, uuid,
>> +			xa_limit_32b, GFP_KERNEL);
>> +}
> 
> ... and open code this in the only caller.
> 
>> +static void
>> +xfs_uuid_delete(
>> +	uuid_t		*uuid,
>> +	unsigned int	index)
>> +{
>> +	ASSERT(uuid_equal(xa_load(&xfs_uuid_table, index), uuid));
>> +	xa_erase(&xfs_uuid_table, index);
>> +}
> 
>>  	mutex_unlock(&xfs_uuid_table_mutex);
> 
> This looks like it should be a lock and not unlock?
> 
>> +	if (unlikely(xfs_uuid_search(uuid))) {
>> +		xfs_warn(mp, "Filesystem has duplicate UUID %pU - can't mount",
>> +				uuid);
>> +		return -EINVAL;
> 
> And this is missing an unlock?
> 
> Just curious how this was tested?  xfs/045 should cover mounting with
> duplicate uuids.

I wrote my own simple test I will also try the xfs/045. As always thanks 
for the
notes, will fix it!.
-- 
-lhe