Re: [PATCH 03/11] fuse: implement the basic iomap mechanisms

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

On Thu, May 29, 2025 at 04:15:57PM -0700, Joanne Koong wrote:
> On Thu, May 29, 2025 at 3:15 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Wed, May 21, 2025 at 5:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > Implement functions to enable upcalling of iomap_begin and iomap_end to
> > > userspace fuse servers.
> > >
> > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > ---
> > >  fs/fuse/fuse_i.h          |   38 ++++++
> > >  fs/fuse/fuse_trace.h      |  258 +++++++++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/fuse.h |   87 ++++++++++++++
> > >  fs/fuse/Kconfig           |   23 ++++
> > >  fs/fuse/Makefile          |    1
> > >  fs/fuse/file_iomap.c      |  280 +++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/fuse/inode.c           |    5 +
> > >  7 files changed, 691 insertions(+), 1 deletion(-)
> > >  create mode 100644 fs/fuse/file_iomap.c
> > >
> > >
> > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > index d56d4fd956db99..aa51f25856697d 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -895,6 +895,9 @@ struct fuse_conn {
> > >         /* Is link not implemented by fs? */
> > >         unsigned int no_link:1;
> > >
> > > +       /* Use fs/iomap for FIEMAP and SEEK_{DATA,HOLE} file operations */
> > > +       unsigned int iomap:1;
> > > +
> > >         /* Use io_uring for communication */
> > >         unsigned int io_uring;
> > >
> > > @@ -1017,6 +1020,11 @@ static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb)
> > >         return sb->s_fs_info;
> > >  }
> > >
> > > +static inline const struct fuse_mount *get_fuse_mount_super_c(const struct super_block *sb)
> > > +{
> > > +       return sb->s_fs_info;
> > > +}
> > > +
> >
> > Instead of adding this new helper (and the ones below), what about
> > modifying the existing (non-const) versions of these helpers to take
> > in const * input args,  eg
> >
> > -static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb)
> > +static inline struct fuse_mount *get_fuse_mount_super(const struct
> > super_block *sb)
> >  {
> >         return sb->s_fs_info;
> >  }
> >
> > Then, doing something like "const struct fuse_mount *mt =
> > get_fuse_mount(inode);" would enforce the same guarantees as "const
> > struct fuse_mount *mt = get_fuse_mount_c(inode);" and we wouldn't need
> > 2 sets of helpers that pretty much do the same thing.
> >
> > >  static inline struct fuse_conn *get_fuse_conn_super(struct super_block *sb)
> > >  {
> > >         return get_fuse_mount_super(sb)->fc;
> > > @@ -1027,16 +1035,31 @@ static inline struct fuse_mount *get_fuse_mount(struct inode *inode)
> > >         return get_fuse_mount_super(inode->i_sb);
> > >  }
> > >
> > > +static inline const struct fuse_mount *get_fuse_mount_c(const struct inode *inode)
> > > +{
> > > +       return get_fuse_mount_super_c(inode->i_sb);
> > > +}
> > > +
> > >  static inline struct fuse_conn *get_fuse_conn(struct inode *inode)
> > >  {
> > >         return get_fuse_mount_super(inode->i_sb)->fc;
> > >  }
> > >
> > > +static inline const struct fuse_conn *get_fuse_conn_c(const struct inode *inode)
> > > +{
> > > +       return get_fuse_mount_super_c(inode->i_sb)->fc;
> > > +}
> > > +
> > >  static inline struct fuse_inode *get_fuse_inode(struct inode *inode)
> > >  {
> > >         return container_of(inode, struct fuse_inode, inode);
> > >  }
> > >
> > > +static inline const struct fuse_inode *get_fuse_inode_c(const struct inode *inode)
> > > +{
> > > +       return container_of(inode, struct fuse_inode, inode);
> > > +}
> > > +
> > >  static inline u64 get_node_id(struct inode *inode)
> > >  {
> > >         return get_fuse_inode(inode)->nodeid;
> > > @@ -1577,4 +1600,19 @@ extern void fuse_sysctl_unregister(void);
> > >  #define fuse_sysctl_unregister()       do { } while (0)
> > >  #endif /* CONFIG_SYSCTL */
> > >
> > > +#if IS_ENABLED(CONFIG_FUSE_IOMAP)
> > > +# include <linux/fiemap.h>
> > > +# include <linux/iomap.h>
> > > +
> > > +bool fuse_iomap_enabled(void);
> > > +
> > > +static inline bool fuse_has_iomap(const struct inode *inode)
> > > +{
> > > +       return get_fuse_conn_c(inode)->iomap;
> > > +}
> > > +#else
> > > +# define fuse_iomap_enabled(...)               (false)
> > > +# define fuse_has_iomap(...)                   (false)
> > > +#endif
> > > +
> > >  #endif /* _FS_FUSE_I_H */
> > > diff --git a/fs/fuse/Kconfig b/fs/fuse/Kconfig
> > > index ca215a3cba3e31..fc7c5bf1cef52d 100644
> > > --- a/fs/fuse/Kconfig
> > > +++ b/fs/fuse/Kconfig
> > > @@ -64,6 +64,29 @@ config FUSE_PASSTHROUGH
> > >
> > >           If you want to allow passthrough operations, answer Y.
> > >
> > > +config FUSE_IOMAP
> > > +       bool "FUSE file IO over iomap"
> > > +       default y
> > > +       depends on FUSE_FS
> > > +       select FS_IOMAP
> > > +       help
> > > +         For supported fuseblk servers, this allows the file IO path to run
> > > +         through the kernel.
> >
> > I have config FUSE_FS select FS_IOMAP in my patchset (not yet
> > submitted) that changes fuse buffered writes / writeback handling to
> > use iomap. Could we just have config FUSE_FS automatically opt into
> > FS_IOMAP here or do you see a reason that this needs to be a separate
> > config?
> 
> Thinking about it some more, the iomap stuff you're adding also
> requires a "depends on BLOCK", so this will need to be a separate
> config anyways regardless of whether the FUSE_FS will always "select
> FS_IOMAP"

I'll add that, thanks.  I forgot that FS_IOMAP no longer selects BLOCK
all the time. :)

--D

> 
> Thanks,
> Joanne
> 
> >
> >
> > Thanks,
> > Joanne
> > > +
> > > +config FUSE_IOMAP_BY_DEFAULT
> > > +       bool "FUSE file I/O over iomap by default"
> > > +       default n
> > > +       depends on FUSE_IOMAP
> > > +       help
> > > +         Enable sending FUSE file I/O over iomap by default.
> > > +
> > > +config FUSE_IOMAP_DEBUG
> > > +       bool "Debug FUSE file IO over iomap"
> > > +       default n
> > > +       depends on FUSE_IOMAP
> > > +       help
> > > +         Enable debugging assertions for the fuse iomap code paths.
> > > +
> > >  config FUSE_IO_URING
> > >         bool "FUSE communication over io-uring"
> > >         default y