| Message ID | 13556dbbd8d0f51bc31e3bdec796283fe85c6baf.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: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 */
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
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
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 >
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
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 --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 */
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