Message-ID: <20260108144502.000024e0@huawei.com>
Date: Thu, 8 Jan 2026 14:45:02 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: John Groves <John@...ves.net>
CC: Miklos Szeredi <miklos@...redi.hu>, Dan Williams
	<dan.j.williams@...el.com>, Bernd Schubert <bschubert@....com>, "Alison
 Schofield" <alison.schofield@...el.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>, "David
 Hildenbrand" <david@...nel.org>, Christian Brauner <brauner@...nel.org>,
	"Darrick J . Wong" <djwong@...nel.org>, Randy Dunlap <rdunlap@...radead.org>,
	Jeff Layton <jlayton@...nel.org>, Amir Goldstein <amir73il@...il.com>, Stefan
 Hajnoczi <shajnocz@...hat.com>, Joanne Koong <joannelkoong@...il.com>, Josef
 Bacik <josef@...icpanda.com>, Bagas Sanjaya <bagasdotme@...il.com>, Chen
 Linxuan <chenlinxuan@...ontech.com>, "James Morse" <james.morse@....com>,
	Fuad Tabba <tabba@...gle.com>, "Sean Christopherson" <seanjc@...gle.com>,
	Shivank Garg <shivankg@....com>, Ackerley Tng <ackerleytng@...gle.com>,
	Gregory Price <gourry@...rry.net>, Aravind Ramesh <arramesh@...ron.com>, Ajay
 Joshi <ajayjoshi@...ron.com>, <venkataravis@...ron.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-cxl@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH V3 16/21] famfs_fuse: GET_DAXDEV message and
 daxdev_table

On Wed,  7 Jan 2026 09:33:25 -0600
John Groves <John@...ves.net> wrote:

> * The new GET_DAXDEV message/response is added
> * The famfs.c:famfs_teardown() function is added as a primary teardown
>   function for famfs.
> * The command it triggered by the update_daxdev_table() call, if there
>   are any daxdevs in the subject fmap that are not represented in the
>   daxdev_table yet.
> * fs/namei.c: export may_open_dev()
> 
> Signed-off-by: John Groves <john@...ves.net>
Hi John,

A few things inline

Thanks,

Jonathan

> ---
>  fs/fuse/famfs.c           | 236 ++++++++++++++++++++++++++++++++++++++
>  fs/fuse/famfs_kfmap.h     |  26 +++++
>  fs/fuse/fuse_i.h          |  13 ++-
>  fs/fuse/inode.c           |   4 +-
>  fs/namei.c                |   1 +
>  include/uapi/linux/fuse.h |  20 ++++
>  6 files changed, 298 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index 2aabd1d589fd..b5cd1b5c1d6c 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
> @@ -20,6 +20,239 @@
>  #include "famfs_kfmap.h"
>  #include "fuse_i.h"
>  
> +/*
> + * famfs_teardown()
> + *
> + * Deallocate famfs metadata for a fuse_conn
> + */
> +void
> +famfs_teardown(struct fuse_conn *fc)
> +{
> +	struct famfs_dax_devlist *devlist = fc->dax_devlist;
> +	int i;
> +
> +	kfree(fc->shadow);
> +
> +	fc->dax_devlist = NULL;
> +
> +	if (!devlist)
> +		return;
> +
> +	if (!devlist->devlist)

I'm going to assume that if this is true, devlist->nslots == 0?
If so I'd skip this check and just let the rest of the code happen.

> +		goto out;
> +
> +	/* Close & release all the daxdevs in our table */
> +	for (i = 0; i < devlist->nslots; i++) {
> +		struct famfs_daxdev *dd = &devlist->devlist[i];
> +
> +		if (!dd->valid)
> +			continue;
> +
> +		/* Release reference from dax_dev_get() */
> +		if (dd->devp)
> +			put_dax(dd->devp);
> +
> +		kfree(dd->name);
> +	}
> +	kfree(devlist->devlist);
> +
> +out:
> +	kfree(devlist);
> +}

