Message-ID: <20260122222233.GA5900@frogsfrogsfrogs>
Date: Thu, 22 Jan 2026 14:22:33 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Joanne Koong <joannelkoong@...il.com>
Cc: miklos@...redi.hu, bernd@...ernd.com, neal@...pa.dev,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 07/31] fuse: create a per-inode flag for toggling iomap

On Wed, Jan 21, 2026 at 05:13:39PM -0800, Joanne Koong wrote:
> On Tue, Oct 28, 2025 at 5:46 PM Darrick J. Wong <djwong@...nel.org> wrote:
> >
> > From: Darrick J. Wong <djwong@...nel.org>
> >
> > Create a per-inode flag to control whether or not this inode actually
> > uses iomap.  This is required for non-regular files because iomap
> > doesn't apply there; and enables fuse filesystems to provide some
> > non-iomap files if desired.
> >
> > Signed-off-by: "Darrick J. Wong" <djwong@...nel.org>
> 
> The logic in this makes sense to me, left just a few comments below.
> 
> Reviewed-by: Joanne Koong <joannelkoong@...il.com>

Thanks!

> > ---
> >  fs/fuse/fuse_i.h          |   17 ++++++++++++++++
> >  include/uapi/linux/fuse.h |    3 +++
> >  fs/fuse/file.c            |    1 +
> >  fs/fuse/file_iomap.c      |   49 +++++++++++++++++++++++++++++++++++++++++++++
> >  fs/fuse/inode.c           |   26 ++++++++++++++++++------
> >  5 files changed, 90 insertions(+), 6 deletions(-)
> >
> >
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 839d4f2ada4656..c7aeb324fe599e 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -257,6 +257,8 @@ enum {
> >          * or the fuse server has an exclusive "lease" on distributed fs
> >          */
> >         FUSE_I_EXCLUSIVE,
> > +       /* Use iomap for this inode */
> > +       FUSE_I_IOMAP,
> >  };
> >
> >  struct fuse_conn;
> > @@ -1717,11 +1719,26 @@ extern const struct fuse_backing_ops fuse_iomap_backing_ops;
> >
> >  void fuse_iomap_mount(struct fuse_mount *fm);
> >  void fuse_iomap_unmount(struct fuse_mount *fm);
> > +
> > +void fuse_iomap_init_reg_inode(struct inode *inode, unsigned attr_flags);
> > +void fuse_iomap_init_nonreg_inode(struct inode *inode, unsigned attr_flags);
> > +void fuse_iomap_evict_inode(struct inode *inode);
> > +
> > +static inline bool fuse_inode_has_iomap(const struct inode *inode)
> > +{
> > +       const struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       return test_bit(FUSE_I_IOMAP, &fi->state);
> > +}
> >  #else
> >  # define fuse_iomap_enabled(...)               (false)
> >  # define fuse_has_iomap(...)                   (false)
> >  # define fuse_iomap_mount(...)                 ((void)0)
> >  # define fuse_iomap_unmount(...)               ((void)0)
> > +# define fuse_iomap_init_reg_inode(...)                ((void)0)
> > +# define fuse_iomap_init_nonreg_inode(...)     ((void)0)
> > +# define fuse_iomap_evict_inode(...)           ((void)0)
> > +# define fuse_inode_has_iomap(...)             (false)
> >  #endif
> >
> >  #endif /* _FS_FUSE_I_H */
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index e571f8ceecbfad..e949bfe022c3b0 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -243,6 +243,7 @@
> >   *
> >   *  7.99
> >   *  - add FUSE_IOMAP and iomap_{begin,end,ioend} for regular file operations
> > + *  - add FUSE_ATTR_IOMAP to enable iomap for specific inodes
> >   */
> >
> >  #ifndef _LINUX_FUSE_H
> > @@ -583,9 +584,11 @@ struct fuse_file_lock {
> >   *
> >   * FUSE_ATTR_SUBMOUNT: Object is a submount root
> >   * FUSE_ATTR_DAX: Enable DAX for this file in per inode DAX mode
> > + * FUSE_ATTR_IOMAP: Use iomap for this inode
> >   */
> >  #define FUSE_ATTR_SUBMOUNT      (1 << 0)
> >  #define FUSE_ATTR_DAX          (1 << 1)
> > +#define FUSE_ATTR_IOMAP                (1 << 2)
> >
> >  /**
> >   * Open flags
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index f1ef77a0be05bb..42c85c19f3b13b 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -3135,6 +3135,7 @@ void fuse_init_file_inode(struct inode *inode, unsigned int flags)
> >         init_waitqueue_head(&fi->page_waitq);
> >         init_waitqueue_head(&fi->direct_io_waitq);
> >
> > +       fuse_iomap_init_reg_inode(inode, flags);
> 
> imo it's a bit confusing to have this here when the rest of the
> fuse_iomap_init_nonreg_inode() calls happen inside the switch case
> statement. Maybe it makes sense to have this inside the switch case
> like the fuse_iomap_init_nonreg_inode() calls, or alternatively move
> the fuse_iomap_init_nonreg_inode() calls into their corresponding
> helpers (eg fuse_init_dir(), etc.), so that it's consistent?

Ah, that.  Originally I /did/ have it in the switch statement in
fuse_init_inode.  Then I started trying to work on fsdax support (HA!)
for which it became necessary to move the fuse_iomap_init_reg_inode call
to fuse_init_file_inode and pass it a pointer to args->flags so that it
could clear FUSE_ATTR_DAX so that the other fuse dax io paths wouldn't
try to install themselves.

I never got fsdax working properly so that's why it's never been
attached to my fuse-iomap patches.  Maybe that'll happen some day in the
meantime ... should fuse_iomap_init_reg_inode move back to the switch?

> >         if (IS_ENABLED(CONFIG_FUSE_DAX))
> >                 fuse_dax_inode_init(inode, flags);
> >  }
> > diff --git a/fs/fuse/file_iomap.c b/fs/fuse/file_iomap.c
> > index 1b9e1bf2f799a3..fc0d5f135bacf9 100644
> > --- a/fs/fuse/file_iomap.c
> > +++ b/fs/fuse/file_iomap.c
> > @@ -635,3 +635,52 @@ void fuse_iomap_unmount(struct fuse_mount *fm)
> >         fuse_flush_requests_and_wait(fc);
> >         fuse_send_destroy(fm);
> >  }
> > +
> > +static inline void fuse_inode_set_iomap(struct inode *inode)
> > +{
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       set_bit(FUSE_I_IOMAP, &fi->state);
> > +}
> > +
> > +static inline void fuse_inode_clear_iomap(struct inode *inode)
> > +{
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       clear_bit(FUSE_I_IOMAP, &fi->state);
> > +}
> > +
> > +void fuse_iomap_init_nonreg_inode(struct inode *inode, unsigned attr_flags)
> > +{
> > +       struct fuse_conn *conn = get_fuse_conn(inode);
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       ASSERT(!S_ISREG(inode->i_mode));
> > +
> > +       if (conn->iomap && (attr_flags & FUSE_ATTR_IOMAP))
> > +               set_bit(FUSE_I_EXCLUSIVE, &fi->state);
> > +}
> > +
> > +void fuse_iomap_init_reg_inode(struct inode *inode, unsigned attr_flags)
> > +{
> > +       struct fuse_conn *conn = get_fuse_conn(inode);
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       ASSERT(S_ISREG(inode->i_mode));
> > +
> > +       if (conn->iomap && (attr_flags & FUSE_ATTR_IOMAP)) {
> > +               set_bit(FUSE_I_EXCLUSIVE, &fi->state);
> > +               fuse_inode_set_iomap(inode);
> > +       }
> > +}
> > +
> > +void fuse_iomap_evict_inode(struct inode *inode)
> > +{
> > +       struct fuse_conn *conn = get_fuse_conn(inode);
> > +       struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +       if (fuse_inode_has_iomap(inode))
> 
> If I'm understanding this correctly, a fuse inode can't have
> FUSE_I_IOMAP set on it if conn>iomap is not enabled, correct?

Correct.

> Maybe it makes sense to just return if (!conn->iomap) at the very
> beginning, to make that more clear?

<shrug> fuse_inode_has_iomap only checks FUSE_I_IOMAP...

> > +               fuse_inode_clear_iomap(inode);
> > +       if (conn->iomap && fuse_inode_is_exclusive(inode))
> > +               clear_bit(FUSE_I_EXCLUSIVE, &fi->state);

...but I wasn't going to assume that iomap is the only way that
FUSE_I_EXCLUSIVE could get set.

On the other hand, for non-regular files we set FUSE_I_EXCLUSIVE only if
conn->iomap is nonzero *and* attr->flags contains FUSE_ATTR_IOMAP.  So
this clearing code isn't quite the same as the setting code.

I wonder if that means we should set FUSE_I_IOMAP for non-regular files?
They don't use iomap itself, but I suppose it would be neat if "iomap
directories" also meant that timestamps and whatnot worked in the same
as they do for regular files.

> > +}
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 271356fa3be3ea..9b9e7b2dd0d928 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -196,6 +196,8 @@ static void fuse_evict_inode(struct inode *inode)
> >                 WARN_ON(!list_empty(&fi->write_files));
> >                 WARN_ON(!list_empty(&fi->queued_writes));
> >         }
> > +
> > +       fuse_iomap_evict_inode(inode);
> >  }
> >
> >  static int fuse_reconfigure(struct fs_context *fsc)
> > @@ -428,20 +430,32 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr,
> >         inode->i_size = attr->size;
> >         inode_set_mtime(inode, attr->mtime, attr->mtimensec);
> >         inode_set_ctime(inode, attr->ctime, attr->ctimensec);
> > -       if (S_ISREG(inode->i_mode)) {
> > +       switch (inode->i_mode & S_IFMT) {
> > +       case S_IFREG:
> >                 fuse_init_common(inode);
> >                 fuse_init_file_inode(inode, attr->flags);
> > -       } else if (S_ISDIR(inode->i_mode))
> > +               break;
> > +       case S_IFDIR:
> >                 fuse_init_dir(inode);
> > -       else if (S_ISLNK(inode->i_mode))
> > +               fuse_iomap_init_nonreg_inode(inode, attr->flags);
> > +               break;
> > +       case S_IFLNK:
> >                 fuse_init_symlink(inode);
> > -       else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> > -                S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> > +               fuse_iomap_init_nonreg_inode(inode, attr->flags);
> > +               break;
> > +       case S_IFCHR:
> > +       case S_IFBLK:
> > +       case S_IFIFO:
> > +       case S_IFSOCK:
> >                 fuse_init_common(inode);
> >                 init_special_inode(inode, inode->i_mode,
> >                                    new_decode_dev(attr->rdev));
> > -       } else
> > +               fuse_iomap_init_nonreg_inode(inode, attr->flags);
> > +               break;
> > +       default:
> >                 BUG();
> 
> Just thinking out loud here and curious to hear whether you like this
> idea or not: another option is calling
> 
> if (conn->iomap)
>     fuse_iomap_init_inode();
> 
> at the end, where fuse_iomap_init_inode() would be something like:
> 
> void fuse_iomap_init_inode(struct inode *inode, unsigned attr_flags)
> {
>     struct fuse_inode *fi = get_fuse_inode(inode);
> 
>     if (attr_flags & FUSE_ATTR_IOMAP)
>           set_bit(FUSE_I_EXCLUSIVE, &fi->state);
> 
>     if (S_ISREG(inode->i_mode))
>             fuse_inode_set_iomap(inode);
> }
> 
> which seems simpler to me than having both
> fuse_iomap_init_nonreg_inode() and fuse_iomap_init_reg_inode()
> function and invoking it per i_mode case.

Yeah that would be simpler, but for that weird fsdax enabling quirk I
mentioned earlier.

Hrmm, I could also modify fuse_dax_inode_init to return without doing
anything if FUSE_ATTR_IOMAP is set.

--D

> Thanks,
> Joanne
> 
> > +               break;
> > +       }
> >         /*
> >          * Ensure that we don't cache acls for daemons without FUSE_POSIX_ACL
> >          * so they see the exact same behavior as before.
> >
> 

Powered by blists - more mailing lists