2024-02-26 12:42:45

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [RFC PATCH 07/20] famfs: Add include/linux/famfs_ioctl.h

On Fri, 23 Feb 2024 11:41:51 -0600
John Groves <[email protected]> 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 <[email protected]>
> ---
> 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 */



2024-02-26 16:46:10

by John Groves

[permalink] [raw]
Subject: Re: [RFC PATCH 07/20] famfs: Add include/linux/famfs_ioctl.h

On 24/02/26 12:39PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:51 -0600
> John Groves <[email protected]> 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 <[email protected]>
> > ---
> > 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