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

[John Groves <John@groves.net>]

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