> On Jul 3, 2023, at 6:56 AM, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Fri, Jun 30, 2023 at 01:48:49PM -0400, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
