| Message ID | 537f836056c141ae093c42b9623d20de919083b1.1708709155.git.john@groves.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce the famfs shared-memory file system | expand |
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");
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
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
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 --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");
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