diff mbox series

[RFC,07/20] famfs: Add include/linux/famfs_ioctl.h

Message ID b40ca30e4bf689249a8c237909d9a7aaca9861e4.1708709155.git.john@groves.net (mailing list archive)
State Superseded, archived
Headers show
Series Introduce the famfs shared-memory file system | expand

Commit Message

John Groves Feb. 23, 2024, 5:41 p.m. UTC
Add uapi include file for famfs. The famfs user space uses ioctl on
individual files to pass in mapping information and file size. This
would be hard to do via sysfs or other means, since it's
file-specific.

Signed-off-by: John Groves <john@groves.net>
---
 include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 include/uapi/linux/famfs_ioctl.h

Comments

Randy Dunlap Feb. 24, 2024, 1:39 a.m. UTC | #1
Hi--

On 2/23/24 09:41, John Groves wrote:
> Add uapi include file for famfs. The famfs user space uses ioctl on
> individual files to pass in mapping information and file size. This
> would be hard to do via sysfs or other means, since it's
> file-specific.
> 
> Signed-off-by: John Groves <john@groves.net>
> ---
>  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 include/uapi/linux/famfs_ioctl.h
> 
> diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> new file mode 100644
> index 000000000000..6b3e6452d02f
> --- /dev/null
> +++ b/include/uapi/linux/famfs_ioctl.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * famfs - dax file system for shared fabric-attached memory
> + *
> + * Copyright 2023-2024 Micron Technology, Inc.
> + *
> + * This file system, originally based on ramfs the dax support from xfs,

      This is confusing to me. Is it just me? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> + * is intended to allow multiple host systems to mount a common file system
> + * view of dax files that map to shared memory.
> + */
> +#ifndef FAMFS_IOCTL_H
> +#define FAMFS_IOCTL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/uuid.h>
> +
> +#define FAMFS_MAX_EXTENTS 2
> +
> +enum extent_type {
> +	SIMPLE_DAX_EXTENT = 13,
> +	INVALID_EXTENT_TYPE,
> +};
> +
> +struct famfs_extent {
> +	__u64              offset;
> +	__u64              len;
> +};
> +
> +enum famfs_file_type {
> +	FAMFS_REG,
> +	FAMFS_SUPERBLOCK,
> +	FAMFS_LOG,
> +};
> +
> +/**

"/**" is used to begin kernel-doc comments, but this comment block is missing
a few entries to make it be kernel-doc compatible. Please either add them
or just use "/*" to begin the comment.

> + * struct famfs_ioc_map
> + *
> + * This is the metadata that indicates where the memory is for a famfs file
> + */
> +struct famfs_ioc_map {
> +	enum extent_type          extent_type;
> +	enum famfs_file_type      file_type;
> +	__u64                     file_size;
> +	__u64                     ext_list_count;
> +	struct famfs_extent       ext_list[FAMFS_MAX_EXTENTS];
> +};
> +
> +#define FAMFSIOC_MAGIC 'u'

This 'u' value should be documented in
Documentation/userspace-api/ioctl/ioctl-number.rst.

and if possible, you might want to use values like 0x5x or 0x8x
that don't conflict with the ioctl numbers that are already used
in the 'u' space.

> +
> +/* famfs file ioctl opcodes */
> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
> +
> +#endif /* FAMFS_IOCTL_H */
John Groves Feb. 24, 2024, 2:23 a.m. UTC | #2
On 24/02/23 05:39PM, Randy Dunlap wrote:
> Hi--
> 
> On 2/23/24 09:41, John Groves wrote:
> > Add uapi include file for famfs. The famfs user space uses ioctl on
> > individual files to pass in mapping information and file size. This
> > would be hard to do via sysfs or other means, since it's
> > file-specific.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> >  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 include/uapi/linux/famfs_ioctl.h
> > 
> > diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> > new file mode 100644
> > index 000000000000..6b3e6452d02f
> > --- /dev/null
> > +++ b/include/uapi/linux/famfs_ioctl.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * famfs - dax file system for shared fabric-attached memory
> > + *
> > + * Copyright 2023-2024 Micron Technology, Inc.
> > + *
> > + * This file system, originally based on ramfs the dax support from xfs,
> 
>       This is confusing to me. Is it just me? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Thanks Randy. I think I was trying to say "based on ramfs *plus* the dax
support from xfs. But I'll try to come up with something more clear than
that...

