Re: [PATCH v2] fuse: allow FUSE drivers to declare themselves free from outside changes

[Amir Goldstein]

On Tue, Apr 2, 2024 at 4:29 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Tue, 2024-04-02 at 15:23 +0200, Bernd Schubert wrote:
> >
> > On 4/2/24 15:10, Jeff Layton wrote:
> > > Traditionally, we've allowed people to set leases on FUSE inodes.  Some
> > > FUSE drivers are effectively local filesystems and should be fine with
> > > kernel-internal lease support. Others are backed by a network server
> > > that may have multiple clients, or may be backed by something non-file
> > > like entirely. On those, we don't want to allow leases.
> > >
> > > Have the filesytem driver to set a fuse_conn flag to indicate whether
> > > the inodes are subject to outside changes, not done via kernel APIs.  If
> > > the flag is unset (the default), then setlease attempts will fail with
> > > -EINVAL, indicating that leases aren't supported on that inode.
> > >
> > > Local-ish filesystems may want to start setting this value to true to
> > > preserve the ability to set leases.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > > This is only tested for compilation, but it's fairly straightforward.
> > >
> > > I've left the default the "safe" value of false, so that we assume that
> > > outside changes are possible unless told otherwise.
> > > ---
> > > Changes in v2:
> > > - renamed flag to FUSE_NO_OUTSIDE_CHANGES
> > > - flesh out comment describing the new flag
> > > ---
> > >  fs/fuse/file.c            | 11 +++++++++++
> > >  fs/fuse/fuse_i.h          |  5 +++++
> > >  fs/fuse/inode.c           |  4 +++-
> > >  include/uapi/linux/fuse.h |  1 +
> > >  4 files changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > index a56e7bffd000..79c7152c0d12 100644
> > > --- a/fs/fuse/file.c
> > > +++ b/fs/fuse/file.c
> > > @@ -3298,6 +3298,16 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off,
> > >     return ret;
> > >  }
> > >
> > > +static int fuse_setlease(struct file *file, int arg,
> > > +                    struct file_lease **flp, void **priv)
> > > +{
> > > +   struct fuse_conn *fc = get_fuse_conn(file_inode(file));
> > > +
> > > +   if (fc->no_outside_changes)
> > > +           return generic_setlease(file, arg, flp, priv);
> > > +   return -EINVAL;
> > > +}
> > > +
> > >  static const struct file_operations fuse_file_operations = {
> > >     .llseek         = fuse_file_llseek,
> > >     .read_iter      = fuse_file_read_iter,
> > > @@ -3317,6 +3327,7 @@ static const struct file_operations fuse_file_operations = {
> > >     .poll           = fuse_file_poll,
> > >     .fallocate      = fuse_file_fallocate,
> > >     .copy_file_range = fuse_copy_file_range,
> > > +   .setlease       = fuse_setlease,
> > >  };
> > >
> > >  static const struct address_space_operations fuse_file_aops  = {
> > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > index b24084b60864..49d44a07b0db 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -860,6 +860,11 @@ struct fuse_conn {
> > >     /** Passthrough support for read/write IO */
> > >     unsigned int passthrough:1;
> > >
> > > +   /** Can we assume that the only changes will be done via the local
> > > +    *  kernel? If the driver represents a network filesystem or is a front
> > > +    *  for data that can change on its own, set this to false. */
> > > +   unsigned int no_outside_changes:1;
> > > +
> > >     /** Maximum stack depth for passthrough backing files */
> > >     int max_stack_depth;
> > >
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index 3a5d88878335..f33aedccdb26 100644
> > > --- a/fs/fuse/inode.c
> > > +++ b/fs/fuse/inode.c
> > > @@ -1330,6 +1330,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > >                     }
> > >                     if (flags & FUSE_NO_EXPORT_SUPPORT)
> > >                             fm->sb->s_export_op = &fuse_export_fid_operations;
> > > +                   if (flags & FUSE_NO_OUTSIDE_CHANGES)
> > > +                           fc->no_outside_changes = 1;
> > >             } else {
> > >                     ra_pages = fc->max_read / PAGE_SIZE;
> > >                     fc->no_lock = 1;
> > > @@ -1377,7 +1379,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > >             FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
> > >             FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
> > >             FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
> > > -           FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND;
> > > +           FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_NO_OUTSIDE_CHANGES;
> > >  #ifdef CONFIG_FUSE_DAX
> > >     if (fm->fc->dax)
> > >             flags |= FUSE_MAP_ALIGNMENT;
> > > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > > index d08b99d60f6f..703d149d45ff 100644
> > > --- a/include/uapi/linux/fuse.h
> > > +++ b/include/uapi/linux/fuse.h
> > > @@ -463,6 +463,7 @@ struct fuse_file_lock {
> > >  #define FUSE_PASSTHROUGH   (1ULL << 37)
> > >  #define FUSE_NO_EXPORT_SUPPORT     (1ULL << 38)
> > >  #define FUSE_HAS_RESEND            (1ULL << 39)
> > > +#define FUSE_NO_OUTSIDE_CHANGES    (1ULL << 40)
> >
> > Above all of these flags are comments explaining the flags, so that one
> > doesn't need to look up in kernel sources what the exact meaning is.
> >
> > Could you please add something like below?
> >
> > FUSE_NO_OUTSIDE_CHANGES: No file changes through other mounts / clients
> >
>
> Definitely. I've added that in my local branch. I can either resend
> later, or maybe Miklos can just add that if he's otherwise OK with this
> patch.

Don't love the name but don't have any suggestions either.

I am wondering out loud, if we have such a mode for the fs,
if and how should it affect caching configuration?

Thanks,
Amir.