diff mbox series

[RFC,09/20] famfs: Add super_operations

Message ID 537f836056c141ae093c42b9623d20de919083b1.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
Introduce the famfs superblock operations

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

Comments

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

> Introduce the famfs superblock operations
> 
> Signed-off-by: John Groves <john@groves.net>
> ---
>  fs/famfs/famfs_inode.c | 72 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 fs/famfs/famfs_inode.c
> 
> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> new file mode 100644
> index 000000000000..3329aff000d1
> --- /dev/null
> +++ b/fs/famfs/famfs_inode.c
> @@ -0,0 +1,72 @@
> +// 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.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/pagemap.h>
> +#include <linux/highmem.h>
> +#include <linux/time.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <linux/backing-dev.h>
> +#include <linux/sched.h>
> +#include <linux/parser.h>
> +#include <linux/magic.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
> +#include <linux/seq_file.h>
> +#include <linux/dax.h>
> +#include <linux/hugetlb.h>
> +#include <linux/uio.h>
> +#include <linux/iomap.h>
> +#include <linux/path.h>
> +#include <linux/namei.h>
> +#include <linux/pfn_t.h>
> +#include <linux/blkdev.h>

That's a lot of header for such a small patch.. I'm going to guess
they aren't all used - bring them in as you need them - I hope
you never need some of these!


> +
> +#include "famfs_internal.h"
> +
> +#define FAMFS_DEFAULT_MODE	0755
> +
> +static const struct super_operations famfs_ops;
> +static const struct inode_operations famfs_file_inode_operations;
> +static const struct inode_operations famfs_dir_inode_operations;

Why are these all up here?

> +
> +/**********************************************************************************
> + * famfs super_operations
> + *
> + * TODO: implement a famfs_statfs() that shows size, free and available space, etc.
> + */
> +
> +/**
> + * famfs_show_options() - Display the mount options in /proc/mounts.
Run kernel doc script + fix all warnings.

> + */
> +static int famfs_show_options(
> +	struct seq_file *m,
> +	struct dentry   *root)
Not that familiar with fs code, but this unusual kernel style. I'd go with 
something more common

static int famfs_show_options(struct seq_file *m, struct dentry *root)

> +{
> +	struct famfs_fs_info *fsi = root->d_sb->s_fs_info;
> +
> +	if (fsi->mount_opts.mode != FAMFS_DEFAULT_MODE)
> +		seq_printf(m, ",mode=%o", fsi->mount_opts.mode);
> +
> +	return 0;
> +}
> +
> +static const struct super_operations famfs_ops = {
> +	.statfs		= simple_statfs,
> +	.drop_inode	= generic_delete_inode,
> +	.show_options	= famfs_show_options,
> +};
> +
> +
One blank line probably fine.


Add the rest of the stuff a module normally has, author etc in this
patch.

> +MODULE_LICENSE("GPL");
John Groves Feb. 26, 2024, 9:47 p.m. UTC | #2
On 24/02/26 12:51PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:53 -0600
> John Groves <John@Groves.net> wrote:
> 
> > Introduce the famfs superblock operations
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> >  fs/famfs/famfs_inode.c | 72 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >  create mode 100644 fs/famfs/famfs_inode.c
> > 
> > diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> > new file mode 100644
> > index 000000000000..3329aff000d1
> > --- /dev/null
> > +++ b/fs/famfs/famfs_inode.c
> > @@ -0,0 +1,72 @@
> > +// 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.
> > + */
> > +
> > +#include <linux/fs.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/highmem.h>
> > +#include <linux/time.h>
> > +#include <linux/init.h>
> > +#include <linux/string.h>
> > +#include <linux/backing-dev.h>
> > +#include <linux/sched.h>
> > +#include <linux/parser.h>
> > +#include <linux/magic.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/fs_context.h>
> > +#include <linux/fs_parser.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/dax.h>
> > +#include <linux/hugetlb.h>
> > +#include <linux/uio.h>
> > +#include <linux/iomap.h>
> > +#include <linux/path.h>
> > +#include <linux/namei.h>
> > +#include <linux/pfn_t.h>
> > +#include <linux/blkdev.h>
> 
> That's a lot of header for such a small patch.. I'm going to guess
> they aren't all used - bring them in as you need them - I hope
> you never need some of these!

