Re: [PATCH V4 1/2] daxctl: Add support for famfs mode

John Groves
2026-04-26T23:57:00Z

Maybe I'm overcomplicating things (it's one of the things I do),
but I'm still struggling through how to address all these issues.
Some comments inline.

On 26/02/26 06:00PM, Alison Schofield wrote:
> On Sun, Jan 18, 2026 at 10:36:38PM +0000, John Groves wrote:
> > From: John Groves <John@Groves.net>
> >
> > Putting a daxdev in famfs mode means binding it to fsdev_dax.ko
> > (drivers/dax/fsdev.c). Finding a daxdev bound to fsdev_dax means
> > it is in famfs mode.
> >
> > The test is added to the destructive test suite since it
> > modifies device modes.
>
> Make it clear that it is added in a separate patch. (and assume you
> can drop the destructive part too.)
>
> >
> > With devdax, famfs, and system-ram modes, the previous logic that assumed
> > 'not in mode X means in mode Y' needed to get slightly more complicated
> >
> > Add explicit mode detection functions:
> > - daxctl_dev_is_famfs_mode(): check if bound to fsdev_dax driver
> > - daxctl_dev_is_devdax_mode(): check if bound to device_dax driver
>
>
> The precedence check (ram->famfs->devdax->unknown) now happens in multiple
> places. How about adding a daxctl_dev_get_mode() helper to centralize that.
> It could be private for now, unless you expect external users to need it.
>
> daxctl_dev_is_famfs_mode() and _is_devdax_mode() are nearly identical aside
> from the module name. Refactoring the shared part into a single helper will
> also make it easier to add a daxctl_dev_get_mode() without duplicating the
> precedence logic.
>
> >
> > Fix mode transition logic in device.c:
> > - disable_devdax_device(): verify device is actually in devdax mode
> > - disable_famfs_device(): verify device is actually in famfs mode
> > - All reconfig_mode_*() functions now explicitly check each mode
> > - Handle unknown mode with error instead of wrong assumption
>
> Wondering about 'Fix' mode transition logic. Was prior logic broken and
> should any of these changes be in a precursor patch that is a 'fix'.
>
>
> >
> > Modify json.c to show 'unknown' if device is not in a recognized mode.
>
> I think this means disabled devices will always look unknown even when
> the intended mode is devdax or famfs, but disabled. This seems to
> change the meaning of mode from 'configured' to 'active' personality.
> Can you detect the configured mode even when disabled?
> Perhaps a man page change about this new behavior?

Good point; before famfs mode there were just 2 modes, and
not-system-ram == devdax mode is the current standard, even if no driver
is bound. At some level that's a conflation, but I'll revise and stick
with that unless you have a better idea.

Is that how you want it? No driver == devdax mode?

Any thoughts?

>
> snip
>
>
> >
> > @@ -724,11 +767,21 @@ static int reconfig_mode_system_ram(struct daxctl_dev *dev)
> > }
> >
> > if (daxctl_dev_is_enabled(dev)) {
> > - rc = disable_devdax_device(dev);
> > - if (rc < 0)
> > - return rc;
> > - if (rc > 0)
>
> Please check the return code semantics.
> This gets rid of the <0 vs >0 distinction. That means a '1' skip
> becomes an error return to the caller. Is that what you want?
>
> Previously, we had a return 1 from disable_devdax_device for
> “not applicable / already in other mode” and I think that is now
> gone.
>
>
> > + if (mem) {
> > + /* already in system-ram mode */
> > skip_enable = 1;
> > + } else if (daxctl_dev_is_famfs_mode(dev)) {
> > + rc = disable_famfs_device(dev);
> > + if (rc)
> > + return rc;
> > + } else if (daxctl_dev_is_devdax_mode(dev)) {
> > + rc = disable_devdax_device(dev);
> > + if (rc)
> > + return rc;
> > + } else {
> > + fprintf(stderr, "%s: unknown mode\n", devname);
> > + return -EINVAL;
> > + }
> > }
> >
>
> snip
>
> > static int reconfig_mode_devdax(struct daxctl_dev *dev)
> > {
> > + struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
> > + const char *devname = daxctl_dev_get_devname(dev);
> > int rc;
> >
> > if (daxctl_dev_is_enabled(dev)) {
> > - rc = disable_system_ram_device(dev);
> > - if (rc)
> > - return rc;
> > + if (mem) {
> > + rc = disable_system_ram_device(dev);
> > + if (rc)
> > + return rc;
> > + } else if (daxctl_dev_is_famfs_mode(dev)) {
> > + rc = disable_famfs_device(dev);
> > + if (rc)
> > + return rc;
> > + } else if (daxctl_dev_is_devdax_mode(dev)) {
> > + /* already in devdax mode, just re-enable */
> > + rc = daxctl_dev_disable(dev);
> > + if (rc)
>
> disable_* helpers print an error message on disable failure.
> Seems this should too.
>
>
> > + return rc;
> > + } else {
> > + fprintf(stderr, "%s: unknown mode\n", devname);
> > + return -EINVAL;
> > + }
> > }
> >
> > rc = daxctl_dev_enable_devdax(dev);
> > @@ -801,6 +870,40 @@ static int reconfig_mode_devdax(struct daxctl_dev *dev)
> > return 0;
> > }
> >
> > +static int reconfig_mode_famfs(struct daxctl_dev *dev)
> > +{
> > + struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
> > + const char *devname = daxctl_dev_get_devname(dev);
> > + int rc;
> > +
> > + if (daxctl_dev_is_enabled(dev)) {
> > + if (mem) {
> > + fprintf(stderr,
> > + "%s is in system-ram mode, must be in devdax mode to convert to famfs\n",
> > + devname);
> > + return -EINVAL;
> > + } else if (daxctl_dev_is_famfs_mode(dev)) {
> > + /* already in famfs mode, just re-enable */
> > + rc = daxctl_dev_disable(dev);
> > + if (rc)
> > + return rc;
> > + } else if (daxctl_dev_is_devdax_mode(dev)) {
> > + rc = disable_devdax_device(dev);
> > + if (rc)
>
> and here too...the disable error message.
>
>
> > + return rc;
> > + } else {
> > + fprintf(stderr, "%s: unknown mode\n", devname);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + rc = daxctl_dev_enable_famfs(dev);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
> > +}
>
> snip
>
> > +DAXCTL_EXPORT int daxctl_dev_is_famfs_mode(struct daxctl_dev *dev)
> > +{
> > + const char *devname = daxctl_dev_get_devname(dev);
> > + struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
> > + char *mod_path, *mod_base;
> > + char path[200];
>
> We have PATH_MAX for the above.

Done, thanks...

>
> > + const int len = sizeof(path);
> > +
> > + if (!device_model_is_dax_bus(dev))
> > + return false;
> > +
> > + if (!daxctl_dev_is_enabled(dev))
> > + return false;
> > +
> > + if (snprintf(path, len, "%s/driver", dev->dev_path) >= len) {
> > + err(ctx, "%s: buffer too small!\n", devname);
> > + return false;
> > + }
> > +
> > + mod_path = realpath(path, NULL);
> > + if (!mod_path)
>
> Maybe a dbg() level err msg here
>
> > + return false;
> > +
> > + mod_base = basename(mod_path);
>
> Please use path_basename() because of this:
> https://lore.kernel.org/all/20260116043056.542346-1-alison.schofield@intel.com/
>
> Give me a minute ;) to push that to the pending branch and you can
> work from there: https://github.com/pmem/ndctl/commits/pending/
>
> snip to end.

Done, thanks

Still chewing on the other stuff