On Wed, 7 Jan 2026 09:33:25 -0600 John Groves <[email protected]> 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 <[email protected]> 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; > +}
