[PATCH 1/1] rpdfs: Initial extended attribute support

[PATCH 1/1] rpdfs: Initial extended attribute support

[Chris Kirby]

Initial extended attribute support

This change adds get, set, and remove support for user
extended attributes. The XATTR_CREATE and XATTR_REPLACE
flags are supported for setxattr.

Add the xattr_creates counter to the rpdfs inode. This is
incremented on each xattr create or replace operation and
is used as the lsq of the xattr key so that we don't need
to worry about collisions on the name hash.

Signed-off-by: Chris Kirby <ckirby@versity.com>
---
 fs/rpdfs/Makefile |   3 +-
 fs/rpdfs/inode.c  |   7 +
 fs/rpdfs/inode.h  |   4 +
 fs/rpdfs/super.c  |   3 +-
 fs/rpdfs/xattr.c  | 409 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/rpdfs/xattr.h  |   7 +
 6 files changed, 431 insertions(+), 2 deletions(-)
 create mode 100644 fs/rpdfs/xattr.c
 create mode 100644 fs/rpdfs/xattr.h

diff --git a/fs/rpdfs/Makefile b/fs/rpdfs/Makefile
index 4241adacf8c0..1466cdf49632 100644
--- a/fs/rpdfs/Makefile
+++ b/fs/rpdfs/Makefile
@@ -16,4 +16,5 @@ rpdfs-y	:= balloc.o \
 	   parse.o \
 	   rht.o \
 	   super.o \
-	   txn.o
+	   txn.o \
+	   xattr.o
diff --git a/fs/rpdfs/inode.c b/fs/rpdfs/inode.c
index 3f9a8e9d993f..2d83fa6352be 100644
--- a/fs/rpdfs/inode.c
+++ b/fs/rpdfs/inode.c
@@ -73,6 +73,9 @@ static void copy_rinode_to_vfs_inode(struct inode *inode, struct rpdfs_inode *ri
 	inode_set_mtime_to_ts(inode, ns_to_timespec64(le64_to_cpu(rinode->mtime_nsec)));
 
 	ri->dirents = rinode->dirents;
+
+	ri->xattrs = rinode->xattrs;
+	ri->xattr_creates = rinode->xattr_creates;
 }
 
 static __le64 cpu_ts64_to_le64_ns(struct timespec64 ts)
@@ -96,6 +99,9 @@ static void copy_vfs_inode_to_rinode(struct rpdfs_inode *rinode, struct inode *i
 	rinode->mtime_nsec = cpu_ts64_to_le64_ns(inode_get_mtime(inode));
 
 	rinode->dirents = ri->dirents;
+
+	rinode->xattrs = ri->xattrs;
+	rinode->xattr_creates = ri->xattr_creates;
 }
 
 /*
@@ -286,6 +292,7 @@ struct inode *rpdfs_new_inode(struct super_block *sb, struct rpdfs_ino_gen *ig)
 
 	ri = RPDFS_I(inode);
 	ri->refresh_wcount = 0;
+	ri->xattr_creates = 0;
 	ri->ig = *ig;
 	inode->i_ino = le64_to_cpu(ri->ig.ino);
 
diff --git a/fs/rpdfs/inode.h b/fs/rpdfs/inode.h
index a57017a4fca0..6b0fddb7f0fb 100644
--- a/fs/rpdfs/inode.h
+++ b/fs/rpdfs/inode.h
@@ -15,8 +15,12 @@ struct rpdfs_inode_info {
 	seqlock_t refresh_seqlock;
 	u64 refresh_wcount;
 
+	/* Uniquifier to avoid xattr name hash collisions */
+	__le64 xattr_creates;
+
 	struct rpdfs_ino_gen ig;
 	struct rpdfs_btree_root dirents;
+	struct rpdfs_btree_root xattrs;
 
 	struct inode vfs_inode;
 };