I didn't phase in headers in this series. Based on these recommendations,
the next version of this series is gonna have to be 100% constructed from
scratch, but okay. My head hurts just thinking about it. I need a nap...

I've been rebasing for 3 weeks to get this series out, and it occurs to
me that maybe there are tools I'm not aware of that make it eaiser? I'm
just typing "rebase -i..." 200 times a day. Is there a less soul-crushing way?

> 
> 
> > +
> > +#include "famfs_internal.h"
> > +
> > +#define FAMFS_DEFAULT_MODE	0755
> > +
> > +static const struct super_operations famfs_ops;
> > +static const struct inode_operations famfs_file_inode_operations;
> > +static const struct inode_operations famfs_dir_inode_operations;
> 
> Why are these all up here?

These forward declarations are needed by a later patch in the series.
They were in famfs_internal.h, but they are only used in this file, so
I moved them here.

For all answers such as this, I will hereafter reply "rebase fu", with
further clarification only if necessary.

> 
> > +
> > +/**********************************************************************************
> > + * famfs super_operations
> > + *
> > + * TODO: implement a famfs_statfs() that shows size, free and available space, etc.
> > + */
> > +
> > +/**
> > + * famfs_show_options() - Display the mount options in /proc/mounts.
> Run kernel doc script + fix all warnings.

Will do; I actually think I have already fixed those...

> 
> > + */
> > +static int famfs_show_options(
> > +	struct seq_file *m,
> > +	struct dentry   *root)
> Not that familiar with fs code, but this unusual kernel style. I'd go with 
> something more common
> 
> static int famfs_show_options(struct seq_file *m, struct dentry *root)

Done. To all functions...

> 
> > +{
> > +	struct famfs_fs_info *fsi = root->d_sb->s_fs_info;
> > +
> > +	if (fsi->mount_opts.mode != FAMFS_DEFAULT_MODE)
> > +		seq_printf(m, ",mode=%o", fsi->mount_opts.mode);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct super_operations famfs_ops = {
> > +	.statfs		= simple_statfs,
> > +	.drop_inode	= generic_delete_inode,
> > +	.show_options	= famfs_show_options,
> > +};
> > +
> > +
> One blank line probably fine.

Done

> 
> 
> Add the rest of the stuff a module normally has, author etc in this
> patch.

Because "rebase fu" I'm not sure the order will remain the same. Will
try not to make anybody tell me this again though...

John
Jonathan Cameron Feb. 27, 2024, 10:34 a.m. UTC | #3
On Mon, 26 Feb 2024 15:47:53 -0600
John Groves <John@groves.net> wrote:

> On 24/02/26 12:51PM, Jonathan Cameron wrote:
> > On Fri, 23 Feb 2024 11:41:53 -0600
> > John Groves <John@Groves.net> wrote:
> >   
> > > Introduce the famfs superblock operations
> > > 
> > > Signed-off-by: John Groves <john@groves.net>
> > > ---
> > >  fs/famfs/famfs_inode.c | 72 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > >  create mode 100644 fs/famfs/famfs_inode.c
> > > 
> > > diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> > > new file mode 100644
> > > index 000000000000..3329aff000d1
> > > --- /dev/null
> > > +++ b/fs/famfs/famfs_inode.c
> > > @@ -0,0 +1,72 @@
> > > +// 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.
> > > + */
> > > +
> > > +#include <linux/fs.h>
> > > +#include <linux/pagemap.h>
> > > +#include <linux/highmem.h>
> > > +#include <linux/time.h>
> > > +#include <linux/init.h>
> > > +#include <linux/string.h>
> > > +#include <linux/backing-dev.h>
> > > +#include <linux/sched.h>
> > > +#include <linux/parser.h>
> > > +#include <linux/magic.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/fs_context.h>
> > > +#include <linux/fs_parser.h>
> > > +#include <linux/seq_file.h>
> > > +#include <linux/dax.h>
> > > +#include <linux/hugetlb.h>
> > > +#include <linux/uio.h>
> > > +#include <linux/iomap.h>
> > > +#include <linux/path.h>
> > > +#include <linux/namei.h>
> > > +#include <linux/pfn_t.h>
> > > +#include <linux/blkdev.h>  
> > 
> > That's a lot of header for such a small patch.. I'm going to guess
> > they aren't all used - bring them in as you need them - I hope
> > you never need some of these!  
> 
> I didn't phase in headers in this series. Based on these recommendations,
> the next version of this series is gonna have to be 100% constructed from
> scratch, but okay. My head hurts just thinking about it. I need a nap...
> 
> I've been rebasing for 3 weeks to get this series out, and it occurs to
> me that maybe there are tools I'm not aware of that make it eaiser? I'm
> just typing "rebase -i..." 200 times a day. Is there a less soul-crushing way?

Hmm. There are things that make it easier to pick and chose parts of a
big diff for different patches.  Some combination of 
git reset HEAD~1
and one of the 'graphical' tools like tig that let you pick lines.

That lets you quickly break up a patch where you want to move things, then
you can reorder the patches to put them next to where you want to move
changes to and rely on git rebase -i with f or s to squash them.

Figuring out optimum path to the eventual break up you want is
a skill though.  When doing this sort of mangling I tend to get it wrong
and shout at my computer a few times a day ;)
Then git rebase --abort and try again.

