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> <CAOQ4uxhpFrRVcviQ6bK1ZMtZDSMXRFuqY-d_+uQ1C0YMDtQpLA@mail.gmail.com>
 <CA+PiJmT1wCoGnqtVSfcM-0qKm=Vu-jPf=7Op90vcGo3A7kYr0g@mail.gmail.com>
In-Reply-To: <CA+PiJmT1wCoGnqtVSfcM-0qKm=Vu-jPf=7Op90vcGo3A7kYr0g@mail.gmail.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Sun, 23 Apr 2023 17:50:53 +0300
Message-ID: <CAOQ4uxgwmsAA8b1ApmHh9fKuSyy0-NKgpkDSLk-gUWnaGKXtFQ@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 Fri, Apr 21, 2023 at 4:41=E2=80=AFAM Daniel Rosenberg <drosen@google.com=
> wrote:
>
> On Mon, Apr 17, 2023 at 10:33=E2=80=AFPM Amir Goldstein <amir73il@gmail.c=
om> wrote:
> >
> >
> > 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?
> >
>
> In these patches, the fuse daemon responds to the lookup request via
> an ioctl, essentially in the same way it would have to the /dev/fuse
> node. It just flags the write as coming from an ioctl and calls
> fuse_dev_do_write. An additional block in the lookup response gives
> the backing file and what bpf_ops to use. The main difference is that
> fuse-bpf uses backing inodes, while passthrough uses a file.

Ah right. I wonder if there is benefit in both APIs or if backing inode
is sufficient to impelelent everything the could be interesting to implemen=
t
with a backing file.

> Fuse-bpf's read/write support currently isn't complete, but it does
> allow for direct passthrough. You could set ops to default to
> userspace in every case that Allesio's passthrough code does and it
> should have about the same effect.

What are the subtle differences then?

> With the struct_op change, I did
> notice that doing something like that is more annoying, and am
> planning to add a default op which only takes the meta info and runs
> if the opcode specific op is not present.
>

Sounds interesting. I'll wait to see what you propose.

>
> > I am missing things like the FILESYSTEM_MAX_STACK_DEPTH check that
> > was added as a result of review on Allesio's patches.
> >
>
> I'd definitely want to fix any issues that were fixed there. There's a
> lot of common code between fuse-bpf and fuse passthrough, so many of
> the suggestions there will apply here.
>

That's why I suggested trying to implement the passthough file ioctl
functionality first to make sure that none of the review comments
in the first round were missed.

But if we need functionality of both ioctls, we can collaborate the
work on merging them separately.

> > 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 passthrou=
gh
> > 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.
>
> We've been using an earlier version of fuse-bpf on Android, closer to
> the V1 patches. They fit our current needs but don't cover everything
> we intend to. The V3 patches switch to a new style of bpf program,
> which I'm hoping to get some feedback on before I spend too much time
> fixing up the details. The backing calls themselves can be reviewed
> separately from that though.
>
> Without bpf, we're essentially enabling complete passthrough at a
> directory or file. By default, once you set a backing file fuse-bpf
> calls by the backing filesystem by default, with no additional
> userspace interaction apart from if an installed bpf program says
> otherwise. If we had some commands without others, we'd have behavior
> changes as we introduce support for additional calls. We'd need a way
> to set default behavior. Perhaps something like a u64 flag field
> extension in FUSE_INIT for indicating which opcodes support backing,
> and a response for what those should default to doing. If there's a
> bpf_op present for a given opcode, it would be able to override that
> default. If we had something like that, we'd be able to add support
> for a subset of opcodes in a sensible way.

So maybe this is something to consider.

Thanks,
Amir.