Re: [PATCH 00/26] FUSE BPF: A Stacked Filesystem Extension for FUSE

[Martin KaFai Lau <martin.lau@linux.dev>]

On 9/30/22 5:05 PM, Daniel Rosenberg wrote:
> On Tue, Sep 27, 2022 at 11:41 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> Interesting idea.
>>
>> Some comments on review logistics:
>> - The set is too long and some of the individual patches are way too long for
>> one single patch to review.  Keep in mind that not all of us here are experts in
>> both fuse and bpf.  Making it easier to review first will help at the beginning.
>>    Some ideas:
>>
>>     - Only implement a few ops in the initial revision. From quickly browsing the
>> set, it is implementing the 'struct file_operations fuse_file_operations'?
>> Maybe the first few revisions can start with a few of the ops first.
>>
> 
> I've split it up a fair bit already, do you mean just sending a subset
> of them at a time? I think the current splitting roughly allows for
> that. Patch 1-4 and 5 deal with bpf/verifier code which isn't used
> until patch 24. I can reorder/split up the opcodes arbitrarily.


> Putting the op codes that implement file passthrough first makes
> sense. The code is much easier to test when all/most are present,
> since then I can just use patch 23 to mount without a daemon and run
> xfs tests on them. At least initially I felt the whole stack was
> useful to give the full picture.

I don't mind to have all op codes in each re-spin as long as it can apply 
cleanly to bpf-next where the bpf implementation part will eventually land. 
Patch 26 has to split up though.  It is a few thousand lines in one patch.

I was just thinking to only do a few op codes, eg. the few android use cases you 
have mentioned.  My feeling is other op codes should not be very different in 
term of the bpf side implementation (or it is not true?).  When the patch set 
getting enough traction, then start adding more op codes in the later revisions. 
  That will likely help to re-spin faster and save you time also.


>> - iiuc, the idea is to allow bpf prog to optionally handle the 'struct
>> file_operations' without going back to the user daemon? Have you looked at
>> struct_ops which seems to be a better fit here?  If the bpf prog does not know
>> how to handle an operation (or file?), it can call fuse_file_llseek (for
>> example) as a kfunc to handle the request.
>>
> 
> I wasn't aware of struct_ops. It looks like that may work for us
> instead of making a new prog type. I'll definitely look into that.
> I'll likely sign up for the bpf office hours next week.

You can take a look at the tools/testing/selftests/bpf/progs/bpf_cubic.c.
It implements the whole tcp congestion in bpf. In particular, the bpf prog is 
implementing the kernel 'struct tcp_congestion_ops'.  That selftest example is 
pretty much a direct copy from the kernel net/ipv4/tcp_cubic.c.  Also, in 
BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, ...), it is directly calling the kfunc's 
tcp_reno_undo_cwnd() when the bpf prog does not need to do anything different 
from the kernel's tcp_reno_undo_cwnd().  Look at how it is marked as __ksym in 
bpf_cubic.c

However, echoing Alexei's earlier reply, struct_ops is good when it needs to 
implement a well defined 'struct xyz_operations' that has all function pointer 
in it.  Taking another skim at the set, it seems like it is mostly trying to 
intercept the fuse_simple_request() call?