Received-SPF: pass (google.com: domain of linux-kernel+bounces-82119-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Sender: John Groves <grovesaustin@gmail.com>
Date: Mon, 26 Feb 2024 12:04:05 -0600
From: John Groves <John@groves.net>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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 07/20] famfs: Add include/linux/famfs_ioctl.h
Message-ID: <4v6ug44nrtk2tqvio2teglsm4auhdvocgyyggtlwc3xkv7b6zw@ntw24jye7omz>
References: <cover.1708709155.git.john@groves.net>
 <b40ca30e4bf689249a8c237909d9a7aaca9861e4.1708709155.git.john@groves.net>
 <20240226123940.0000692c@Huawei.com>
 <z3fx5uiv6uu4sawvxrhfvx42qetchmq4ozxhq2huwg2rrcyk5c@odbiisdhop2m>
 <20240226165639.000025c6@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226165639.000025c6@Huawei.com>

On 24/02/26 04:56PM, Jonathan Cameron wrote:
> On Mon, 26 Feb 2024 10:44:43 -0600
> John Groves <John@groves.net> wrote:
> 
> > On 24/02/26 12:39PM, Jonathan Cameron wrote:
> > > On Fri, 23 Feb 2024 11:41:51 -0600
> > > John Groves <John@Groves.net> wrote:
> > >   
> > > > Add uapi include file for famfs. The famfs user space uses ioctl on
> > > > individual files to pass in mapping information and file size. This
> > > > would be hard to do via sysfs or other means, since it's
> > > > file-specific.
> > > > 
> > > > Signed-off-by: John Groves <john@groves.net>
> > > > ---
> > > >  include/uapi/linux/famfs_ioctl.h | 56 ++++++++++++++++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 include/uapi/linux/famfs_ioctl.h
> > > > 
> > > > diff --git a/include/uapi/linux/famfs_ioctl.h b/include/uapi/linux/famfs_ioctl.h
> > > > new file mode 100644
> > > > index 000000000000..6b3e6452d02f
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/famfs_ioctl.h
> > > > @@ -0,0 +1,56 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > +/*
> > > > + * 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_IOCTL_H
> > > > +#define FAMFS_IOCTL_H
> > > > +
> > > > +#include <linux/ioctl.h>
> > > > +#include <linux/uuid.h>
> > > > +
> > > > +#define FAMFS_MAX_EXTENTS 2  
> > > Why 2?  
> > 
> > You catch everything! 
> > 
> > This limit is in place to avoid supporting somethign we're not testing. It
> > will probably be raised later.
> > 
> > Currently user space doesn't support deleting files, which makes it easy
> > to ignore whether any clients have a stale view of metadata. If there is
> > no delete, there's actually no reason to have more than 1 extent.
> Then have 1. + a Comment on why it is 1.

Actually we test the 2 case. That seemed important to testing ioctl and
famfs_meta_to_dax_offset(). It just doesn't yet happen in the wild. Will
clarify with a comment.

> > 
> > > > +
> > > > +enum extent_type {
> > > > +	SIMPLE_DAX_EXTENT = 13,  
> > > 
> > > Comment on this would be good to have  
> > 
> > Done. Basically we anticipate there being other types of extents in the
> > future.
> 
> I was more curious about the 13!

I think I was just being feisty that day. Will drop that...

> 
> > 
> > >   
> > > > +	INVALID_EXTENT_TYPE,
> > > > +};
> > > > +
> > > > +struct famfs_extent {
> > > > +	__u64              offset;
> > > > +	__u64              len;
> > > > +};
> > > > +
> > > > +enum famfs_file_type {
> > > > +	FAMFS_REG,
> > > > +	FAMFS_SUPERBLOCK,
> > > > +	FAMFS_LOG,
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct famfs_ioc_map
> > > > + *
> > > > + * This is the metadata that indicates where the memory is for a famfs file
> > > > + */
> > > > +struct famfs_ioc_map {
> > > > +	enum extent_type          extent_type;
> > > > +	enum famfs_file_type      file_type;  
> > > 
> > > These are going to be potentially varying in size depending on arch, compiler
> > > settings etc.  Been a while, but I though best practice for uapi was always
> > > fixed size elements even though we lose the typing.  
> > 
> > I might not be following you fully here. User space is running the same
> > arch as kernel, so an enum can't be a different size, right? It could be
> > a different size on different arches, but this is just between user/kernel.
> 
> I can't remember why, but this has bitten me in the past.
> Ah, should have known Daniel would have written something on it ;)
> https://www.kernel.org/doc/html/next/process/botching-up-ioctls.html
> 
> It's the fun of need for compat ioctls with 32bit userspace on 64bit kernels.
> 
> The alignment one is key as well. That bit me more than once due to
> 32bit x86 aligning 64 bit integers at 32 bits.
> 
> We could just not support these cases but it's easy to get right so why
> bother with complexity of ruling them out.

Makes sense. Will do.

> 
> > 
> > I initially thought of XDR for on-media-format, which file systems need
> > to do with on-media structs (superblocks, logs, inodes, etc. etc.). But
> > this struct is not used in that way.
> > 
> > In fact, famfs' on-media/in-memory metadata (superblock, log, log entries)
> > is only ever read read and written by user space - so it's the user space
> > code that needs XDR on-media-format handling.
> > 
> > So to clarify - do you think those enums should be u32 or the like?
> 
> Yes. As it's userspace, uint32_t maybe or __u32. I 'think'
> both are acceptable in uapi headers these days.

Roger that.

Thanks,
John