diff mbox series

[RFC,08/20] famfs: Add famfs_internal.h

Message ID 13556dbbd8d0f51bc31e3bdec796283fe85c6baf.1708709155.git.john@groves.net
State Superseded
Headers show
Series Introduce the famfs shared-memory file system | expand

Commit Message

John Groves Feb. 23, 2024, 5:41 p.m. UTC
Add the famfs_internal.h include file. This contains internal data
structures such as the per-file metadata structure (famfs_file_meta)
and extent formats.

Signed-off-by: John Groves <john@groves.net>
---
 fs/famfs/famfs_internal.h | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 fs/famfs/famfs_internal.h

Comments

Jonathan Cameron Feb. 26, 2024, 12:48 p.m. UTC | #1
On Fri, 23 Feb 2024 11:41:52 -0600
John Groves <John@Groves.net> wrote:

> Add the famfs_internal.h include file. This contains internal data
> structures such as the per-file metadata structure (famfs_file_meta)
> and extent formats.
> 
> Signed-off-by: John Groves <john@groves.net>
Hi John,

Build this up as you add the definitions in later patches.

Separate header patches just make people jump back and forth when trying
to review.  Obviously more work to build this stuff up cleanly but
it's worth doing to save review time.

Generally I'd plumb up Kconfig and Makefile a the beginning as it means
that the set is bisectable and we can check the logic of building each stage.
That is harder to do but tends to bring benefits in forcing clear step
wise approach on a patch set. Feel free to ignore this one though as it
can slow things down.

A few trivial comments inline.

> ---
>  fs/famfs/famfs_internal.h | 53 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 fs/famfs/famfs_internal.h
> 
> diff --git a/fs/famfs/famfs_internal.h b/fs/famfs/famfs_internal.h
> new file mode 100644
> index 000000000000..af3990d43305
> --- /dev/null
> +++ b/fs/famfs/famfs_internal.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 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_INTERNAL_H
> +#define FAMFS_INTERNAL_H
> +
> +#include <linux/atomic.h>

Why?

> +#include <linux/famfs_ioctl.h>
> +
> +#define FAMFS_MAGIC 0x87b282ff
> +
> +#define FAMFS_BLKDEV_MODE (FMODE_READ|FMODE_WRITE)

Spaces around | 

> +
> +extern const struct file_operations      famfs_file_operations;

I wouldn't force alignment. It rots too often as new stuff gets added
and doesn't really help readability much.

> +
> +/*
> + * Each famfs dax file has this hanging from its inode->i_private.
> + */
> +struct famfs_file_meta {
> +	int                   error;
> +	enum famfs_file_type  file_type;
> +	size_t                file_size;
> +	enum extent_type      tfs_extent_type;
> +	size_t                tfs_extent_ct;
> +	struct famfs_extent   tfs_extents[];  /* flexible array */

Comment kind of obvious ;) I'd drop it.  Though we have
magic markings for __counted_by which would be good to use from the start.



> +};
> +
> +struct famfs_mount_opts {
> +	umode_t mode;
> +};
> +
> +extern const struct iomap_ops             famfs_iomap_ops;
> +extern const struct vm_operations_struct  famfs_file_vm_ops;
> +
> +#define ROOTDEV_STRLEN 80

Why?  You aren't creating an array of this size here so I can't
immediately see what the define is for.

> +
> +struct famfs_fs_info {
> +	struct famfs_mount_opts  mount_opts;
> +	struct file             *dax_filp;
> +	struct dax_device       *dax_devp;
> +	struct bdev_handle      *bdev_handle;
> +	struct list_head         fsi_list;
> +	char                    *rootdev;
> +};
> +
> +#endif /* FAMFS_INTERNAL_H */
John Groves Feb. 26, 2024, 5:35 p.m. UTC | #2
On 24/02/26 12:48PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:52 -0600
> John Groves <John@Groves.net> wrote:
> 
> > Add the famfs_internal.h include file. This contains internal data
> > structures such as the per-file metadata structure (famfs_file_meta)
> > and extent formats.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> Hi John,
> 
> Build this up as you add the definitions in later patches.
> 
> Separate header patches just make people jump back and forth when trying
> to review.  Obviously more work to build this stuff up cleanly but
> it's worth doing to save review time.
> 

