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

[Alison Schofield <alison.schofield@intel.com>]

On Sun, Apr 26, 2026 at 06:56:46PM -0500, John Groves wrote:
> 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.


Jumping to the part you commented on, which I think was the biggie:

> 
> 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?
> 

I do think we need to introduce "unknown" rather than keep reporting
devdax for all non-system-ram devices. With famfs added, that old
"not system-ram == devdax" shortcut just isn’t true anymore, and in the
unbound case we really don’t know if it’s devdax or famfs. I’d rather say
"unknown" than guess wrong.

That said, I don’t think we should drop to "unknown" when we actually do
know the mode. In particular, disable shouldn’t cause us to lose it. We
already report state separately, so I’d expect something like this:
	mode=devdax,  state=disabled
and not like this:
	mode=unknown, state=disabled
for a device that we knew was devdax (same idea for famfs).

Also wondering about behavior here: if a device ends up in mode="unknown",
what does enable-device do? It doesn’t take a mode, so if we’ve lost that
info across disable it’s not obvious how we pick which driver to bind.
Before famfs we kind of got away with defaulting to devdax, but that
doesn’t really work anymore.

So I think the rule should be: report a real mode when we can, and only
use "unknown" when it’s actually ambiguous. That keeps disable/enable
workflows predictable.

And if we do introduce "unknown", we need to document when it shows up,
since this is a change from the old behavior.

-- Alison

snipping here, I didn't see any questions or comments below here
expect for the done on the PATH_MAX usage.