Received-SPF: pass (google.com: domain of linux-kernel+bounces-128037-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: id8a24192:Fastmail
Message-ID: <8a8e8c0d-7878-4289-b490-cb9bf17e56b9@fastmail.fm>
Date: Tue, 2 Apr 2024 15:23:57 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] fuse: allow FUSE drivers to declare themselves free
 from outside changes
To: Jeff Layton <jlayton@kernel.org>, Miklos Szeredi <miklos@szeredi.hu>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240402-setlease-v2-1-b098a5f9295d@kernel.org>
From: Bernd Schubert <bernd.schubert@fastmail.fm>
Content-Language: en-US, de-DE, fr
In-Reply-To: <20240402-setlease-v2-1-b098a5f9295d@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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@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 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


Thanks,
Bernd