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

[Alison Schofield]

On Thu, Feb 26, 2026 at 06:00:41PM -0800, Alison Schofield wrote:
> On Sun, Jan 18, 2026 at 10:36:38PM +0000, John Groves wrote:
> > From: John Groves <John@xxxxxxxxxx>
> > 

Hi John,

This is where I left off with the actual changes to "daxctl" for FAMFS.
We need a new rev of this ndctl set that includes both patches rebased
on ndctl pending and addressing the review comments below for daxctl.
(Although I've used more recent branches, I haven't looked at whether
these issues were addressed in the code.)

With a new rev, I'll take another look at ensuring a dax device is
available for the unit test.

Thanks!

--Alison


> > 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?
> 
> 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.
> 
> > +	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@xxxxxxxxx/
> 
> 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.