End result is that you end up with coherent series and it looks like
you wrote perfect code in nice steps from the start!

Jonathan
John Groves Feb. 27, 2024, 5:48 p.m. UTC | #4
On 24/02/26 12:51PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:53 -0600
> John Groves <John@Groves.net> wrote:
> > + */
> > +static int famfs_show_options(
> > +	struct seq_file *m,
> > +	struct dentry   *root)
> Not that familiar with fs code, but this unusual kernel style. I'd go with 
> something more common
> 
> static int famfs_show_options(struct seq_file *m, struct dentry *root)

Actually, xfs does function declarations and prototypes this way, not sure if
it's everywhere. But I like this format because changing one argument usually
doesn't put un-changed args into the diff.

So I may keep this style after all.

John
diff mbox series

Patch

diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
new file mode 100644
index 000000000000..3329aff000d1
--- /dev/null
+++ b/fs/famfs/famfs_inode.c
@@ -0,0 +1,72 @@ 
+// 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.
+ */
+
+#include <linux/fs.h>
+#include <linux/pagemap.h>
+#include <linux/highmem.h>
+#include <linux/time.h>
+#include <linux/init.h>
+#include <linux/string.h>
+#include <linux/backing-dev.h>
+#include <linux/sched.h>
+#include <linux/parser.h>
+#include <linux/magic.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
+#include <linux/seq_file.h>
+#include <linux/dax.h>
+#include <linux/hugetlb.h>
+#include <linux/uio.h>
+#include <linux/iomap.h>
+#include <linux/path.h>
+#include <linux/namei.h>
+#include <linux/pfn_t.h>
+#include <linux/blkdev.h>
+
+#include "famfs_internal.h"
+
+#define FAMFS_DEFAULT_MODE	0755
+
+static const struct super_operations famfs_ops;
+static const struct inode_operations famfs_file_inode_operations;
+static const struct inode_operations famfs_dir_inode_operations;
+
+/**********************************************************************************
+ * famfs super_operations
+ *
+ * TODO: implement a famfs_statfs() that shows size, free and available space, etc.
+ */
+
+/**
+ * famfs_show_options() - Display the mount options in /proc/mounts.
+ */
+static int famfs_show_options(
+	struct seq_file *m,
+	struct dentry   *root)
+{
+	struct famfs_fs_info *fsi = root->d_sb->s_fs_info;
+
+	if (fsi->mount_opts.mode != FAMFS_DEFAULT_MODE)
+		seq_printf(m, ",mode=%o", fsi->mount_opts.mode);
+
+	return 0;
+}
+
+static const struct super_operations famfs_ops = {
+	.statfs		= simple_statfs,
+	.drop_inode	= generic_delete_inode,
+	.show_options	= famfs_show_options,
+};
+
+
+MODULE_LICENSE("GPL");