Ohhhhkaaaaay. I think you're right, just not looking forward to
all that rebasing.

> Generally I'd plumb up Kconfig and Makefile a the beginning as it means
> that the set is bisectable and we can check the logic of building each stage.
> That is harder to do but tends to bring benefits in forcing clear step
> wise approach on a patch set. Feel free to ignore this one though as it
> can slow things down.

I'm not sure that's practical. A file system needs a bunch of different
kinds of operations
- super_operations
- fs_context_operations
- inode_operations
- file_operations
- dax holder_operations, iomap_ops
- etc.

Will think about the dependency graph of these entities, but I'm not sure
it's tractable...

> 
> A few trivial comments inline.
> 
> > ---
> >  fs/famfs/famfs_internal.h | 53 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 fs/famfs/famfs_internal.h
> > 
> > diff --git a/fs/famfs/famfs_internal.h b/fs/famfs/famfs_internal.h
> > new file mode 100644
> > index 000000000000..af3990d43305
> > --- /dev/null
> > +++ b/fs/famfs/famfs_internal.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * 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_INTERNAL_H
> > +#define FAMFS_INTERNAL_H
> > +
> > +#include <linux/atomic.h>
> 
> Why?

Because fault counters are the one phased change to this file, and this
should have been with that. That may go away, but either way I'll do the
phased thing with this file.

> 
> > +#include <linux/famfs_ioctl.h>
> > +
> > +#define FAMFS_MAGIC 0x87b282ff
> > +
> > +#define FAMFS_BLKDEV_MODE (FMODE_READ|FMODE_WRITE)
> 
> Spaces around | 

Done

> 
> > +
> > +extern const struct file_operations      famfs_file_operations;
> 
> I wouldn't force alignment. It rots too often as new stuff gets added
> and doesn't really help readability much.

OK

> 
> > +
> > +/*
> > + * Each famfs dax file has this hanging from its inode->i_private.
> > + */
> > +struct famfs_file_meta {
> > +	int                   error;
> > +	enum famfs_file_type  file_type;
> > +	size_t                file_size;
> > +	enum extent_type      tfs_extent_type;
> > +	size_t                tfs_extent_ct;
> > +	struct famfs_extent   tfs_extents[];  /* flexible array */
> 
> Comment kind of obvious ;) I'd drop it.  Though we have
> magic markings for __counted_by which would be good to use from the start.

Done

> 
> 
> 
> > +};
> > +
> > +struct famfs_mount_opts {
> > +	umode_t mode;
> > +};
> > +
> > +extern const struct iomap_ops             famfs_iomap_ops;
> > +extern const struct vm_operations_struct  famfs_file_vm_ops;
> > +
> > +#define ROOTDEV_STRLEN 80
> 
> Why?  You aren't creating an array of this size here so I can't
> immediately see what the define is for.

Oversight. It was a char array but I switched to strdup() and 
failed to delete this. Gone now, thanks.

Thanks,
John
Jonathan Cameron Feb. 27, 2024, 10:28 a.m. UTC | #3
On Mon, 26 Feb 2024 11:35:17 -0600
John Groves <John@groves.net> wrote:

> On 24/02/26 12:48PM, Jonathan Cameron wrote:
> > On Fri, 23 Feb 2024 11:41:52 -0600
> > John Groves <John@Groves.net> wrote:
> >   
> > > Add the famfs_internal.h include file. This contains internal data
> > > structures such as the per-file metadata structure (famfs_file_meta)
> > > and extent formats.
> > > 
> > > Signed-off-by: John Groves <john@groves.net>  
> > Hi John,
> > 
> > Build this up as you add the definitions in later patches.
> > 
> > Separate header patches just make people jump back and forth when trying
> > to review.  Obviously more work to build this stuff up cleanly but
> > it's worth doing to save review time.
> >   
> 
> Ohhhhkaaaaay. I think you're right, just not looking forward to
> all that rebasing.

:)  Patch mangling is half the fun of upstream development :)