> +/**
> + * famfs_fuse_get_daxdev() - Retrieve info for a DAX device from fuse server
> + *
> + * Send a GET_DAXDEV message to the fuse server to retrieve info on a
> + * dax device.
> + *
> + * @fm:     fuse_mount
> + * @index:  the index of the dax device; daxdevs are referred to by index
> + *          in fmaps, and the server resolves the index to a particular daxdev
> + *
> + * Returns: 0=success
> + *          -errno=failure
> + */
> +static int
> +famfs_fuse_get_daxdev(struct fuse_mount *fm, const u64 index)
> +{
> +	struct fuse_daxdev_out daxdev_out = { 0 };
> +	struct fuse_conn *fc = fm->fc;
> +	struct famfs_daxdev *daxdev;
> +	int err = 0;
Always set before use so no need to init.

> +
> +	FUSE_ARGS(args);
> +
> +	/* Store the daxdev in our table */
> +	if (index >= fc->dax_devlist->nslots) {
> +		pr_err("%s: index(%lld) > nslots(%d)\n",
> +		       __func__, index, fc->dax_devlist->nslots);
> +		err = -EINVAL;
> +		goto out;

I'd return here as nothing to do.

> +	}
> +
> +	args.opcode = FUSE_GET_DAXDEV;
> +	args.nodeid = index;
> +
> +	args.in_numargs = 0;
> +
> +	args.out_numargs = 1;
> +	args.out_args[0].size = sizeof(daxdev_out);
> +	args.out_args[0].value = &daxdev_out;
> +
> +	/* Send GET_DAXDEV command */
> +	err = fuse_simple_request(fm, &args);
> +	if (err) {
> +		pr_err("%s: err=%d from fuse_simple_request()\n",
> +		       __func__, err);
> +		/*

I'm not sure what local comment style is, but be consistent of
whether there is a blank line or not.

> +		 * Error will be that the payload is smaller than FMAP_BUFSIZE,
> +		 * which is the max we can handle. Empty payload handled below.
> +		 */
> +		goto out;
return here is probably simpler.

> +	}
> +
> +	down_write(&fc->famfs_devlist_sem);
> +
> +	daxdev = &fc->dax_devlist->devlist[index];
> +
> +	/* Abort if daxdev is now valid (race - another thread got it first) */
> +	if (daxdev->valid) {
> +		up_write(&fc->famfs_devlist_sem);
> +		/* We already have a valid entry at this index */
> +		pr_debug("%s: daxdev already known\n", __func__);
> +		goto out;
> +	}
> +
> +	/* Verify that the dev is valid and can be opened and gets the devno */
> +	err = famfs_verify_daxdev(daxdev_out.name, &daxdev->devno);
> +	if (err) {
> +		up_write(&fc->famfs_devlist_sem);
> +		pr_err("%s: err=%d from famfs_verify_daxdev()\n", __func__, err);
> +		goto out;
> +	}
> +
> +	/* This will fail if it's not a dax device */
> +	daxdev->devp = dax_dev_get(daxdev->devno);
> +	if (!daxdev->devp) {
> +		up_write(&fc->famfs_devlist_sem);

Move the label before the up_write, so you don't need to do it in each
error case or use a guard()

> +		pr_warn("%s: device %s not found or not dax\n",
> +			__func__, daxdev_out.name);
> +		err = -ENODEV;
> +		goto out;
> +	}
> +
> +	daxdev->name = kstrdup(daxdev_out.name, GFP_KERNEL);
Can fail.

> +	wmb(); /* all daxdev fields must be visible before marking it valid */
> +	daxdev->valid = 1;
> +
> +	up_write(&fc->famfs_devlist_sem);
> +
> +out:
> +	return err;
> +}
> +
> +/**
> + * famfs_update_daxdev_table() - Update the daxdev table
> + * @fm   - fuse_mount
> + * @meta - famfs_file_meta, in-memory format, built from a GET_FMAP response
> + *
> + * This function is called for each new file fmap, to verify whether all
> + * referenced daxdevs are already known (i.e. in the table). Any daxdev
> + * indices referenced in @meta but not in the table will be retrieved via
> + * famfs_fuse_get_daxdev() and added to the table
> + *
> + * Return: 0=success
> + *         -errno=failure
> + */
> +static int
> +famfs_update_daxdev_table(
> +	struct fuse_mount *fm,
> +	const struct famfs_file_meta *meta)
> +{
> +	struct famfs_dax_devlist *local_devlist;
> +	struct fuse_conn *fc = fm->fc;
> +	int err;
> +	int i;

Might as well put those on one line or move i down to the loop init.

> +
> +	/* First time through we will need to allocate the dax_devlist */
> +	if (unlikely(!fc->dax_devlist)) {

I'd avoid unlikely markings unless you have good evidence they are needed.
Let the branch predictors figure it out.

> +		local_devlist = kcalloc(1, sizeof(*fc->dax_devlist), GFP_KERNEL);
> +		if (!local_devlist)
> +			return -ENOMEM;
> +
> +		local_devlist->nslots = MAX_DAXDEVS;
> +
> +		local_devlist->devlist = kcalloc(MAX_DAXDEVS,
> +						 sizeof(struct famfs_daxdev),
> +						 GFP_KERNEL);
> +		if (!local_devlist->devlist) {
> +			kfree(local_devlist);
> +			return -ENOMEM;
> +		}
> +
> +		/* We don't need famfs_devlist_sem here because we use cmpxchg */
> +		if (cmpxchg(&fc->dax_devlist, NULL, local_devlist) != NULL) {
> +			kfree(local_devlist->devlist);
> +			kfree(local_devlist); /* another thread beat us to it */
> +		}
> +	}
> +
> +	down_read(&fc->famfs_devlist_sem);
> +	for (i = 0; i < fc->dax_devlist->nslots; i++) {
> +		if (!(meta->dev_bitmap & (1ULL << i)))

Could you do for_each_set_bit() on that bitmap?
Might end up clearer.

> +			continue;
> +
> +		/* This file meta struct references devindex i
> +		 * if devindex i isn't in the table; get it...
> +		 */
> +		if (!(fc->dax_devlist->devlist[i].valid)) {

Maybe flip logic and do a continue as you do with the condition above.

> +			up_read(&fc->famfs_devlist_sem);
> +
> +			err = famfs_fuse_get_daxdev(fm, i);
> +			if (err)
> +				pr_err("%s: failed to get daxdev=%d\n",
> +				       __func__, i);
> +
> +			down_read(&fc->famfs_devlist_sem);
> +		}
> +	}
> +	up_read(&fc->famfs_devlist_sem);
> +
> +	return 0;
> +}

Powered by blists - more mailing lists