> 
> > + * is intended to allow multiple host systems to mount a common file system
> > + * view of dax files that map to shared memory.
> > + */
> > +#ifndef FAMFS_IOCTL_H
> > +#define FAMFS_IOCTL_H
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/uuid.h>
> > +
> > +#define FAMFS_MAX_EXTENTS 2
> > +
> > +enum extent_type {
> > +	SIMPLE_DAX_EXTENT = 13,
> > +	INVALID_EXTENT_TYPE,
> > +};
> > +
> > +struct famfs_extent {
> > +	__u64              offset;
> > +	__u64              len;
> > +};
> > +
> > +enum famfs_file_type {
> > +	FAMFS_REG,
> > +	FAMFS_SUPERBLOCK,
> > +	FAMFS_LOG,
> > +};
> > +
> > +/**
> 
> "/**" is used to begin kernel-doc comments, but this comment block is missing
> a few entries to make it be kernel-doc compatible. Please either add them
> or just use "/*" to begin the comment.

Will do, thanks. And I'll check the whole code base for other instances;
I won't be surprise if I was sloop about that in more than one place.

> 
> > + * struct famfs_ioc_map
> > + *
> > + * This is the metadata that indicates where the memory is for a famfs file
> > + */
> > +struct famfs_ioc_map {
> > +	enum extent_type          extent_type;
> > +	enum famfs_file_type      file_type;
> > +	__u64                     file_size;
> > +	__u64                     ext_list_count;
> > +	struct famfs_extent       ext_list[FAMFS_MAX_EXTENTS];
> > +};
> > +
> > +#define FAMFSIOC_MAGIC 'u'
> 
> This 'u' value should be documented in
> Documentation/userspace-api/ioctl/ioctl-number.rst.
> 
> and if possible, you might want to use values like 0x5x or 0x8x
> that don't conflict with the ioctl numbers that are already used
> in the 'u' space.

Will do. I was trying to be too clever there, invoking "mu" for
micron. 

> 
> > +
> > +/* famfs file ioctl opcodes */
> > +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
> > +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
> > +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
> > +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
> > +
> > +#endif /* FAMFS_IOCTL_H */
> 
> -- 
> #Randy

Thank you for taking the time to look it over, Randy.

John
Randy Dunlap Feb. 24, 2024, 3:27 a.m. UTC | #3
Hi John,

On 2/23/24 18:23, John Groves wrote:
>>> +
>>> +#define FAMFSIOC_MAGIC 'u'
>> This 'u' value should be documented in
>> Documentation/userspace-api/ioctl/ioctl-number.rst.
>>
>> and if possible, you might want to use values like 0x5x or 0x8x
>> that don't conflict with the ioctl numbers that are already used
>> in the 'u' space.
> Will do. I was trying to be too clever there, invoking "mu" for
> micron. 

I might have been unclear about this one.
It's OK to use 'u' but the values 1-4 below conflict in the 'u' space:

'u'   00-1F  linux/smb_fs.h                                          gone
'u'   20-3F  linux/uvcvideo.h                                        USB video class host driver
'u'   40-4f  linux/udmabuf.h

so if you could use
'u'   50-5f
or
'u'   80-8f

then those conflicts wouldn't be there.
HTH.

>>> +
>>> +/* famfs file ioctl opcodes */
>>> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
>>> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
>>> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
>>> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
John Groves Feb. 24, 2024, 11:32 p.m. UTC | #4
On 24/02/23 07:27PM, Randy Dunlap wrote:
> Hi John,
> 
> On 2/23/24 18:23, John Groves wrote:
> >>> +
> >>> +#define FAMFSIOC_MAGIC 'u'
> >> This 'u' value should be documented in
> >> Documentation/userspace-api/ioctl/ioctl-number.rst.
> >>
> >> and if possible, you might want to use values like 0x5x or 0x8x
> >> that don't conflict with the ioctl numbers that are already used
> >> in the 'u' space.
> > Will do. I was trying to be too clever there, invoking "mu" for
> > micron. 
> 
> I might have been unclear about this one.
> It's OK to use 'u' but the values 1-4 below conflict in the 'u' space:
> 
> 'u'   00-1F  linux/smb_fs.h                                          gone
> 'u'   20-3F  linux/uvcvideo.h                                        USB video class host driver
> 'u'   40-4f  linux/udmabuf.h
> 
> so if you could use
> 'u'   50-5f
> or
> 'u'   80-8f
> 
> then those conflicts wouldn't be there.
> HTH.
> 
> >>> +
> >>> +/* famfs file ioctl opcodes */
> >>> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
> >>> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
> >>> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
> >>> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
> 
> -- 
> #Randy

