Re: [PATCH v7 1/3] libfs: Add directory operations for stable offsets

[Christian Brauner <brauner@kernel.org>]

On Fri, Jun 30, 2023 at 01:48:49PM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Create a vector of directory operations in fs/libfs.c that handles
> directory seeks and readdir via stable offsets instead of the
> current cursor-based mechanism.
> 
> For the moment these are unused.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  Documentation/filesystems/locking.rst |    2 
>  Documentation/filesystems/vfs.rst     |    6 +
>  fs/libfs.c                            |  247 +++++++++++++++++++++++++++++++++
>  include/linux/fs.h                    |   18 ++
>  4 files changed, 272 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index ed148919e11a..6a928fee3400 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -85,6 +85,7 @@ prototypes::
>  			    struct dentry *dentry, struct fileattr *fa);
>  	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
>  	struct posix_acl * (*get_acl)(struct mnt_idmap *, struct dentry *, int);
> +	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
>  
>  locking rules:
>  	all may block
> @@ -115,6 +116,7 @@ atomic_open:	shared (exclusive if O_CREAT is set in open flags)
>  tmpfile:	no
>  fileattr_get:	no or exclusive
>  fileattr_set:	exclusive
> +get_offset_ctx: no
>  ==============	=============================================
>  
>  
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index cb2a97e49872..898d0b43109e 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -515,6 +515,7 @@ As of kernel 2.6.22, the following members are defined:
>  		int (*fileattr_set)(struct mnt_idmap *idmap,
>  				    struct dentry *dentry, struct fileattr *fa);
>  		int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
> +	        struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
>  	};
>  
>  Again, all methods are called without any locks being held, unless
> @@ -675,7 +676,10 @@ otherwise noted.
>  	called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
>  	change miscellaneous file flags and attributes.  Callers hold
>  	i_rwsem exclusive.  If unset, then fall back to f_op->ioctl().
> -
> +``get_offset_ctx``
> +	called to get the offset context for a directory inode. A
> +        filesystem must define this operation to use
> +        simple_offset_dir_operations.
>  
>  The Address Space Object
>  ========================
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 5b851315eeed..68b0000dc518 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -239,6 +239,253 @@ const struct inode_operations simple_dir_inode_operations = {
>  };
>  EXPORT_SYMBOL(simple_dir_inode_operations);
>  
> +static void offset_set(struct dentry *dentry, unsigned long offset)
> +{
> +	dentry->d_fsdata = (void *)offset;
> +}
> +
> +static unsigned long dentry2offset(struct dentry *dentry)
> +{
> +	return (unsigned long)dentry->d_fsdata;
> +}

This looks fine to me and tmpfs xfstests seem happy too. Currently we
use unsigned long in some places, and u32 in some other places. It's not
a big deal but I would prefer if we kept this consistent and made it
clear everywhere that the offset is a 32 bit unsigned and that the
xarray's limit is U32_MAX. So I would like to fold the following change
into this series unless there are objections:

diff --git a/fs/libfs.c b/fs/libfs.c
index 68b0000dc518..a7e56baf8bbd 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -239,14 +239,14 @@ const struct inode_operations simple_dir_inode_operations = {
 };
 EXPORT_SYMBOL(simple_dir_inode_operations);

-static void offset_set(struct dentry *dentry, unsigned long offset)
+static void offset_set(struct dentry *dentry, u32 offset)
 {
-       dentry->d_fsdata = (void *)offset;
+       dentry->d_fsdata = (void *)((uintptr_t)(offset));
 }

-static unsigned long dentry2offset(struct dentry *dentry)
+static u32 dentry2offset(struct dentry *dentry)
 {
-       return (unsigned long)dentry->d_fsdata;
+       return (u32)((uintptr_t)(dentry->d_fsdata));
 }

 /**
@@ -296,12 +296,13 @@ int simple_offset_add(struct offset_ctx *octx, struct dentry *dentry)
  */
 void simple_offset_remove(struct offset_ctx *octx, struct dentry *dentry)
 {
-       unsigned long index = dentry2offset(dentry);
+       u32 offset;

-       if (index == 0)
+       offset = dentry2offset(dentry);
+       if (offset == 0)
                return;

-       xa_erase(&octx->xa, index);
+       xa_erase(&octx->xa, offset);
        offset_set(dentry, 0);
 }

@@ -322,8 +323,8 @@ int simple_offset_rename_exchange(struct inode *old_dir,
 {
        struct offset_ctx *old_ctx = old_dir->i_op->get_offset_ctx(old_dir);
        struct offset_ctx *new_ctx = new_dir->i_op->get_offset_ctx(new_dir);
-       unsigned long old_index = dentry2offset(old_dentry);
-       unsigned long new_index = dentry2offset(new_dentry);
+       u32 old_index = dentry2offset(old_dentry);
+       u32 new_index = dentry2offset(new_dentry);
        int ret;

        simple_offset_remove(old_ctx, old_dentry);
@@ -414,7 +415,7 @@ static struct dentry *offset_find_next(struct xa_state *xas)

 static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
 {
-       loff_t offset = dentry2offset(dentry);
+       u32 offset = dentry2offset(dentry);
        struct inode *inode = d_inode(dentry);

        return ctx->actor(ctx, dentry->d_name.name, dentry->d_name.len, offset,