> 
> > Generally I'd plumb up Kconfig and Makefile a the beginning as it means
> > that the set is bisectable and we can check the logic of building each stage.
> > That is harder to do but tends to bring benefits in forcing clear step
> > wise approach on a patch set. Feel free to ignore this one though as it
> > can slow things down.  
> 
> I'm not sure that's practical. A file system needs a bunch of different
> kinds of operations
> - super_operations
> - fs_context_operations
> - inode_operations
> - file_operations
> - dax holder_operations, iomap_ops
> - etc.
> 
> Will think about the dependency graph of these entities, but I'm not sure
> it's tractable...

Sure.  There's a difference though between doing something useful (or
even successfully loading) and being able to build it at intermediate steps.
I'm only looking for buildability.

If not possible, even with a few stubs, empty ops structures etc
then fair enough.

Jonathan
Christian Brauner Feb. 27, 2024, 1:38 p.m. UTC | #4
On Fri, Feb 23, 2024 at 11:41:52AM -0600, John Groves wrote:
> Add the famfs_internal.h include file. This contains internal data
> structures such as the per-file metadata structure (famfs_file_meta)
> and extent formats.
> 
> Signed-off-by: John Groves <john@groves.net>
> ---
>  fs/famfs/famfs_internal.h | 53 +++++++++++++++++++++++++++++++++++++++

Already mentioned in another reply here but adding a bunch of types such
as famfs_file_operations that aren't even defines is pretty odd. So you
should reorder this.

>  1 file changed, 53 insertions(+)
>  create mode 100644 fs/famfs/famfs_internal.h
> 
> diff --git a/fs/famfs/famfs_internal.h b/fs/famfs/famfs_internal.h
> new file mode 100644
> index 000000000000..af3990d43305
> --- /dev/null
> +++ b/fs/famfs/famfs_internal.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 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_INTERNAL_H
> +#define FAMFS_INTERNAL_H
> +
> +#include <linux/atomic.h>
> +#include <linux/famfs_ioctl.h>
> +
> +#define FAMFS_MAGIC 0x87b282ff

That needs to go into include/uapi/linux/magic.h.

> +
> +#define FAMFS_BLKDEV_MODE (FMODE_READ|FMODE_WRITE)
> +
> +extern const struct file_operations      famfs_file_operations;
> +
> +/*
> + * Each famfs dax file has this hanging from its inode->i_private.
> + */
> +struct famfs_file_meta {
> +	int                   error;
> +	enum famfs_file_type  file_type;
> +	size_t                file_size;
> +	enum extent_type      tfs_extent_type;
> +	size_t                tfs_extent_ct;
> +	struct famfs_extent   tfs_extents[];  /* flexible array */
> +};
> +
> +struct famfs_mount_opts {
> +	umode_t mode;
> +};
> +
> +extern const struct iomap_ops             famfs_iomap_ops;
> +extern const struct vm_operations_struct  famfs_file_vm_ops;
> +
> +#define ROOTDEV_STRLEN 80
> +
> +struct famfs_fs_info {
> +	struct famfs_mount_opts  mount_opts;
> +	struct file             *dax_filp;
> +	struct dax_device       *dax_devp;
> +	struct bdev_handle      *bdev_handle;
> +	struct list_head         fsi_list;
> +	char                    *rootdev;
> +};
> +
> +#endif /* FAMFS_INTERNAL_H */
> -- 
> 2.43.0
>
John Groves Feb. 27, 2024, 2:12 p.m. UTC | #5
On 24/02/27 02:38PM, Christian Brauner wrote:
> On Fri, Feb 23, 2024 at 11:41:52AM -0600, John Groves wrote:
> > Add the famfs_internal.h include file. This contains internal data
> > structures such as the per-file metadata structure (famfs_file_meta)
> > and extent formats.
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> >  fs/famfs/famfs_internal.h | 53 +++++++++++++++++++++++++++++++++++++++
> 
> Already mentioned in another reply here but adding a bunch of types such
> as famfs_file_operations that aren't even defines is pretty odd. So you
> should reorder this.

Acknowledged, thanks. V2 will phase in only what is needed by the
code in each patch.

> 
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 fs/famfs/famfs_internal.h
> > 
> > diff --git a/fs/famfs/famfs_internal.h b/fs/famfs/famfs_internal.h
> > new file mode 100644
> > index 000000000000..af3990d43305
> > --- /dev/null
> > +++ b/fs/famfs/famfs_internal.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * 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_INTERNAL_H
> > +#define FAMFS_INTERNAL_H
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/famfs_ioctl.h>
> > +
> > +#define FAMFS_MAGIC 0x87b282ff
> 
> That needs to go into include/uapi/linux/magic.h.

