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@xxxxxxxxx> wrote:
> >
> > On Wed, May 21, 2025 at 5:03 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
> > >
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > Implement functions to enable upcalling of iomap_begin and iomap_end to
> > > userspace fuse servers.
> > >
> > > Signed-off-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> > > ---
> > > 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
