Re: [PATCH 03/31] fuse: make debugging configurable at runtime

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jan 21, 2026 at 04:23:08PM -0800, Joanne Koong wrote:
> On Wed, Jan 21, 2026 at 4:02 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Wed, Jan 21, 2026 at 03:42:04PM -0800, Joanne Koong wrote:
> > > On Tue, Oct 28, 2025 at 5:45 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > Use static keys so that we can configure debugging assertions and dmesg
> > > > warnings at runtime.  By default this is turned off so the cost is
> > > > merely scanning a nop sled.  However, fuse server developers can turn
> > > > it on for their debugging systems.
> > > >
> > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > ---
> > > >  fs/fuse/fuse_i.h     |    8 +++++
> > > >  fs/fuse/iomap_i.h    |   16 ++++++++--
> > > >  fs/fuse/Kconfig      |   15 +++++++++
> > > >  fs/fuse/file_iomap.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  fs/fuse/inode.c      |    7 ++++
> > > >  5 files changed, 124 insertions(+), 3 deletions(-)
> > > >
> > > >
> > > > diff --git a/fs/fuse/file_iomap.c b/fs/fuse/file_iomap.c
> > > > index a88f5d8d2bce15..b6fc70068c5542 100644
> > > > --- a/fs/fuse/file_iomap.c
> > > > +++ b/fs/fuse/file_iomap.c
> > > > @@ -8,6 +8,12 @@
> > > >  #include "fuse_trace.h"
> > > >  #include "iomap_i.h"
> > > >
> > > > +#if IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG_DEFAULT)
> > > > +DEFINE_STATIC_KEY_TRUE(fuse_iomap_debug);
> > > > +#else
> > > > +DEFINE_STATIC_KEY_FALSE(fuse_iomap_debug);
> > > > +#endif
> > > > +
> > > >  static bool __read_mostly enable_iomap =
> > > >  #if IS_ENABLED(CONFIG_FUSE_IOMAP_BY_DEFAULT)
> > > >         true;
> > > > @@ -17,6 +23,81 @@ static bool __read_mostly enable_iomap =
> > > >  module_param(enable_iomap, bool, 0644);
> > > >  MODULE_PARM_DESC(enable_iomap, "Enable file I/O through iomap");
> > > >
> > > > +#if IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG)
> > > > +static struct kobject *iomap_kobj;
> > > > +
> > > > +static ssize_t fuse_iomap_debug_show(struct kobject *kobject,
> > > > +                                    struct kobj_attribute *a, char *buf)
> > > > +{
> > > > +       return sysfs_emit(buf, "%d\n", !!static_key_enabled(&fuse_iomap_debug));
> > > > +}
> > > > +
> > > > +static ssize_t fuse_iomap_debug_store(struct kobject *kobject,
> > > > +                                     struct kobj_attribute *a,
> > > > +                                     const char *buf, size_t count)
> > > > +{
> > > > +       int ret;
> > > > +       int val;
> > > > +
> > > > +       ret = kstrtoint(buf, 0, &val);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       if (val < 0 || val > 1)
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (val)
> > > > +               static_branch_enable(&fuse_iomap_debug);
> > > > +       else
> > > > +               static_branch_disable(&fuse_iomap_debug);
> > > > +
> > > > +       return count;
> > > > +}
> > > > +
> > > > +#define __INIT_KOBJ_ATTR(_name, _mode, _show, _store)                  \
> > > > +{                                                                      \
> > > > +       .attr   = { .name = __stringify(_name), .mode = _mode },        \
> > > > +       .show   = _show,                                                \
> > > > +       .store  = _store,                                               \
> > > > +}
> > > > +
> > > > +#define FUSE_ATTR_RW(_name, _show, _store)                     \
> > > > +       static struct kobj_attribute fuse_attr_##_name =        \
> > > > +                       __INIT_KOBJ_ATTR(_name, 0644, _show, _store)
> > > > +
> > > > +#define FUSE_ATTR_PTR(_name)                                   \
> > > > +       (&fuse_attr_##_name.attr)
> > > > +
> > > > +FUSE_ATTR_RW(debug, fuse_iomap_debug_show, fuse_iomap_debug_store);
> > > > +
> > > > +static const struct attribute *fuse_iomap_attrs[] = {
> > > > +       FUSE_ATTR_PTR(debug),
> > > > +       NULL,
> > > > +};
> > > > +
> > > > +int fuse_iomap_sysfs_init(struct kobject *fuse_kobj)
> > > > +{
> > > > +       int error;
> > > > +
> > > > +       iomap_kobj = kobject_create_and_add("iomap", fuse_kobj);
> > > > +       if (!iomap_kobj)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       error = sysfs_create_files(iomap_kobj, fuse_iomap_attrs);
> > > > +       if (error) {
> > > > +               kobject_put(iomap_kobj);
> > > > +               return error;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +void fuse_iomap_sysfs_cleanup(struct kobject *fuse_kobj)
> > > > +{
> > >
> > > Is sysfs_remove_files() also needed here?
> >
> > kobject_put is supposed to tear down the attrs that sysfs_create_files
> > attaches to iomap_kobj.  Though you're right to be suspicious -- there
> > are a lot of places that explicitly call sysfs_remove_files to undo
> > sysfs_create_files; and also a lot of places that just let kobject_put
> > do the dirty work.
> 
> Makes sense, thanks for the context.
> >
> > > > +       kobject_put(iomap_kobj);
> > > > +}
> > > > +#endif /* IS_ENABLED(CONFIG_FUSE_IOMAP_DEBUG) */
> > > > +
> > > >  bool fuse_iomap_enabled(void)
> > > >  {
> > > >         /* Don't let anyone touch iomap until the end of the patchset. */
> > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > index 1eea8dc6e723c6..eec711302a4a13 100644
> > > > --- a/fs/fuse/inode.c
> > > > +++ b/fs/fuse/inode.c
> > > > @@ -2277,8 +2277,14 @@ static int fuse_sysfs_init(void)
> > > >         if (err)
> > > >                 goto out_fuse_unregister;
> > > >
> > > > +       err = fuse_iomap_sysfs_init(fuse_kobj);
> > > > +       if (err)
> > > > +               goto out_fuse_connections;
> > > > +
> > > >         return 0;
> > > >
> > > > + out_fuse_connections:
> > > > +       sysfs_remove_mount_point(fuse_kobj, "connections");
> > > >   out_fuse_unregister:
> > > >         kobject_put(fuse_kobj);
> > > >   out_err:
> > > > @@ -2287,6 +2293,7 @@ static int fuse_sysfs_init(void)
> > > >
> > > >  static void fuse_sysfs_cleanup(void)
> > > >  {
> > > > +       fuse_iomap_sysfs_cleanup(fuse_kobj);
> > > >         sysfs_remove_mount_point(fuse_kobj, "connections");
> > > >         kobject_put(fuse_kobj);
> > > >  }
> > > >
> > > Could you explain why it's better that this goes through sysfs than
> > > through a module param?
> >
> > You can dynamically enable debugging on a production system.  I (by
> > which I really mean the support org) wishes they could do that with XFS.
> >
> > Module parameters don't come with setter functions so you can't call
> > static_branch_{enable,disable} when the parameter value updates.
> >
> 
> Ohh I thought the "module_param_cb()" stuff does let you do that and
> can be dynamically enabled/disabled as well? I mostly ask because it
> feels like it'd be nicer from a user POV if all the config stuff (eg
> enable uring, enable iomap, etc.) is in one place.

TIL today.

HAH well that's been there since 2.6.0.  Silly me, that's been there
forever.

I'll switch it to a magic module parameter that has a setter.  Much
easier than thinking about /anything/ related to sysfs.

--D

> Thanks,
> Joanne
> 
> > --D
> >
> > > Thanks,
> > > Joanne