Received-SPF: pass (google.com: domain of linux-kernel+bounces-128094-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240402-setlease-v2-1-b098a5f9295d@kernel.org>
 <8a8e8c0d-7878-4289-b490-cb9bf17e56b9@fastmail.fm> <f6bbdf158f0ca7a12de9b9f980d4d7fa31399ed9.camel@kernel.org>
In-Reply-To: <f6bbdf158f0ca7a12de9b9f980d4d7fa31399ed9.camel@kernel.org>
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 2 Apr 2024 17:02:12 +0300
Message-ID: <CAOQ4uxiv7xSUS7RDK3esa1Crp8reYXewxkr5fFbhG623P20PwA@mail.gmail.com>
Subject: Re: [PATCH v2] fuse: allow FUSE drivers to declare themselves free
 from outside changes
To: Jeff Layton <jlayton@kernel.org>
Cc: Bernd Schubert <bernd.schubert@fastmail.fm>, Miklos Szeredi <miklos@szeredi.hu>, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 2, 2024 at 4:29=E2=80=AFPM Jeff Layton <jlayton@kernel.org> wro=
te:
>
> 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.  So=
me
> > > FUSE drivers are effectively local filesystems and should be fine wit=
h
> > > kernel-internal lease support. Others are backed by a network server
> > > that may have multiple clients, or may be backed by something non-fil=
e
> > > 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 wit=
h
> > > -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@kernel.org>
> > > ---
> > > 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 th=
at
> > > 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 fil=
e *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 =3D 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 =3D {
> > >     .llseek         =3D fuse_file_llseek,
> > >     .read_iter      =3D fuse_file_read_iter,
> > > @@ -3317,6 +3327,7 @@ static const struct file_operations fuse_file_o=
perations =3D {
> > >     .poll           =3D fuse_file_poll,
> > >     .fallocate      =3D fuse_file_fallocate,
> > >     .copy_file_range =3D fuse_copy_file_range,
> > > +   .setlease       =3D fuse_setlease,
> > >  };
> > >
> > >  static const struct address_space_operations fuse_file_aops  =3D {
> > > 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 loca=
l
> > > +    *  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_moun=
t *fm, struct fuse_args *args,
> > >                     }
> > >                     if (flags & FUSE_NO_EXPORT_SUPPORT)
> > >                             fm->sb->s_export_op =3D &fuse_export_fid_=
operations;
> > > +                   if (flags & FUSE_NO_OUTSIDE_CHANGES)
> > > +                           fc->no_outside_changes =3D 1;
> > >             } else {
> > >                     ra_pages =3D fc->max_read / PAGE_SIZE;
> > >                     fc->no_lock =3D 1;
> > > @@ -1377,7 +1379,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > >             FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_E=
XT |
> > >             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_OUTSID=
E_CHANGES;
> > >  #ifdef CONFIG_FUSE_DAX
> > >     if (fm->fc->dax)
> > >             flags |=3D 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.