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

[Chuck Lever III <chuck.lever@oracle.com>]

> On Jul 3, 2023, at 6:56 AM, Christian Brauner <brauner@kernel.org> wrote:
> 
> 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.

The two types are in response to the xarray API, which is a little
confusing (sometimes it wants a ulong, sometimes a u32). I tried
to make the type casting explicit wherever possible.

Your clean-up looks like a readability improvement to me.


> 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,


--
Chuck Lever