Message-ID: <2vkgyxe3mnyamj33axiwthmqo32akdakfgv3vfauziakjnzqtj@vr3erk5wdshq>
Date: Fri, 11 Jul 2025 08:46:54 -0500
From: John Groves <John@...ves.net>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: Dan Williams <dan.j.williams@...el.com>, 
	Miklos Szeredi <miklos@...redb.hu>, Bernd Schubert <bschubert@....com>, 
	John Groves <jgroves@...ron.com>, Jonathan Corbet <corbet@....net>, 
	Vishal Verma <vishal.l.verma@...el.com>, Dave Jiang <dave.jiang@...el.com>, 
	Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, 
	Randy Dunlap <rdunlap@...radead.org>, Jeff Layton <jlayton@...nel.org>, 
	Kent Overstreet <kent.overstreet@...ux.dev>, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	nvdimm@...ts.linux.dev, linux-cxl@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	Amir Goldstein <amir73il@...il.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
	Stefan Hajnoczi <shajnocz@...hat.com>, Joanne Koong <joannelkoong@...il.com>, 
	Josef Bacik <josef@...icpanda.com>, Aravind Ramesh <arramesh@...ron.com>, 
	Ajay Joshi <ajayjoshi@...ron.com>
Subject: Re: [RFC V2 12/18] famfs_fuse: Plumb the GET_FMAP message/response

On 25/07/08 09:27PM, Darrick J. Wong wrote:
> On Thu, Jul 03, 2025 at 01:50:26PM -0500, John Groves wrote:
> > Upon completion of an OPEN, if we're in famfs-mode we do a GET_FMAP to
> > retrieve and cache up the file-to-dax map in the kernel. If this
> > succeeds, read/write/mmap are resolved direct-to-dax with no upcalls.
> > 
> > GET_FMAP has a variable-size response payload, and the allocated size
> > is sent in the in_args[0].size field. If the fmap would overflow the
> > message, the fuse server sends a reply of size 'sizeof(uint32_t)' which
> > specifies the size of the fmap message. Then the kernel can realloc a
> > large enough buffer and try again.
> > 
> > Signed-off-by: John Groves <john@...ves.net>
> > ---
> >  fs/fuse/file.c            | 84 +++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/fuse_i.h          | 36 ++++++++++++++++-
> >  fs/fuse/inode.c           | 19 +++++++--
> >  fs/fuse/iomode.c          |  2 +-
> >  include/uapi/linux/fuse.h | 18 +++++++++
> >  5 files changed, 154 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 93b82660f0c8..8616fb0a6d61 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -230,6 +230,77 @@ static void fuse_truncate_update_attr(struct inode *inode, struct file *file)
> >  	fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE);
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +
> > +#define FMAP_BUFSIZE 4096
> 
> PAGE_SIZE ?

Like it. Queued to -next