diff --git a/fs/rpdfs/super.c b/fs/rpdfs/super.c
index 9b18c25175b7..6a036d07cb30 100644
--- a/fs/rpdfs/super.c
+++ b/fs/rpdfs/super.c
@@ -17,6 +17,7 @@
 #include "net_tcp.h"
 #include "parse.h"
 #include "pr.h"
+#include "xattr.h"
 
 struct rpdfs_fs_context {
 	struct rpdfs_net_transport_addr source_addr;
@@ -42,7 +43,7 @@ static const struct super_operations rpdfs_sop = {
 static int rpdfs_set_super(struct super_block *sb, struct fs_context *fc)
 {
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
-	sb->s_xattr = NULL;
+	sb->s_xattr = rpdfs_xattr_handlers;
 	sb->s_op = &rpdfs_sop;
 	sb->s_time_gran = 1;
 	sb->s_time_min = 0;
diff --git a/fs/rpdfs/xattr.c b/fs/rpdfs/xattr.c
new file mode 100644
index 000000000000..a26615d0ebca
--- /dev/null
+++ b/fs/rpdfs/xattr.c
@@ -0,0 +1,409 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/string.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/xattr.h>
+#include <linux/xxhash.h>
+
+#include "btree_txn.h"
+#include "dir.h"
+#include "file.h"
+#include "inode.h"
+#include "pr.h"
+#include "xattr.h"
+
+#define RPDFS_XATTR_MAX_NAME_LEN	RPDFS_NAME_MAX
+
+struct key_xattr {
+	struct rpdfs_btree_key key;
+	const void *value;
+	struct rpdfs_xattr xattr;
+};
+
+static unsigned xattr_size(struct key_xattr *kx)
+{
+	int name_len = kx->xattr.name_len;
+
+	return offsetof(struct rpdfs_xattr, name[name_len]) +
+		__le16_to_cpu(kx->xattr.val_len);
+}
+
+static u64 xattr_hash(const char *name, const size_t name_len)
+{
+	return xxh64(name, name_len, RPDFS_XATTR_HASH_SEED);
+}
+
+static void init_xattr_key(struct rpdfs_btree_key *key, u64 hash)
+{
+	key->msq = cpu_to_le64(hash);
+	key->lsq = 0;
+}
+
+static void xattr_key_set_uniq(struct key_xattr *kx, __le64 creates)
+{
+	kx->key.lsq = creates;
+}
+
+static u64 xattr_key_hash(struct rpdfs_btree_key *key)
+{
+	return le64_to_cpu(key->msq);
+}
+
+static bool xattr_name_matches(struct rpdfs_btree_item_args *bti,
+			       const char *name, unsigned name_len)
+{
+	struct rpdfs_xattr *xattr = bti->val;
+
+	return xattr->name_len == name_len &&
+		memcmp(xattr->name, name, name_len) == 0;
+}
+
+static bool xattr_hash_and_name_matches(struct rpdfs_btree_item_args *bti,
+					struct key_xattr *kx)
+{
+	return xattr_key_hash(&bti->key) == xattr_key_hash(&kx->key) &&
+	       xattr_name_matches(bti, kx->xattr.name, kx->xattr.name_len);
+}
+
+static void init_key_xattr(struct key_xattr *kx, const char *name,
+			   const size_t name_len, const void *value,
+			   size_t size, bool copy_value)
+{
+	BUG_ON(name_len == 0);
+	BUG_ON(name_len > RPDFS_XATTR_MAX_NAME_LEN);
+	BUG_ON(name_len + size > RPDFS_XATTR_MAX_SIZE);
+
+	init_xattr_key(&kx->key, xattr_hash(name, name_len));
+
+	memcpy(&kx->xattr.name[0], name, name_len);
+	kx->xattr.name_len = name_len;
+	kx->xattr.val_len = cpu_to_le16(size);
+
+	if (copy_value) {
+		char *dest;
+
+		dest = (char *)kx + offsetof(struct key_xattr,
+				xattr.name[name_len]);
+		memcpy(dest, value, size);
+		kx->value = dest;
+	} else {
+		kx->value = value;
+	}
+}
+
+static struct key_xattr *alloc_key_xattr(const char *name, const void *value,
+					 size_t size, bool copy_value)
+{
+	const size_t name_len = strlen(name);
+	size_t alloc_size;
+	struct key_xattr *kx;
+
+	alloc_size = offsetof(struct key_xattr, xattr.name[name_len]);
+	if (copy_value)
+		alloc_size += size;
+
+	kx = kmalloc(alloc_size, GFP_NOFS);
+	if (kx)
+		init_key_xattr(kx, name, name_len, value, size, copy_value);
+
+	return kx;
+}
+
+static int add_xattr_item_cb(struct rpdfs_fs_info *rfi,
+			     struct rpdfs_btree_item_args *a,
+                             struct rpdfs_btree_item_args *b,
+			     struct rpdfs_btree_item_args *ins,
+			     void *arg)
+{
+	struct key_xattr *kx = arg;
+
+	if (a && xattr_key_hash(&kx->key) == xattr_key_hash(&a->key))
+		return -EEXIST;
+
+	ins->key = kx->key;
+	ins->val = &kx->xattr;
+	ins->val_size = xattr_size(kx);
+
+	return 0;
+}
+
+static int delete_xattr_item_cb(struct rpdfs_fs_info *rfi,
+				struct rpdfs_btree_item_args *a,
+				struct rpdfs_btree_item_args *b,
+				struct rpdfs_btree_item_args *ins,
+				void *arg)
+{
+	struct key_xattr *kx = arg;
+
+	if (a && xattr_hash_and_name_matches(a, kx))
+		return 0;
+	else if (b && xattr_hash_and_name_matches(b, kx))
+		return 1;
+
+	return -ENODATA;
+}
+
+static int prepare_add_xattr(struct rpdfs_fs_info *rfi,
+			     struct rpdfs_transaction *txn,
+			     struct inode *inode,
+			     struct key_xattr *kx)
+{
+	struct rpdfs_inode_info *ri = RPDFS_I(inode);
+
+	return rpdfs_btree_txn_prepare_insert(rfi, txn, &ri->xattrs, &kx->key,
+					      xattr_size(kx), add_xattr_item_cb,
+					      kx);
+}
+
+static int prepare_delete_xattr(struct rpdfs_fs_info *rfi,
+				struct rpdfs_transaction *txn,
+				struct inode *inode,
+				struct key_xattr *kx)
+{
+	struct rpdfs_inode_info *ri = RPDFS_I(inode);
+
+	return rpdfs_btree_txn_prepare_delete(rfi, txn, &ri->xattrs, &kx->key,
+					      xattr_size(kx),
+					      delete_xattr_item_cb, kx);
+}
+
+static int apply_add_xattr(struct rpdfs_fs_info *rfi,
+			   struct rpdfs_transaction *txn,
+			   struct inode *inode,
+			   struct key_xattr *kx)
+{
+	struct rpdfs_inode_info *ri = RPDFS_I(inode);
+
+        return rpdfs_btree_txn_apply_insert(rfi, txn, &ri->xattrs, &kx->key,
+                                            xattr_size(kx),
+                                            add_xattr_item_cb, kx);
+}
+
+static int apply_delete_xattr(struct rpdfs_fs_info *rfi,
+			      struct rpdfs_transaction *txn,
+			      struct inode *inode,
+			      struct key_xattr *kx)
+{
+        struct rpdfs_inode_info *ri = RPDFS_I(inode);
+
+	return rpdfs_btree_txn_apply_delete(rfi, txn, &ri->xattrs, &kx->key,
+					    xattr_size(kx),
+					    delete_xattr_item_cb, kx);
+}
+
+static int lookup_xattr_cb(struct rpdfs_fs_info *rfi,
+			   struct rpdfs_btree_item_args *a,
+			   struct rpdfs_btree_item_args *b,
+			   struct rpdfs_btree_item_args *c,
+			   void *arg)
+{
+	struct key_xattr *kx = arg;
+	struct rpdfs_btree_item_args *match = NULL;
+	int ret;
+
+	if (a && xattr_hash_and_name_matches(a, kx))
+		match = a;
+	else if (b && xattr_hash_and_name_matches(b, kx))
+		match = b;
+
+	if (match) {
+		struct rpdfs_xattr *xattr = match->val;
+		int name_len = xattr->name_len;
+		int in_val_len = __le16_to_cpu(xattr->val_len);
+		int buf_len = __le16_to_cpu(kx->xattr.val_len);
+		char *src;
+		char *dest;
+
+		src = (char *)xattr +
+			offsetof(struct rpdfs_xattr, name[name_len]);
+		dest = (char *)kx->value;
+
+		if (in_val_len <= buf_len) {
+			memcpy(dest, src, in_val_len);
+			ret = in_val_len;
+		} else {
+			/* the caller just wants the size */
+			if (buf_len == 0)
+				ret = in_val_len;
+			else
+				ret = -ERANGE;
+		}
+	} else {
+		ret = -ENODATA;
+	}
+
+	return ret;
+}
+
+static int lookup_xattr(struct rpdfs_fs_info *rfi, struct inode *inode,
+			const char *name, void *value, size_t size)
+{
+	DECLARE_RPDFS_TXN(txn);
+	struct key_xattr *kx;
+	int ret;
+
+	kx = alloc_key_xattr(name, value, size, false);
+
+	do {
+		ret = rpdfs_inode_txn_prepare(rfi, &txn, inode, 0) ?:
+		      rpdfs_btree_txn_prepare_lookup(rfi, &txn,
+						     &RPDFS_I(inode)->xattrs,
+						     &kx->key, lookup_xattr_cb,
+						     kx);
+	} while (rpdfs_txn_retry(rfi, &txn, &ret));
+
+	rpdfs_txn_reset(rfi, &txn);
+	kfree(kx);
+
+	return ret;
+}
+
+static int rpdfs_xattr_get(struct inode *inode, const char *name,
+			   void *value, size_t size)
+{
+	struct rpdfs_fs_info *rfi = RPDFS_INODE_FS(inode);
+
+	if (strlen(name) > RPDFS_XATTR_MAX_NAME_LEN)
+		return -ERANGE;
+
+	return lookup_xattr(rfi, inode, name, value, size);
+}
+
+static int rpdfs_xattr_set(struct inode *inode, const char *name,
+			   const void *value, size_t size, int flags)
+{
+	struct rpdfs_fs_info *rfi = RPDFS_INODE_FS(inode);
+	struct rpdfs_inode_info *ri = RPDFS_I(inode);
+	struct key_xattr *old_kx = NULL;
+	struct key_xattr *new_kx = NULL;
+	DECLARE_RPDFS_TXN(txn);
+	u64 creates;
+	bool have_old;
+	int ret;
+
+	if (name == NULL)
+		return -EINVAL;
+
+	if (strlen(name) > RPDFS_XATTR_MAX_NAME_LEN)
+		return -ERANGE;
+
+	if (value && size > RPDFS_XATTR_MAX_SIZE)
+		return -ERANGE;
+
+	if (((flags & XATTR_CREATE) && (flags & XATTR_REPLACE)) ||
+	    (flags & ~(XATTR_CREATE | XATTR_REPLACE)))
+		return -EINVAL;
+
+	old_kx = alloc_key_xattr(name, value, 0, false);
+	if (!old_kx)
+		return -ENOMEM;
+
+	new_kx = alloc_key_xattr(name, value, size, true);
+	if (!new_kx) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	do {
+		have_old = false;
+
+		ret = rpdfs_inode_txn_prepare(rfi, &txn, inode, RBAF_WRITE);
+		if (ret == 0) {
+			ret = rpdfs_btree_txn_prepare_lookup(rfi, &txn,
+							     &RPDFS_I(inode)->xattrs,
+							     &old_kx->key,
+							     lookup_xattr_cb,
+							     old_kx);
+
+			/* lookup returns the xattr size if it finds the name */
+			if (ret >= 0) {
+				have_old = true;
+				ret = 0;
+			}
+
+			/*
+			 * It's an error to specify XATTR_REPLACE if the name
+			 * doesn't already exist.
+			 */
+			if (!have_old) {
+				if (flags & XATTR_REPLACE)
+					ret = -ENODATA;
+				else
+					ret = 0;
+			}
+
+			/*
+			 * It's an error to specify XATTR_CREATE if the name
+			 * already exists.
+			 */
+			if (ret == 0 && have_old && (flags & XATTR_CREATE))
+				ret = -EEXIST;
+
+			if (ret == 0 && have_old) {
+				ret = prepare_delete_xattr(rfi, &txn, inode,
+							   old_kx);
+			}
+		}
+
+		if (ret == 0 && value != NULL) {
+			xattr_key_set_uniq(new_kx, ri->xattr_creates);
+
+			ret = prepare_add_xattr(rfi, &txn, inode, new_kx);
+		}
+	} while (rpdfs_txn_retry(rfi, &txn, &ret));
+
+	if (ret < 0)
+		goto out;
+
+	creates = __le64_to_cpu(ri->xattr_creates) + 1;
+
+	if (have_old)
+		apply_delete_xattr(rfi, &txn, inode, old_kx);
+
+	if (value) {
+		apply_add_xattr(rfi, &txn, inode, new_kx);
+		ri->xattr_creates = cpu_to_le64(creates);
+	}
+
+	rpdfs_inode_txn_update(rfi, &txn, inode);
+
+out:
+	rpdfs_txn_reset(rfi, &txn);
+	kfree(old_kx);
+	kfree(new_kx);
+
+	return ret;
+}
+
+static int rpdfs_xattr_get_handler(const struct xattr_handler *handler,
+				   struct dentry *unused, struct inode *inode,
+				   const char *name, void *value, size_t size)
+{
+	name = xattr_full_name(handler, name);
+
+	return rpdfs_xattr_get(inode, name, value, size);
+}
+
+static int rpdfs_xattr_set_handler(const struct xattr_handler *handler,
+				   struct mnt_idmap *idmap,
+				   struct dentry *unused, struct inode *inode,
+				   const char *name, const void *value,
+				   size_t size, int flags)
+{
+	name = xattr_full_name(handler, name);
+
+	return rpdfs_xattr_set(inode, name, value, size, flags);
+}
+
+static const struct xattr_handler rpdfs_xattr_user_handler = {
+	.prefix = XATTR_USER_PREFIX,
+        .get = rpdfs_xattr_get_handler,
+        .set = rpdfs_xattr_set_handler,
+};
+
+const struct xattr_handler * const rpdfs_xattr_handlers[] = {
+	&rpdfs_xattr_user_handler,
+	NULL
+};
+
diff --git a/fs/rpdfs/xattr.h b/fs/rpdfs/xattr.h
new file mode 100644
index 000000000000..d185f4ccf532
--- /dev/null
+++ b/fs/rpdfs/xattr.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef RPDFS_XATTR_H
+#define RPDFS_XATTR_H
+
+extern const struct xattr_handler * const rpdfs_xattr_handlers[];
+
+#endif
-- 
2.53.0

Re: [PATCH 1/1] rpdfs: Initial extended attribute support

[Zach Brown]

On Tue, Mar 03, 2026 at 09:20:52AM -0600, Chris Kirby wrote:
> Initial extended attribute support

Just a few cleanups..

> +#define RPDFS_XATTR_MAX_NAME_LEN	RPDFS_NAME_MAX

I suppose this should go in format-block.h by the other xattr constants.
(And like the dirent _NAME_MAX equivalent).

> +	if (copy_value) {
> +		char *dest;

I have a really old habit to not declare local variables in blocks from
old gcc bugs that used to blow out the stack.  (I think due to
larger alignments.. it's been so long!)  Anyway, if possible, declare
everything up top.

> +		dest = (char *)kx + offsetof(struct key_xattr,
> +				xattr.name[name_len]);
> +		memcpy(dest, value, size);

But in this case do we still need this single-use indirection after the
flex array fixes?  If we do, so be it, but if this is left over from
trying to avoid the array overflow warnings before the flex union fix
then it'd be nice to clean this (and the other instances) up.

> +static int add_xattr_item_cb(struct rpdfs_fs_info *rfi,
> +			     struct rpdfs_btree_item_args *a,
> +                             struct rpdfs_btree_item_args *b,
> +			     struct rpdfs_btree_item_args *ins,
> +			     void *arg)
> +{
> +	struct key_xattr *kx = arg;
> +
> +	if (a && xattr_key_hash(&kx->key) == xattr_key_hash(&a->key))
> +		return -EEXIST;

We all knew this, but to be explicit: these patterns don't make sense
for the xattrs that have a unique lsq in the key.  These come from the
dirents that only allow a single collision bit so that they can be done
in one call to a handful of items in one block.  When we have a full
64bits of collision the potentially matching entries can span lots of
items and blocks in the btree.

But the btree is going away, and this makes it an easy to follow
derivation from the dirents, so sure, let's go with this for now.  It
won't matter before we replace the btree and all of this changes.

> +	do {
> +		have_old = false;
> +
> +		ret = rpdfs_inode_txn_prepare(rfi, &txn, inode, RBAF_WRITE);
> +		if (ret == 0) {
> +			ret = rpdfs_btree_txn_prepare_lookup(rfi, &txn,
> +							     &RPDFS_I(inode)->xattrs,
> +							     &old_kx->key,
> +							     lookup_xattr_cb,
> +							     old_kx);
> +
> +			/* lookup returns the xattr size if it finds the name */
> +			if (ret >= 0) {
> +				have_old = true;
> +				ret = 0;
> +			}
> +
> +			/*
> +			 * It's an error to specify XATTR_REPLACE if the name
> +			 * doesn't already exist.
> +			 */
> +			if (!have_old) {
> +				if (flags & XATTR_REPLACE)
> +					ret = -ENODATA;
> +				else
> +					ret = 0;
> +			}
> +
> +			/*
> +			 * It's an error to specify XATTR_CREATE if the name
> +			 * already exists.
> +			 */
> +			if (ret == 0 && have_old && (flags & XATTR_CREATE))
> +				ret = -EEXIST;
> +
> +			if (ret == 0 && have_old) {
> +				ret = prepare_delete_xattr(rfi, &txn, inode,
> +							   old_kx);
> +			}
> +		}
> +
> +		if (ret == 0 && value != NULL) {
> +			xattr_key_set_uniq(new_kx, ri->xattr_creates);
> +
> +			ret = prepare_add_xattr(rfi, &txn, inode, new_kx);
> +		}
> +	} while (rpdfs_txn_retry(rfi, &txn, &ret));

Oof, that's a lot to be in the retry block.  If it's more than a handful
of lines of obvious cascading error checking, can you pop it up in a
helper function?

> +	creates = __le64_to_cpu(ri->xattr_creates) + 1;
> +
> +	if (have_old)
> +		apply_delete_xattr(rfi, &txn, inode, old_kx);
> +
> +	if (value) {
> +		apply_add_xattr(rfi, &txn, inode, new_kx);
> +		ri->xattr_creates = cpu_to_le64(creates);

Drop the "creates" that might not have been used and increment directly
with le64_add_cpu(&ri->xattr_creates, 1);

- z