diff mbox series

[RFC] fuse: require FUSE drivers to opt-in for local file leases

Message ID 20240319-setlease-v1-1-4ce5a220e85d@kernel.org (mailing list archive)
State New
Headers show
Series [RFC] fuse: require FUSE drivers to opt-in for local file leases | expand

Commit Message

Jeffrey Layton March 19, 2024, 4:54 p.m. UTC 
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. But others are backed by a network server
that may have multiple clients, or may be backed by something non-file
like entirely.

Have the filesytem driver to set a fuse_conn flag to indicate whether it
wants support for local, in-kernel leases. If the flag is unset (the
default), then setlease attempts will fail with -EINVAL, indicating that
leases aren't supported on that inode.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
This is only tested for compilation, but it's fairly straightforward.
Having the FUSE drivers opt-out of this support might be more
backward-compatible, but that is a bit more dangerous. I'd rather driver
maintainer consciously opt-in.
---
 fs/fuse/file.c            | 11 +++++++++++
 fs/fuse/fuse_i.h          |  3 +++
 fs/fuse/inode.c           |  4 +++-
 include/uapi/linux/fuse.h |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)


---
base-commit: 0a7b0acecea273c8816f4f5b0e189989470404cf
change-id: 20240319-setlease-ce31fb8777b0

Best regards,

Comments

Miklos Szeredi March 26, 2024, 9:47 a.m. UTC | #1 
On Tue, 19 Mar 2024 at 17:54, Jeff Layton <jlayton@kernel.org> 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. But others are backed by a network server
> that may have multiple clients, or may be backed by something non-file
> like entirely.
>
> Have the filesytem driver to set a fuse_conn flag to indicate whether it
> wants support for local, in-kernel leases. If the flag is unset (the
> default), then setlease attempts will fail with -EINVAL, indicating that
> leases aren't supported on that inode.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This is only tested for compilation, but it's fairly straightforward.
> Having the FUSE drivers opt-out of this support might be more
> backward-compatible, but that is a bit more dangerous. I'd rather driver
> maintainer consciously opt-in.

In the end the lease behavior will need to be reverted if there are
regressions.  I really don't know which is worse, the risk of
regressions or the of risk data corruption...

Also I'd prefer a more general flag indicating that the the kernel
driver can assume no external changes to the filesystem.  E.g.
FUSE_NO_OUTSIDE_CHANGE.

Does this make sense?  Can you imagine a case where FUSE_LOCAL_LEASES
would be set, but caching policy would not assume no external changes?

Thanks,
Miklos
Jeffrey Layton March 26, 2024, 10:10 a.m. UTC | #2 
On Tue, 2024-03-26 at 10:47 +0100, Miklos Szeredi wrote:
> On Tue, 19 Mar 2024 at 17:54, Jeff Layton <jlayton@kernel.org> 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. But others are backed by a network server
> > that may have multiple clients, or may be backed by something non-file
> > like entirely.
> > 
> > Have the filesytem driver to set a fuse_conn flag to indicate whether it
> > wants support for local, in-kernel leases. If the flag is unset (the
> > default), then setlease attempts will fail with -EINVAL, indicating that
> > leases aren't supported on that inode.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This is only tested for compilation, but it's fairly straightforward.
> > Having the FUSE drivers opt-out of this support might be more
> > backward-compatible, but that is a bit more dangerous. I'd rather driver
> > maintainer consciously opt-in.
> 
> In the end the lease behavior will need to be reverted if there are
> regressions.  I really don't know which is worse, the risk of
> regressions or the of risk data corruption...
> 

Yeah, it's a tough call. My hope was that FUSE drivers that did want
lease support can just set the flag and and rebuild. Worst case, they'd
just lose lease support until that was done. Lease support is generally
an optimization, so things should still work but may be slower.

> Also I'd prefer a more general flag indicating that the the kernel
> driver can assume no external changes to the filesystem.  E.g.
> FUSE_NO_OUTSIDE_CHANGE.
> 
> Does this make sense?  Can you imagine a case where FUSE_LOCAL_LEASES
> would be set, but caching policy would not assume no external changes?
> 

No, that seems like it would work just as well. I can respin the patch
along those lines and resend.
diff mbox series

Patch

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a56e7bffd000..3d9aef376783 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->local_leases)
+		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..2909788bf69f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -860,6 +860,9 @@  struct fuse_conn {
 	/** Passthrough support for read/write IO */
 	unsigned int passthrough:1;
 
+	/** Opt-in to local kernel lease support */
+	unsigned int local_leases: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..6958c7690e68 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_LOCAL_LEASES)
+				fc->local_leases = 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_LOCAL_LEASES;
 #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..76322808159d 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_LOCAL_LEASES	(1ULL << 40)
 
 /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */
 #define FUSE_DIRECT_IO_RELAX	FUSE_DIRECT_IO_ALLOW_MMAP