> 
> > +
> > +static int
> > +fuse_get_fmap(struct fuse_mount *fm, struct inode *inode, u64 nodeid)
> > +{
> > +	struct fuse_get_fmap_in inarg = { 0 };
> > +	size_t fmap_bufsize = FMAP_BUFSIZE;
> > +	ssize_t fmap_size;
> > +	int retries = 1;
> > +	void *fmap_buf;
> > +	int rc;
> > +
> > +	FUSE_ARGS(args);
> > +
> > +	fmap_buf = kcalloc(1, FMAP_BUFSIZE, GFP_KERNEL);
> > +	if (!fmap_buf)
> > +		return -EIO;
> > +
> > + retry_once:
> > +	inarg.size = fmap_bufsize;
> > +
> > +	args.opcode = FUSE_GET_FMAP;
> > +	args.nodeid = nodeid;
> > +
> > +	args.in_numargs = 1;
> > +	args.in_args[0].size = sizeof(inarg);
> > +	args.in_args[0].value = &inarg;
> > +
> > +	/* Variable-sized output buffer
> > +	 * this causes fuse_simple_request() to return the size of the
> > +	 * output payload
> > +	 */
> > +	args.out_argvar = true;
> > +	args.out_numargs = 1;
> > +	args.out_args[0].size = fmap_bufsize;
> > +	args.out_args[0].value = fmap_buf;
> > +
> > +	/* Send GET_FMAP command */
> > +	rc = fuse_simple_request(fm, &args);
> > +	if (rc < 0) {
> > +		pr_err("%s: err=%d from fuse_simple_request()\n",
> > +		       __func__, rc);
> > +		return rc;
> > +	}
> > +	fmap_size = rc;
> > +
> > +	if (retries && fmap_size == sizeof(uint32_t)) {
> > +		/* fmap size exceeded fmap_bufsize;
> > +		 * actual fmap size returned in fmap_buf;
> > +		 * realloc and retry once
> > +		 */
> > +		fmap_bufsize = *((uint32_t *)fmap_buf);
> > +
> > +		--retries;
> > +		kfree(fmap_buf);
> > +		fmap_buf = kcalloc(1, fmap_bufsize, GFP_KERNEL);
> > +		if (!fmap_buf)
> > +			return -EIO;
> > +
> > +		goto retry_once;
> > +	}
> > +
> > +	/* Will call famfs_file_init_dax() when that gets added */
> 
> Hard to say what this does without looking further down in the patchset.
> :)

New comment:
	/* We retrieved the "fmap" (the file's map to memory), but
	 * we haven't used it yet. A call to famfs_file_init_dax() will be added
	 * here in a subsequent patch, when we add the ability to attach
	 * fmaps to files.
	 */

