Re: [PATCH 12/26] fuse-bpf: Add support for fallocate

[Dave Chinner <david@fromorbit.com>]

On Mon, Sep 26, 2022 at 04:18:08PM -0700, Daniel Rosenberg wrote:
> Signed-off-by: Daniel Rosenberg <drosen@google.com>
> Signed-off-by: Paul Lawrence <paullawrence@google.com>
> ---
>  fs/fuse/backing.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/file.c    | 10 ++++++++++
>  fs/fuse/fuse_i.h  | 11 +++++++++++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> index 97e92c633cfd..95c60d6d7597 100644
> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
> @@ -188,6 +188,54 @@ ssize_t fuse_backing_mmap(struct file *file, struct vm_area_struct *vma)
>  	return ret;
>  }
>  
> +int fuse_file_fallocate_initialize_in(struct bpf_fuse_args *fa,
> +				      struct fuse_fallocate_in *ffi,
> +				      struct file *file, int mode, loff_t offset, loff_t length)
> +{
> +	struct fuse_file *ff = file->private_data;
> +
> +	*ffi = (struct fuse_fallocate_in) {
> +		.fh = ff->fh,
> +		.offset = offset,
> +		.length = length,
> +		.mode = mode,
> +	};
> +
> +	*fa = (struct bpf_fuse_args) {
> +		.opcode = FUSE_FALLOCATE,
> +		.nodeid = ff->nodeid,
> +		.in_numargs = 1,
> +		.in_args[0].size = sizeof(*ffi),
> +		.in_args[0].value = ffi,
> +	};
> +
> +	return 0;
> +}
> +
> +int fuse_file_fallocate_initialize_out(struct bpf_fuse_args *fa,
> +				       struct fuse_fallocate_in *ffi,
> +				       struct file *file, int mode, loff_t offset, loff_t length)
> +{
> +	return 0;
> +}
> +
> +int fuse_file_fallocate_backing(struct bpf_fuse_args *fa, int *out,
> +				struct file *file, int mode, loff_t offset, loff_t length)
> +{
> +	const struct fuse_fallocate_in *ffi = fa->in_args[0].value;
> +	struct fuse_file *ff = file->private_data;
> +
> +	*out = vfs_fallocate(ff->backing_file, ffi->mode, ffi->offset,
> +			     ffi->length);
> +	return 0;
> +}
> +
> +int fuse_file_fallocate_finalize(struct bpf_fuse_args *fa, int *out,
> +				 struct file *file, int mode, loff_t offset, loff_t length)
> +{
> +	return 0;
> +}
> +
>  /*******************************************************************************
>   * Directory operations after here                                             *
>   ******************************************************************************/
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index dd4485261cc7..ef6f6b0b3b59 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -3002,6 +3002,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
>  
>  	bool block_faults = FUSE_IS_DAX(inode) && lock_inode;
>  
> +#ifdef CONFIG_FUSE_BPF
> +	if (fuse_bpf_backing(inode, struct fuse_fallocate_in, err,
> +			       fuse_file_fallocate_initialize_in,
> +			       fuse_file_fallocate_initialize_out,
> +			       fuse_file_fallocate_backing,
> +			       fuse_file_fallocate_finalize,
> +			       file, mode, offset, length))
> +		return err;
> +#endif

As I browse through this series, I find this pattern unnecessarily
verbose and it exposes way too much of the filtering mechanism to
code that should not have to know anything about it.

Wouldn't it be better to code this as:

	error = fuse_filter_fallocate(file, mode, offset, length);
	if (error < 0)
		return error;


And then make this fuse_bpf_backing() call and all the indirect
functions it uses internal (i.e. static) in fs/fuse/backing.c?

That way the interface in fs/fuse/fuse_i.h can be much cleaner and
handle the #ifdef CONFIG_FUSE_BPF directly by:

#ifdef CONFIG_FUSE_BPF
....
int fuse_filter_fallocate(file, mode, offset, length);
....
#else /* !CONFIG_FUSE_BPF */
....
static inline fuse_filter_fallocate(file, mode, offset, length)
{
	return 0;
}
....
#endif /* CONFIG_FUSE_BPF */

This seems much cleaner to me than exposing fuse_bpf_backing()
boiler plate all over the code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com