Re: [PATCH 2/5] fuse: add STATX request

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 8/10/23 16:08, Miklos Szeredi wrote:
> On Thu, 10 Aug 2023 at 15:23, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/10/23 12:54, Miklos Szeredi wrote:
>>> Use the same structure as statx.
>>
>> Wouldn't it be easier to just include struct statx? Or is there an issue
>> with __u32, etc? If so, just a sufficiently large array could be used
>> and statx values just mem-copied in/out?
> 
> <linux/uapi/fuse.h> is OS independent.  Ports can grab it and use it
> in their userspace and kernel implementations.

Ok, but why not just like this?

struct fuse_statx {
	uint8_t fuse_statx_values[256];
}

struct fuse_statx fuse_statx;
struct statx *statx_ptr = (struct statx *)&fuse_statx.fuse_statx_values;


Hmm, I see an issue if that struct is passed over network and different 
OS are involved, which _might_ have different structs or sizes. So yeah, 
the copy approach is safest. Although as fuse_statx is between kernel 
and userspace only - it should not matter?

> 
> 
>>
>>>
>>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>> ---
>>>    include/uapi/linux/fuse.h | 56 ++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index b3fcab13fcd3..fe700b91b33b 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -207,6 +207,9 @@
>>>     *  - add FUSE_EXT_GROUPS
>>>     *  - add FUSE_CREATE_SUPP_GROUP
>>>     *  - add FUSE_HAS_EXPIRE_ONLY
>>> + *
>>> + *  7.39
>>> + *  - add FUSE_STATX and related structures
>>>     */
>>>
>>>    #ifndef _LINUX_FUSE_H
>>> @@ -242,7 +245,7 @@
>>>    #define FUSE_KERNEL_VERSION 7
>>>
>>>    /** Minor version number of this interface */
>>> -#define FUSE_KERNEL_MINOR_VERSION 38
>>> +#define FUSE_KERNEL_MINOR_VERSION 39
>>>
>>>    /** The node ID of the root inode */
>>>    #define FUSE_ROOT_ID 1
>>> @@ -269,6 +272,40 @@ struct fuse_attr {
>>>        uint32_t        flags;
>>>    };
>>>
>>> +/*
>>> + * The following structures are bit-for-bit compatible with the statx(2) ABI in
>>> + * Linux.
>>> + */
>>> +struct fuse_sx_time {
>>> +     int64_t         tv_sec;
>>> +     uint32_t        tv_nsec;
>>> +     int32_t         __reserved;
>>> +};
>>> +
>>> +struct fuse_statx {
>>> +     uint32_t        mask;
>>> +     uint32_t        blksize;
>>> +     uint64_t        attributes;
>>> +     uint32_t        nlink;
>>> +     uint32_t        uid;
>>> +     uint32_t        gid;
>>> +     uint16_t        mode;
>>> +     uint16_t        __spare0[1];
>>> +     uint64_t        ino;
>>> +     uint64_t        size;
>>> +     uint64_t        blocks;
>>> +     uint64_t        attributes_mask;
>>> +     struct fuse_sx_time     atime;
>>> +     struct fuse_sx_time     btime;
>>> +     struct fuse_sx_time     ctime;
>>> +     struct fuse_sx_time     mtime;
>>> +     uint32_t        rdev_major;
>>> +     uint32_t        rdev_minor;
>>> +     uint32_t        dev_major;
>>> +     uint32_t        dev_minor;
>>> +     uint64_t        __spare2[14];
>>> +};
>>
>> Looks like some recent values are missing?
> 
> It doesn't matter, since those parts are not used.
> 
>>
>>          /* 0x90 */
>>          __u64   stx_mnt_id;
>>          __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
>>          __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
>>          /* 0xa0 */
>>          __u64   __spare3[12];   /* Spare space for future expansion */
>>          /* 0x100 */
>>
>> Which is basically why my personal preference would be not to do have a
>> copy of the struct - there is maintenance overhead.
> 
> Whenever the new fields would be used in the kernel the fields can be
> added.  So no need to continually update the one in fuse, since those

Maybe I'm over optimizing, I just see that userspace side then also 
needs an updated struct - which can be easily forgotten. While plain 
struct statx would't have that issue.



Thanks,
Bernd