> 
> > +	kfree(fmap_buf);
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static int fuse_open(struct inode *inode, struct file *file)
> >  {
> >  	struct fuse_mount *fm = get_fuse_mount(inode);
> > @@ -263,6 +334,19 @@ static int fuse_open(struct inode *inode, struct file *file)
> >  
> >  	err = fuse_do_open(fm, get_node_id(inode), file, false);
> >  	if (!err) {
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +		if (fm->fc->famfs_iomap) {
> > +			if (S_ISREG(inode->i_mode)) {
> 
> /me wonders if you want to turn this into a dumb helper to reduce the
> indenting levels?
> 
> #if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> static inline bool fuse_is_famfs_file(struct inode *inode)
> {
> 	return fm->fc->famfs_iomap && S_ISREG(inode->i_mode);
> }
> #else
> # define fuse_is_famfs_file(...)	(false)
> #endif
> 
> 	if (!err) {
> 		if (fuse_is_famfs_file(inode)) {
> 			rc = fuse_get_fmap(fm, inode);
> 			...
> 		}
> 	}
> 

I've already refactored helpers and simplified this logic in the -next 
branch, including losing the conditrional code here in file.c:

	if (!err) {
		if ((fm->fc->famfs_iomap) && (S_ISREG(inode->i_mode))) {
			int rc;
			/* Get the famfs fmap */
			rc = fuse_get_fmap(fm, inode);
			...
		}
		...
	}

So I think it's quite a bit cleaner... will send out an updated patch
pretty soon (probably next week, without the poisoned page fixes yet).

> > +				int rc;
> > +				/* Get the famfs fmap */
> > +				rc = fuse_get_fmap(fm, inode,
> > +						   get_node_id(inode));
> 
> Just get_node_id inside fuse_get_fmap to reduce the parameter count.

Done, thanks

> 
> > +				if (rc)
> > +					pr_err("%s: fuse_get_fmap err=%d\n",
> > +					       __func__, rc);
> > +			}
> > +		}
> > +#endif
> >  		ff = file->private_data;
> >  		err = fuse_finish_open(inode, file);
> >  		if (err)
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index f4ee61046578..e01d6e5c6e93 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -193,6 +193,10 @@ struct fuse_inode {
> >  	/** Reference to backing file in passthrough mode */
> >  	struct fuse_backing *fb;
> >  #endif
> > +
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +	void *famfs_meta;
> > +#endif
> 
> What gets stored in here?

Explanatory comment added:
	/* Pointer to the file's famfs metadata. Primary content is the
	 * in-memory version of the fmap - the map from file's offset range
	 * to DAX memory
	 */

> 
> >  };
> >  
> >  /** FUSE inode state bits */
> > @@ -945,6 +949,8 @@ struct fuse_conn {
> >  #endif
> >  
> >  #if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +	struct rw_semaphore famfs_devlist_sem;
> > +	struct famfs_dax_devlist *dax_devlist;
> >  	char *shadow;
> >  #endif
> >  };
> > @@ -1435,11 +1441,14 @@ void fuse_free_conn(struct fuse_conn *fc);
> >  
> >  /* dax.c */
> >  
> > +static inline int fuse_file_famfs(struct fuse_inode *fi); /* forward */
> > +
> >  /* This macro is used by virtio_fs, but now it also needs to filter for
> >   * "not famfs"
> >   */
> >  #define FUSE_IS_VIRTIO_DAX(fuse_inode) (IS_ENABLED(CONFIG_FUSE_DAX)	\
> > -					&& IS_DAX(&fuse_inode->inode))
> > +					&& IS_DAX(&fuse_inode->inode)	\
> > +					&& !fuse_file_famfs(fuse_inode))
> >  
> >  ssize_t fuse_dax_read_iter(struct kiocb *iocb, struct iov_iter *to);
> >  ssize_t fuse_dax_write_iter(struct kiocb *iocb, struct iov_iter *from);
> > @@ -1550,4 +1559,29 @@ extern void fuse_sysctl_unregister(void);
> >  #define fuse_sysctl_unregister()	do { } while (0)
> >  #endif /* CONFIG_SYSCTL */
> >  
> > +/* famfs.c */
> > +static inline struct fuse_backing *famfs_meta_set(struct fuse_inode *fi,
> > +						       void *meta)
> > +{
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +	return xchg(&fi->famfs_meta, meta);
> > +#else
> > +	return NULL;
> > +#endif
> > +}
> > +
> > +static inline void famfs_meta_free(struct fuse_inode *fi)
> > +{
> > +	/* Stub wil be connected in a subsequent commit */
> > +}
> > +
> > +static inline int fuse_file_famfs(struct fuse_inode *fi)
> > +{
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +	return (READ_ONCE(fi->famfs_meta) != NULL);
> > +#else
> > +	return 0;
> > +#endif
> > +}
> 
> ...or maybe this is the predicate you want to see if you really need to
> fmapping related stuff?
> 
> > +
> >  #endif /* _FS_FUSE_I_H */
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index a7e1cf8257b0..b071d16f7d04 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -117,6 +117,9 @@ static struct inode *fuse_alloc_inode(struct super_block *sb)
> >  	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> >  		fuse_inode_backing_set(fi, NULL);
> >  
> > +	if (IS_ENABLED(CONFIG_FUSE_FAMFS_DAX))
> > +		famfs_meta_set(fi, NULL);
> > +
> >  	return &fi->inode;
> >  
> >  out_free_forget:
> > @@ -138,6 +141,13 @@ static void fuse_free_inode(struct inode *inode)
> >  	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> >  		fuse_backing_put(fuse_inode_backing(fi));
> >  
> > +#if IS_ENABLED(CONFIG_FUSE_FAMFS_DAX)
> > +	if (S_ISREG(inode->i_mode) && fi->famfs_meta) {
> > +		famfs_meta_free(fi);
> > +		famfs_meta_set(fi, NULL);
> 
> _free should null out the pointer, no?

Good point - will do

<snip>

Thanks Darrick!
John

Powered by blists - more mailing lists