Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230418014037.2412394-1-drosen@google.com>
In-Reply-To: <20230418014037.2412394-1-drosen@google.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 18 Apr 2023 08:33:38 +0300
Message-ID: <CAOQ4uxhpFrRVcviQ6bK1ZMtZDSMXRFuqY-d_+uQ1C0YMDtQpLA@mail.gmail.com>
Subject: Re: [RFC PATCH bpf-next v3 00/37] FUSE BPF: A Stacked Filesystem
 Extension for FUSE
To:     Daniel Rosenberg <drosen@google.com>
Cc:     Miklos Szeredi <miklos@szeredi.hu>, bpf@vger.kernel.org,
        Alexei Starovoitov <ast@kernel.org>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-unionfs@vger.kernel.org,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Shuah Khan <shuah@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Joanne Koong <joannelkoong@gmail.com>,
        Mykola Lysenko <mykolal@fb.com>, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Apr 18, 2023 at 4:40=E2=80=AFAM Daniel Rosenberg <drosen@google.com=
> wrote:
>
> These patches extend FUSE to be able to act as a stacked filesystem. This
> allows pure passthrough, where the fuse file system simply reflects the l=
ower
> filesystem, and also allows optional pre and post filtering in BPF and/or=
 the
> userspace daemon as needed. This can dramatically reduce or even eliminat=
e
> transitions to and from userspace.
>
> In this patch set, I've reworked the bpf code to add a new struct_op type
> instead of a new program type, and used new kfuncs in place of new helper=
s.
> Additionally, it now uses dynptrs for variable sized buffers. The first t=
hree
> patches are repeats of a previous patch set which I have not yet adjusted=
 for
> comments. I plan to adjust those and submit them separately with fixes, b=
ut
> wanted to have the current fuse-bpf code visible before then.
>
> Patches 4-7 mostly rearrange existing code to remove noise from the main =
patch.
> Patch 8 contains the main sections of fuse-bpf
> Patches 9-25 implementing most FUSE functions as operations on a lower
> filesystem. From patch 25, you can run fuse as a passthrough filesystem.
> Patches 26-32 provide bpf functionality so that you can alter fuse parame=
ters
> via fuse_op programs.
> Patch 33 extends this to userspace, and patches 34-37 add some testing
> functionality.
>

That's a nice logical breakup for review.

I feel there is so much subtle code in those patches that the
only sane path forward is to review and merge them in phases.

Your patches adds this config:

+config FUSE_BPF
+       bool "Adds BPF to fuse"
+       depends on FUSE_FS
+       depends on BPF
+       help
+         Extends FUSE by adding BPF to prefilter calls and
potentially pass to a
+         backing file system

Since your patches add the PASSTHROUGH functionality before adding
BPF functionality, would it make sense to review and merge the PASSTHROUGH
functionality strictly before the BPF functionality?

Alternatively, you could aim to merge support for some PASSTHROUGH ops
then support for some BPF functionality and then slowly add ops to both.

Which brings me to my biggest concern.
I still do not see how these patches replace Allesio's
FUSE_DEV_IOC_PASSTHROUGH_OPEN patches.

Is the idea here that ioctl needs to be done at FUSE_LOOKUP
instead or in addition to the ioctl on FUSE_OPEN to setup the
read/write passthrough on the backing file?

I am missing things like the FILESYSTEM_MAX_STACK_DEPTH check that
was added as a result of review on Allesio's patches.

The reason I am concerned about this is that we are using the
FUSE_DEV_IOC_PASSTHROUGH_OPEN patches and I would like
to upstream their functionality sooner rather than later.
These patches have already been running in production for a while
I believe that they are running in Android as well and there is value
in upsteaming well tested patches.

The API does not need to stay FUSE_DEV_IOC_PASSTHROUGH_OPEN
it should be an API that is extendable to FUSE-BPF, but it would be
useful if the read/write passthrough could be the goal for first merge.

Does any of this make sense to you?
Can you draw a roadmap for merging FUSE-BPF that starts with
a first (hopefully short term) phase that adds the read/write passthrough
functionality?

I can help with review and testing of that part if needed.
I was planning to discuss this with you on LSFMM anyway,
but better start the discussion beforehand.

Thanks,
Amir.