Thanks Randy; I think I'm the one that didn't read carefully enough.

Does this look right?

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 457e16f06e04..44a44809657b 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -288,6 +288,7 @@ Code  Seq#    Include File                                           Comments
 'u'   00-1F  linux/smb_fs.h                                          gone
 'u'   20-3F  linux/uvcvideo.h                                        USB video class host driver
 'u'   40-4f  linux/udmabuf.h                                         userspace dma-buf misc device
+'u'   50-5F  linux/famfs_ioctl.h                                     famfs shared memory file system
 'v'   00-1F  linux/ext2_fs.h                                         conflict!
 'v'   00-1F  linux/fs.h                                              conflict!
 'v'   00-0F  linux/sonypi.h                                          conflict!
diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
index 6b3e6452d02f..57521898ed57 100644
--- a/include/uapi/linux/famfs_ioctl.h
+++ b/include/uapi/linux/famfs_ioctl.h
@@ -48,9 +48,9 @@ struct famfs_ioc_map {
 #define FAMFSIOC_MAGIC 'u'

 /* famfs file ioctl opcodes */
-#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
-#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
-#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
-#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
+#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 0x50, struct famfs_ioc_map)
+#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 0x51, struct famfs_ioc_map)
+#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 0x52, struct famfs_extent)
+#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  0x53)

Thank you!
John
Randy Dunlap Feb. 24, 2024, 11:40 p.m. UTC | #5
On 2/24/24 15:32, John Groves wrote:
> On 24/02/23 07:27PM, Randy Dunlap wrote:
>> Hi John,
>>
>> On 2/23/24 18:23, John Groves wrote:
>>>>> +
>>>>> +#define FAMFSIOC_MAGIC 'u'
>>>> This 'u' value should be documented in
>>>> Documentation/userspace-api/ioctl/ioctl-number.rst.
>>>>
>>>> and if possible, you might want to use values like 0x5x or 0x8x
>>>> that don't conflict with the ioctl numbers that are already used
>>>> in the 'u' space.
>>> Will do. I was trying to be too clever there, invoking "mu" for
>>> micron. 
>>
>> I might have been unclear about this one.
>> It's OK to use 'u' but the values 1-4 below conflict in the 'u' space:
>>
>> 'u'   00-1F  linux/smb_fs.h                                          gone
>> 'u'   20-3F  linux/uvcvideo.h                                        USB video class host driver
>> 'u'   40-4f  linux/udmabuf.h
>>
>> so if you could use
>> 'u'   50-5f
>> or
>> 'u'   80-8f
>>
>> then those conflicts wouldn't be there.
>> HTH.
>>
>>>>> +
>>>>> +/* famfs file ioctl opcodes */
>>>>> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
>>>>> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
>>>>> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
>>>>> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
>>
>> -- 
>> #Randy
> 
> Thanks Randy; I think I'm the one that didn't read carefully enough.
> 
> Does this look right?
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 457e16f06e04..44a44809657b 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -288,6 +288,7 @@ Code  Seq#    Include File                                           Comments
>  'u'   00-1F  linux/smb_fs.h                                          gone
>  'u'   20-3F  linux/uvcvideo.h                                        USB video class host driver
>  'u'   40-4f  linux/udmabuf.h                                         userspace dma-buf misc device
> +'u'   50-5F  linux/famfs_ioctl.h                                     famfs shared memory file system
>  'v'   00-1F  linux/ext2_fs.h                                         conflict!
>  'v'   00-1F  linux/fs.h                                              conflict!
>  'v'   00-0F  linux/sonypi.h                                          conflict!
> diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> index 6b3e6452d02f..57521898ed57 100644
> --- a/include/uapi/linux/famfs_ioctl.h
> +++ b/include/uapi/linux/famfs_ioctl.h
> @@ -48,9 +48,9 @@ struct famfs_ioc_map {
>  #define FAMFSIOC_MAGIC 'u'
> 
>  /* famfs file ioctl opcodes */
> -#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
> -#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
> -#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
> -#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 0x50, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 0x51, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 0x52, struct famfs_extent)
> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  0x53)
> 
> Thank you!
> John
> 

Yes, that looks good.
Thanks.
Jonathan Cameron Feb. 26, 2024, 12:39 p.m. UTC | #6
On Fri, 23 Feb 2024 11:41:51 -0600
John Groves <John@Groves.net> wrote:

> Add uapi include file for famfs. The famfs user space uses ioctl on
> individual files to pass in mapping information and file size. This
> would be hard to do via sysfs or other means, since it's
> file-specific.
> 
> Signed-off-by: John Groves <john@groves.net>
> ---
>  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 include/uapi/linux/famfs_ioctl.h
> 
> diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> new file mode 100644
> index 000000000000..6b3e6452d02f
> --- /dev/null
> +++ b/include/uapi/linux/famfs_ioctl.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * famfs - dax file system for shared fabric-attached memory
> + *
> + * Copyright 2023-2024 Micron Technology, Inc.
> + *
> + * This file system, originally based on ramfs the dax support from xfs,
> + * is intended to allow multiple host systems to mount a common file system
> + * view of dax files that map to shared memory.
> + */
> +#ifndef FAMFS_IOCTL_H
> +#define FAMFS_IOCTL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/uuid.h>
> +
> +#define FAMFS_MAX_EXTENTS 2
Why 2?
> +
> +enum extent_type {
> +	SIMPLE_DAX_EXTENT = 13,

Comment on this would be good to have

> +	INVALID_EXTENT_TYPE,
> +};
> +
> +struct famfs_extent {
> +	__u64              offset;
> +	__u64              len;
> +};
> +
> +enum famfs_file_type {
> +	FAMFS_REG,
> +	FAMFS_SUPERBLOCK,
> +	FAMFS_LOG,
> +};
> +
> +/**
> + * struct famfs_ioc_map
> + *
> + * This is the metadata that indicates where the memory is for a famfs file
> + */
> +struct famfs_ioc_map {
> +	enum extent_type          extent_type;
> +	enum famfs_file_type      file_type;

These are going to be potentially varying in size depending on arch, compiler
settings etc.  Been a while, but I though best practice for uapi was always
fixed size elements even though we lose the typing.


> +	__u64                     file_size;
> +	__u64                     ext_list_count;
> +	struct famfs_extent       ext_list[FAMFS_MAX_EXTENTS];
> +};
> +
> +#define FAMFSIOC_MAGIC 'u'
> +
> +/* famfs file ioctl opcodes */
> +#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
> +#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
> +#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
> +
> +#endif /* FAMFS_IOCTL_H */
John Groves Feb. 26, 2024, 4:44 p.m. UTC | #7
On 24/02/26 12:39PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:51 -0600
> John Groves <John@Groves.net> wrote:
> 
> > Add uapi include file for famfs. The famfs user space uses ioctl on
> > individual files to pass in mapping information and file size. This
> > would be hard to do via sysfs or other means, since it's
> > file-specific.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> >  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 include/uapi/linux/famfs_ioctl.h
> > 
> > diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> > new file mode 100644
> > index 000000000000..6b3e6452d02f
> > --- /dev/null
> > +++ b/include/uapi/linux/famfs_ioctl.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * famfs - dax file system for shared fabric-attached memory
> > + *
> > + * Copyright 2023-2024 Micron Technology, Inc.
> > + *
> > + * This file system, originally based on ramfs the dax support from xfs,
> > + * is intended to allow multiple host systems to mount a common file system
> > + * view of dax files that map to shared memory.
> > + */
> > +#ifndef FAMFS_IOCTL_H
> > +#define FAMFS_IOCTL_H
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/uuid.h>
> > +
> > +#define FAMFS_MAX_EXTENTS 2
> Why 2?

You catch everything! 

This limit is in place to avoid supporting somethign we're not testing. It
will probably be raised later.

Currently user space doesn't support deleting files, which makes it easy
to ignore whether any clients have a stale view of metadata. If there is
no delete, there's actually no reason to have more than 1 extent.

> > +
> > +enum extent_type {
> > +	SIMPLE_DAX_EXTENT = 13,
> 
> Comment on this would be good to have

Done. Basically we anticipate there being other types of extents in the
future.

> 
> > +	INVALID_EXTENT_TYPE,
> > +};
> > +
> > +struct famfs_extent {
> > +	__u64              offset;
> > +	__u64              len;
> > +};
> > +
> > +enum famfs_file_type {
> > +	FAMFS_REG,
> > +	FAMFS_SUPERBLOCK,
> > +	FAMFS_LOG,
> > +};
> > +
> > +/**
> > + * struct famfs_ioc_map
> > + *
> > + * This is the metadata that indicates where the memory is for a famfs file
> > + */
> > +struct famfs_ioc_map {
> > +	enum extent_type          extent_type;
> > +	enum famfs_file_type      file_type;
> 
> These are going to be potentially varying in size depending on arch, compiler
> settings etc.  Been a while, but I though best practice for uapi was always
> fixed size elements even though we lose the typing.

I might not be following you fully here. User space is running the same
arch as kernel, so an enum can't be a different size, right? It could be
a different size on different arches, but this is just between user/kernel.

I initially thought of XDR for on-media-format, which file systems need
to do with on-media structs (superblocks, logs, inodes, etc. etc.). But
this struct is not used in that way.

In fact, famfs' on-media/in-memory metadata (superblock, log, log entries)
is only ever read read and written by user space - so it's the user space
code that needs XDR on-media-format handling.

So to clarify - do you think those enums should be u32 or the like?

Thanks!
John
Jonathan Cameron Feb. 26, 2024, 4:56 p.m. UTC | #8
On Mon, 26 Feb 2024 10:44:43 -0600
John Groves <John@groves.net> wrote:

> On 24/02/26 12:39PM, Jonathan Cameron wrote:
> > On Fri, 23 Feb 2024 11:41:51 -0600
> > John Groves <John@Groves.net> wrote:
> >   
> > > Add uapi include file for famfs. The famfs user space uses ioctl on
> > > individual files to pass in mapping information and file size. This
> > > would be hard to do via sysfs or other means, since it's
> > > file-specific.
> > > 
> > > Signed-off-by: John Groves <john@groves.net>
> > > ---
> > >  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 include/uapi/linux/famfs_ioctl.h
> > > 
> > > diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> > > new file mode 100644
> > > index 000000000000..6b3e6452d02f
> > > --- /dev/null
> > > +++ b/include/uapi/linux/famfs_ioctl.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/*
> > > + * famfs - dax file system for shared fabric-attached memory
> > > + *
> > > + * Copyright 2023-2024 Micron Technology, Inc.
> > > + *
> > > + * This file system, originally based on ramfs the dax support from xfs,
> > > + * is intended to allow multiple host systems to mount a common file system
> > > + * view of dax files that map to shared memory.
> > > + */
> > > +#ifndef FAMFS_IOCTL_H
> > > +#define FAMFS_IOCTL_H
> > > +
> > > +#include <linux/ioctl.h>
> > > +#include <linux/uuid.h>
> > > +
> > > +#define FAMFS_MAX_EXTENTS 2  
> > Why 2?  
> 
> You catch everything! 
> 
> This limit is in place to avoid supporting somethign we're not testing. It
> will probably be raised later.
> 
> Currently user space doesn't support deleting files, which makes it easy
> to ignore whether any clients have a stale view of metadata. If there is
> no delete, there's actually no reason to have more than 1 extent.
Then have 1. + a Comment on why it is 1.
> 
> > > +
> > > +enum extent_type {
> > > +	SIMPLE_DAX_EXTENT = 13,  
> > 
> > Comment on this would be good to have  
> 
> Done. Basically we anticipate there being other types of extents in the
> future.

I was more curious about the 13!

> 
> >   
> > > +	INVALID_EXTENT_TYPE,
> > > +};
> > > +
> > > +struct famfs_extent {
> > > +	__u64              offset;
> > > +	__u64              len;
> > > +};
> > > +
> > > +enum famfs_file_type {
> > > +	FAMFS_REG,
> > > +	FAMFS_SUPERBLOCK,
> > > +	FAMFS_LOG,
> > > +};
> > > +
> > > +/**
> > > + * struct famfs_ioc_map
> > > + *
> > > + * This is the metadata that indicates where the memory is for a famfs file
> > > + */
> > > +struct famfs_ioc_map {
> > > +	enum extent_type          extent_type;
> > > +	enum famfs_file_type      file_type;  
> > 
> > These are going to be potentially varying in size depending on arch, compiler
> > settings etc.  Been a while, but I though best practice for uapi was always
> > fixed size elements even though we lose the typing.  
> 
> I might not be following you fully here. User space is running the same
> arch as kernel, so an enum can't be a different size, right? It could be
> a different size on different arches, but this is just between user/kernel.

I can't remember why, but this has bitten me in the past.
Ah, should have known Daniel would have written something on it ;)
https://www.kernel.org/doc/html/next/process/botching-up-ioctls.html

It's the fun of need for compat ioctls with 32bit userspace on 64bit kernels.

The alignment one is key as well. That bit me more than once due to
32bit x86 aligning 64 bit integers at 32 bits.

We could just not support these cases but it's easy to get right so why
bother with complexity of ruling them out.

> 
> I initially thought of XDR for on-media-format, which file systems need
> to do with on-media structs (superblocks, logs, inodes, etc. etc.). But
> this struct is not used in that way.
> 
> In fact, famfs' on-media/in-memory metadata (superblock, log, log entries)
> is only ever read read and written by user space - so it's the user space
> code that needs XDR on-media-format handling.
> 
> So to clarify - do you think those enums should be u32 or the like?

Yes. As it's userspace, uint32_t maybe or __u32. I 'think'
both are acceptable in uapi headers these days.

> 
> Thanks!
> John
>
John Groves Feb. 26, 2024, 6:04 p.m. UTC | #9
On 24/02/26 04:56PM, Jonathan Cameron wrote:
> On Mon, 26 Feb 2024 10:44:43 -0600
> John Groves <John@groves.net> wrote:
> 
> > On 24/02/26 12:39PM, Jonathan Cameron wrote:
> > > On Fri, 23 Feb 2024 11:41:51 -0600
> > > John Groves <John@Groves.net> wrote:
> > >   
> > > > Add uapi include file for famfs. The famfs user space uses ioctl on
> > > > individual files to pass in mapping information and file size. This
> > > > would be hard to do via sysfs or other means, since it's
> > > > file-specific.
> > > > 
> > > > Signed-off-by: John Groves <john@groves.net>
> > > > ---
> > > >  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 include/uapi/linux/famfs_ioctl.h
> > > > 
> > > > diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> > > > new file mode 100644
> > > > index 000000000000..6b3e6452d02f
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/famfs_ioctl.h
> > > > @@ -0,0 +1,56 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > +/*
> > > > + * famfs - dax file system for shared fabric-attached memory
> > > > + *
> > > > + * Copyright 2023-2024 Micron Technology, Inc.
> > > > + *
> > > > + * This file system, originally based on ramfs the dax support from xfs,
> > > > + * is intended to allow multiple host systems to mount a common file system
> > > > + * view of dax files that map to shared memory.
> > > > + */
> > > > +#ifndef FAMFS_IOCTL_H
> > > > +#define FAMFS_IOCTL_H
> > > > +
> > > > +#include <linux/ioctl.h>
> > > > +#include <linux/uuid.h>
> > > > +
> > > > +#define FAMFS_MAX_EXTENTS 2  
> > > Why 2?  
> > 
> > You catch everything! 
> > 
> > This limit is in place to avoid supporting somethign we're not testing. It
> > will probably be raised later.
> > 
> > Currently user space doesn't support deleting files, which makes it easy
> > to ignore whether any clients have a stale view of metadata. If there is
> > no delete, there's actually no reason to have more than 1 extent.
> Then have 1. + a Comment on why it is 1.

Actually we test the 2 case. That seemed important to testing ioctl and
famfs_meta_to_dax_offset(). It just doesn't yet happen in the wild. Will
clarify with a comment.

> > 
> > > > +
> > > > +enum extent_type {
> > > > +	SIMPLE_DAX_EXTENT = 13,  
> > > 
> > > Comment on this would be good to have  
> > 
> > Done. Basically we anticipate there being other types of extents in the
> > future.
> 
> I was more curious about the 13!

I think I was just being feisty that day. Will drop that...

> 
> > 
> > >   
> > > > +	INVALID_EXTENT_TYPE,
> > > > +};
> > > > +
> > > > +struct famfs_extent {
> > > > +	__u64              offset;
> > > > +	__u64              len;
> > > > +};
> > > > +
> > > > +enum famfs_file_type {
> > > > +	FAMFS_REG,
> > > > +	FAMFS_SUPERBLOCK,
> > > > +	FAMFS_LOG,
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct famfs_ioc_map
> > > > + *
> > > > + * This is the metadata that indicates where the memory is for a famfs file
> > > > + */
> > > > +struct famfs_ioc_map {
> > > > +	enum extent_type          extent_type;
> > > > +	enum famfs_file_type      file_type;  
> > > 
> > > These are going to be potentially varying in size depending on arch, compiler
> > > settings etc.  Been a while, but I though best practice for uapi was always
> > > fixed size elements even though we lose the typing.  
> > 
> > I might not be following you fully here. User space is running the same
> > arch as kernel, so an enum can't be a different size, right? It could be
> > a different size on different arches, but this is just between user/kernel.
> 
> I can't remember why, but this has bitten me in the past.
> Ah, should have known Daniel would have written something on it ;)
> https://www.kernel.org/doc/html/next/process/botching-up-ioctls.html
> 
> It's the fun of need for compat ioctls with 32bit userspace on 64bit kernels.
> 
> The alignment one is key as well. That bit me more than once due to
> 32bit x86 aligning 64 bit integers at 32 bits.
> 
> We could just not support these cases but it's easy to get right so why
> bother with complexity of ruling them out.

Makes sense. Will do.

> 
> > 
> > I initially thought of XDR for on-media-format, which file systems need
> > to do with on-media structs (superblocks, logs, inodes, etc. etc.). But
> > this struct is not used in that way.
> > 
> > In fact, famfs' on-media/in-memory metadata (superblock, log, log entries)
> > is only ever read read and written by user space - so it's the user space
> > code that needs XDR on-media-format handling.
> > 
> > So to clarify - do you think those enums should be u32 or the like?
> 
> Yes. As it's userspace, uint32_t maybe or __u32. I 'think'
> both are acceptable in uapi headers these days.

Roger that.

Thanks,
John
diff mbox series

Patch

diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
new file mode 100644
index 000000000000..6b3e6452d02f
--- /dev/null
+++ b/include/uapi/linux/famfs_ioctl.h
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * famfs - dax file system for shared fabric-attached memory
+ *
+ * Copyright 2023-2024 Micron Technology, Inc.
+ *
+ * This file system, originally based on ramfs the dax support from xfs,
+ * is intended to allow multiple host systems to mount a common file system
+ * view of dax files that map to shared memory.
+ */
+#ifndef FAMFS_IOCTL_H
+#define FAMFS_IOCTL_H
+
+#include <linux/ioctl.h>
+#include <linux/uuid.h>
+
+#define FAMFS_MAX_EXTENTS 2
+
+enum extent_type {
+	SIMPLE_DAX_EXTENT = 13,
+	INVALID_EXTENT_TYPE,
+};
+
+struct famfs_extent {
+	__u64              offset;
+	__u64              len;
+};
+
+enum famfs_file_type {
+	FAMFS_REG,
+	FAMFS_SUPERBLOCK,
+	FAMFS_LOG,
+};
+
+/**
+ * struct famfs_ioc_map
+ *
+ * This is the metadata that indicates where the memory is for a famfs file
+ */
+struct famfs_ioc_map {
+	enum extent_type          extent_type;
+	enum famfs_file_type      file_type;
+	__u64                     file_size;
+	__u64                     ext_list_count;
+	struct famfs_extent       ext_list[FAMFS_MAX_EXTENTS];
+};
+
+#define FAMFSIOC_MAGIC 'u'
+
+/* famfs file ioctl opcodes */
+#define FAMFSIOC_MAP_CREATE    _IOW(FAMFSIOC_MAGIC, 1, struct famfs_ioc_map)
+#define FAMFSIOC_MAP_GET       _IOR(FAMFSIOC_MAGIC, 2, struct famfs_ioc_map)
+#define FAMFSIOC_MAP_GETEXT    _IOR(FAMFSIOC_MAGIC, 3, struct famfs_extent)
+#define FAMFSIOC_NOP           _IO(FAMFSIOC_MAGIC,  4)
+
+#endif /* FAMFS_IOCTL_H */