| Message ID | 20220926231822.994383-13-drosen@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | FUSE BPF: A Stacked Filesystem Extension for FUSE | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-PR | fail | PR summary |
| bpf/vmtest-bpf-VM_Test-1 | pending | Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain }} |
| bpf/vmtest-bpf-VM_Test-2 | fail | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-VM_Test-3 | fail | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-VM_Test-4 | fail | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-VM_Test-5 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-VM_Test-6 | success | Logs for set-matrix |
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.
On Tue, Sep 27, 2022 at 3:07 PM Dave Chinner <david@fromorbit.com> wrote: > > 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 > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > Thanks for the suggestion, that'll help clean things up a bit. It's quite nice to have fresh eyes looking over the code. -Daniel
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 + if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) return -EOPNOTSUPP; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index fc3e8adf0422..0e4996766c6c 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1422,6 +1422,17 @@ int fuse_lseek_finalize(struct bpf_fuse_args *fa, loff_t *out, struct file *file ssize_t fuse_backing_mmap(struct file *file, struct vm_area_struct *vma); +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); +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); +int fuse_file_fallocate_backing(struct bpf_fuse_args *fa, int *out, + struct file *file, int mode, loff_t offset, loff_t length); +int fuse_file_fallocate_finalize(struct bpf_fuse_args *fa, int *out, + struct file *file, int mode, loff_t offset, loff_t length); + struct fuse_lookup_io { struct fuse_entry_out feo; struct fuse_entry_bpf feb;