Received-SPF: pass (google.com: domain of linux-kernel+bounces-81407-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 26 Feb 2024 12:48:18 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: John Groves <John@Groves.net>
CC: John Groves <jgroves@micron.com>, Jonathan Corbet <corbet@lwn.net>, "Dan
 Williams" <dan.j.williams@intel.com>, Vishal Verma
	<vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>, "Alexander
 Viro" <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, "Jan
 Kara" <jack@suse.cz>, Matthew Wilcox <willy@infradead.org>,
	<linux-cxl@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<nvdimm@lists.linux.dev>, <john@jagalactic.com>, Dave Chinner
	<david@fromorbit.com>, Christoph Hellwig <hch@infradead.org>,
	<dave.hansen@linux.intel.com>, <gregory.price@memverge.com>
Subject: Re: [RFC PATCH 08/20] famfs: Add famfs_internal.h
Message-ID: <20240226124818.0000251d@Huawei.com>
In-Reply-To: <13556dbbd8d0f51bc31e3bdec796283fe85c6baf.1708709155.git.john@groves.net>
References: <cover.1708709155.git.john@groves.net>
	<13556dbbd8d0f51bc31e3bdec796283fe85c6baf.1708709155.git.john@groves.net>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

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 */