On Tue, 2024-06-04 at 10:02 +0200, Miklos Szeredi wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, 3 Jun 2024 at 17:28, Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> wrote:
> >
> > On Mon, Jun 03, 2024 at 04:56:14PM +0200, Miklos Szeredi wrote:
> > > On Mon, Jun 3, 2024 at 3:44 PM Stefan Hajnoczi
> > > <stefanha@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Jun 03, 2024 at 11:06:19AM +0200, Miklos Szeredi wrote:
> > > > > On Mon, 3 Jun 2024 at 10:53, Peter-Jan Gootzen
> > > > > <pgootzen@xxxxxxxxxx> wrote:
> > > > >
> > > > > > We also considered this idea, it would kind of be like
> > > > > > locking FUSE into
> > > > > > being x86. However I think this is not backwards compatible.
> > > > > > Currently
> > > > > > an ARM64 client and ARM64 server work just fine. But making
> > > > > > such a
> > > > > > change would break if the client has the new driver version
> > > > > > and the
> > > > > > server is not updated to know that it should interpret x86
> > > > > > specifically.
> > > > >
> > > > > This would need to be negotiated, of course.
> > > > >
> > > > > But it's certainly simpler to just indicate the client arch in
> > > > > the
> > > > > INIT request. Let's go with that for now.
> > > >
> > > > In the long term it would be cleanest to choose a single
> > > > canonical
> > > > format instead of requiring drivers and devices to implement
> > > > many
> > > > arch-specific formats. I liked the single canonical format idea
> > > > you
> > > > suggested.
> > > >
> > > > My only concern is whether there are more commands/fields in
> > > > FUSE that
> > > > operate in an arch-specific way (e.g. ioctl)? If there really
> > > > are parts
> > > > that need to be arch-specific, then it might be necessary to
> > > > negotiate
> > > > an architecture after all.
> > >
> > > How about something like this:
> > >
> > > - by default fall back to no translation for backward
> > > compatibility
> > > - server may request matching by sending its own arch identifier
> > > in
> > > fuse_init_in
> > > - client sends back its arch identifier in fuse_init_out
> > > - client also sends back a flag indicating whether it will
> > > transform
> > > to canonical or not
> > >
> > > This means the client does not have to implement translation for
> > > every
> > > architecture, only ones which are frequently used as guest. The
> > > server may opt to implement its own translation if it's lacking in
> > > the
> > > client, or it can just fail.
> >
> > From the client perspective:
> >
> > 1. Do not negotiate arch in fuse_init_out - hopefully client and
> > server
> > know what they are doing :). This is the current behavior.
> > 2. Reply to fuse_init_in with server's arch in fuse_init_out -
> > client
> > translates according to server's arch.
> > 3. Reply to fuse_init_in with canonical flag set in fuse_init_out -
> > client and server use canonical format.
> >
> > From the server perspective:
> >
> > 1. Client does not negotiate arch - the current behavior (good
> > luck!).
> > 2. Arch received in fuse_init_out from client - must be equal to
> > server's arch since there is no way for the server to reject the
> > arch.
> > 3. Canonical flag received in fuse_init_out from client - client and
> > server use canonical format.
>
> Yeah, something like that (I got the direction of the negotiation
> wrong).
>
> See below patch.
>
> I'm thinking that fuse_init_out need not even have the server arch,
> since the client will only be translating to the canonical arch. The
> client sends its own arch in fuse_init_in.arch_id and advertises with
> FUSE_CANON_ARCH set in fuse_init_in.flags whether it supports
> transforming to canonical. If the server wants canonicalization, then
> it responds with FUSE_CANON_ARCH set in fuse_init_out.flags.
Will the FUSE_CANON_ARCH then be default/required in init_in from the
new minor onwards?
If so, a server/device that supports the new minor, would only need to
support the canonical format.
The fuse_init_in.arch_id is then only really used for the server/device
to know the format of IOCTL (like Idan brought up).
>
> This works for legacy server that doesn't interpret the new flag and
> field, and also for legacy client that doesn't set either (zero
> arch_id means: unknown architecture).
>
> arch_id could be a hash of the arch name, so that the fuse header file
> doesn't need to be updated whenever a new architecture is added.
Who defines what the arch names are? What about capitalization?
The last time an arch with its own constants was added was 12 years ago
with ARM64. So the header wouldn't change often. Or is this something
that the kernel avoids in general?
>
> Thanks,
> Miklos
>
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index d08b99d60f6f..c63d8bab2d37 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -421,6 +421,7 @@ struct fuse_file_lock {
> * FUSE_NO_EXPORT_SUPPORT: explicitly disable export support
> * FUSE_HAS_RESEND: kernel supports resending pending requests, and
> the high bit
> * FUSE_NO_EXPORT_SUPPORT: explicitly disable export support
> * FUSE_HAS_RESEND: kernel supports resending pending requests, and
> the high bit
> * of the request ID indicates resend requests
> + * FUSE_CANON_ARCH: translate arch specific constants to canonical
> */
> #define FUSE_ASYNC_READ (1 << 0)
> #define FUSE_POSIX_LOCKS (1 << 1)
> @@ -463,6 +464,7 @@ struct fuse_file_lock {
> #define FUSE_PASSTHROUGH (1ULL << 37)
> #define FUSE_NO_EXPORT_SUPPORT (1ULL << 38)
> #define FUSE_HAS_RESEND (1ULL << 39)
> +#define FUSE_CANON_ARCH (1ULL << 40)
>
> /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */
> #define FUSE_DIRECT_IO_RELAX FUSE_DIRECT_IO_ALLOW_MMAP
> @@ -874,7 +876,8 @@ struct fuse_init_in {
> uint32_t max_readahead;
> uint32_t flags;
> uint32_t flags2;
> - uint32_t unused[11];
What is the reason for splitting the unused bytes here?
> + uint32_t arch_id;
> + uint32_t unused[10];
> };
>
> #define FUSE_COMPAT_INIT_OUT_SIZE 8
If arch_id is only used for IOCTL and the rest of the translation is
through the canonical format with FUSE_CANON_ARCH, then I like this
approach.
I think that if we introduce the canonical format, and also require the
server or client to be ready to do translation from and towards the
handshaked format specified in arch_id. Then it will be overly
complicated on both sides without adding any value.
- Peter-Jan