Done for v2.

Thank you,
John
John Groves Feb. 28, 2024, 1:06 a.m. UTC | #6
On 24/02/27 10:28AM, Jonathan Cameron wrote:
> On Mon, 26 Feb 2024 11:35:17 -0600
> John Groves <John@groves.net> wrote:
> 
> > On 24/02/26 12:48PM, Jonathan Cameron wrote:
> > > On Fri, 23 Feb 2024 11:41:52 -0600
> > > John Groves <John@Groves.net> wrote:
> > >   
> > > > Add the famfs_internal.h include file. This contains internal data
> > > > structures such as the per-file metadata structure (famfs_file_meta)
> > > > and extent formats.
> > > > 
> > > > Signed-off-by: John Groves <john@groves.net>  
> > > Hi John,
> > > 
> > > Build this up as you add the definitions in later patches.
> > > 
> > > Separate header patches just make people jump back and forth when trying
> > > to review.  Obviously more work to build this stuff up cleanly but
> > > it's worth doing to save review time.
> > >   
> > 
> > Ohhhhkaaaaay. I think you're right, just not looking forward to
> > all that rebasing.
> 
> :)  Patch mangling is half the fun of upstream development :)
> 
> > 
> > > Generally I'd plumb up Kconfig and Makefile a the beginning as it means
> > > that the set is bisectable and we can check the logic of building each stage.
> > > That is harder to do but tends to bring benefits in forcing clear step
> > > wise approach on a patch set. Feel free to ignore this one though as it
> > > can slow things down.  
> > 
> > I'm not sure that's practical. A file system needs a bunch of different
> > kinds of operations
> > - super_operations
> > - fs_context_operations
> > - inode_operations
> > - file_operations
> > - dax holder_operations, iomap_ops
> > - etc.
> > 
> > Will think about the dependency graph of these entities, but I'm not sure
> > it's tractable...
> 
> Sure.  There's a difference though between doing something useful (or
> even successfully loading) and being able to build it at intermediate steps.
> I'm only looking for buildability.
> 
> If not possible, even with a few stubs, empty ops structures etc
> then fair enough.
> 
> Jonathan

I'm through at least the first stage of grief on this. By the time we're
through this I'll be able to reconstitute the whole bloody thing from memory,
backwards :D

John
diff mbox series

Patch

diff --git a/fs/famfs/famfs_internal.h b/fs/famfs/famfs_internal.h
new file mode 100644
index 000000000000..af3990d43305
--- /dev/null
+++ b/fs/famfs/famfs_internal.h
@@ -0,0 +1,53 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 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_INTERNAL_H
+#define FAMFS_INTERNAL_H
+
+#include <linux/atomic.h>
+#include <linux/famfs_ioctl.h>
+
+#define FAMFS_MAGIC 0x87b282ff
+
+#define FAMFS_BLKDEV_MODE (FMODE_READ|FMODE_WRITE)
+
+extern const struct file_operations      famfs_file_operations;
+
+/*
+ * Each famfs dax file has this hanging from its inode->i_private.
+ */
+struct famfs_file_meta {
+	int                   error;
+	enum famfs_file_type  file_type;
+	size_t                file_size;
+	enum extent_type      tfs_extent_type;
+	size_t                tfs_extent_ct;
+	struct famfs_extent   tfs_extents[];  /* flexible array */
+};
+
+struct famfs_mount_opts {
+	umode_t mode;
+};
+
+extern const struct iomap_ops             famfs_iomap_ops;
+extern const struct vm_operations_struct  famfs_file_vm_ops;
+
+#define ROOTDEV_STRLEN 80
+
+struct famfs_fs_info {
+	struct famfs_mount_opts  mount_opts;
+	struct file             *dax_filp;
+	struct dax_device       *dax_devp;
+	struct bdev_handle      *bdev_handle;
+	struct list_head         fsi_list;
+	char                    *rootdev;
+};
+
+#endif /* FAMFS_INTERNAL_H */