virtio-fs tests between host(x86) and dpu(arm64)

virtio-fs tests between host(x86) and dpu(arm64)

[Lege Wang]

Hello,

I see that you have added multi-queue support for virtio-fs, thanks for this work.
From your patch's commit log, your host is x86-64, dpu is arm64, but there're
differences about O_DIRECT and O_DIRECTORY between these two architectures.

Test program:
#define _GNU_SOURCE

#include <stdio.h>
#include <fcntl.h>

int main(void)
{
        printf("O_DIRECT:%o\n", O_DIRECT);
        printf("O_DIRECTORY:%o\n", O_DIRECTORY);
        return 0;
}

In x86-64, this test program outputs:
O_DIRECT:40000
O_DIRECTORY:200000

But in arm64, it outpus:
O_DIRECT:200000
O_DIRECTORY:40000

In kernel fuse module, fuse_create_in->flags will used to hold open(2)'s flags, then
a O_DIRECT flag from host(x86) would be treated as O_DIRECTORY in dpu(arm64), which
seems a serious bug.

From your fio job, you use libaio engine, so it's assumed that direct-io is
enabled, so I wonder why you don't get errors. Could you please try below
command in your virtio-fs mount point:
  dd if=/dev/zero of=tst_file bs=4096 count=1 oflag=direct
to see whether it occurs any error.

Regards,
Xiaoguang Wang

Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Peter-Jan Gootzen]

Hi Xiaoguang and others,
 
You have identified an issue that we are also running into and had also
planned to address with the community.
We currently solve this by explicitly telling the virtio-fs device which
architecture is running on the host through a side-channel so that it
can correctly interpret the flags (e.g. O_DIRECT).
As others are also running into this with the increased popularity of
hardware virtio-fs devices, let's get into this issue.
I have included the FUSE maintainer Milkos Szeredi and the Virtio
maintainer+specification chair Michael S. Tsirkin in this thread.

The core issue here lies in the fact that these fcntl.h definitions are
different per architecture (as of now there are 9 fcntl.h headers). More
specifically, the real numbers defined in the header are different, on
top of possible endianness differences.
FUSE has a mechanism to detect endianness differences with the 32bit
in.opcode value (via FUSE_INIT_BSWAP_RESERVED).
However, there is no mechanism in FUSE or virtio-fs to tell the FUSE
file system or the virtio-fs device which architecture is running on the
host. Thus, the virtio-fs device currently cannot know how to correctly
interpret the fcntl.h flags.
For example, we are dealing with systems that have ARM64 host and ARM64
virtio-fs device, or x86 host and ARM64 virtio-fs device.
As FUSE already contains the mechanism for endianness, it would make
sense to also include a mechanism for the architecture in FUSE to solve
this issue.


We would like to make a proposal regarding our idea for solving this
issue before sending in a patch:
Use a uint32_t from the unused array in FUSE_INIT to encode an `uint32_t
arch_indicator` that contains one of the architecture IDs specified in a
new enum (is there an existing enum like such?):
enum fuse_arch_indicator {
    FUSE_ARCH_NONE = 0,
    FUSE_ARCH_X86 = 1,
    FUSE_ARCH_ARM64 = 2,
    ...
}
Through this the host tells the FUSE file system which version of
fcntl.h it will use.
The FUSE file system should keep a copy of all the possible fcntl
headers and use the one indicated by the `fuse_init_in.arch_indicator`.

For backwards compatibility, a minor version bump is needed. A new file
system implementation connected to an old driver will see the
FUSE_ARCH_NONE or the old minor version, and it will know that it cannot
read the `arch_indicator` and that it cannot make better assumptions
than previously possible.

This would be a minimal, backwards compatible change that extends the
current FUSE portability scheme and doesn't require any specification
changes. When the time comes that a new architecture is introduced with
its own fcntl.h we must simply add another enumerator and an ifdef to
the code setting the `arch_indicator`.

 
- Peter-Jan

On Thu, 2024-05-30 at 09:31 +0000, Lege Wang wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hello,
> 
> I see that you have added multi-queue support for virtio-fs, thanks
> for this work.
> From your patch's commit log, your host is x86-64, dpu is arm64, but
> there're
> differences about O_DIRECT and O_DIRECTORY between these two
> architectures.
> 
> Test program:
> #define _GNU_SOURCE
> 
> #include <stdio.h>
> #include <fcntl.h>
> 
> int main(void)
> {
>         printf("O_DIRECT:%o\n", O_DIRECT);
>         printf("O_DIRECTORY:%o\n", O_DIRECTORY);
>         return 0;
> }
> 
> In x86-64, this test program outputs:
> O_DIRECT:40000
> O_DIRECTORY:200000
> 
> But in arm64, it outpus:
> O_DIRECT:200000
> O_DIRECTORY:40000
> 
> In kernel fuse module, fuse_create_in->flags will used to hold
> open(2)'s flags, then
> a O_DIRECT flag from host(x86) would be treated as O_DIRECTORY in
> dpu(arm64), which
> seems a serious bug.
> 
> From your fio job, you use libaio engine, so it's assumed that direct-
> io is
> enabled, so I wonder why you don't get errors. Could you please try
> below
> command in your virtio-fs mount point:
>   dd if=/dev/zero of=tst_file bs=4096 count=1 oflag=direct
> to see whether it occurs any error.
> 
> Regards,
> Xiaoguang Wang

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Mon, 3 Jun 2024 at 10:02, Peter-Jan Gootzen <pgootzen@nvidia.com> wrote:

> We would like to make a proposal regarding our idea for solving this
> issue before sending in a patch:
> Use a uint32_t from the unused array in FUSE_INIT to encode an `uint32_t
> arch_indicator` that contains one of the architecture IDs specified in a
> new enum (is there an existing enum like such?):
> enum fuse_arch_indicator {
>     FUSE_ARCH_NONE = 0,
>     FUSE_ARCH_X86 = 1,
>     FUSE_ARCH_ARM64 = 2,
>     ...
> }
> Through this the host tells the FUSE file system which version of
> fcntl.h it will use.
> The FUSE file system should keep a copy of all the possible fcntl
> headers and use the one indicated by the `fuse_init_in.arch_indicator`.

To be clear: you propose that the fuse client (in the VM kernel) sets
the arch indicator and the server (on the host) translates constants?

That sounds like a good plan.

Alternatively the client would optionally translate to a common set of
constants (x86 would be a good choice) and then the server would only
need to know the translation between x86 and native.

What about errno?  Any other arch specific constants?

Thanks,
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Peter-Jan Gootzen]

On Mon, 2024-06-03 at 10:24 +0200, Miklos Szeredi wrote:
> On Mon, 3 Jun 2024 at 10:02, Peter-Jan Gootzen <pgootzen@nvidia.com>
> wrote:
> 
> > We would like to make a proposal regarding our idea for solving this
> > issue before sending in a patch:
> > Use a uint32_t from the unused array in FUSE_INIT to encode an
> > `uint32_t
> > arch_indicator` that contains one of the architecture IDs specified
> > in a
> > new enum (is there an existing enum like such?):
> > enum fuse_arch_indicator {
> >     FUSE_ARCH_NONE = 0,
> >     FUSE_ARCH_X86 = 1,
> >     FUSE_ARCH_ARM64 = 2,
> >     ...
> > }
> > Through this the host tells the FUSE file system which version of
> > fcntl.h it will use.
> > The FUSE file system should keep a copy of all the possible fcntl
> > headers and use the one indicated by the
> > `fuse_init_in.arch_indicator`.
> 
> To be clear: you propose that the fuse client (in the VM kernel) sets
> the arch indicator and the server (on the host) translates constants?
Correct. Or in our case of virtio-fs, the FUSE server running behind the
virtio-fs device (possibly on another architecture) will translate
constants before sending them to the host.

> 
> That sounds like a good plan.
> 
> Alternatively the client would optionally translate to a common set of
> constants (x86 would be a good choice) and then the server would only
> need to know the translation between x86 and native.
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.

> 
> What about errno?  Any other arch specific constants?
Yes there may be other arch specific constants that we need to consider.
I don't think errno is already an issue for us as there x86 and ARM64
are mostly the same, and everything in errno-base.h is already
compatible and are luckily most used in file systems.
But this proposed change would also help possible issues there.

> 
> Thanks,
> Miklos

- Peter-Jan

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Mon, 3 Jun 2024 at 10:53, Peter-Jan Gootzen <pgootzen@nvidia.com> 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.

Thanks,
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

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@nvidia.com> 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.

Stefan

> 
> Thanks,
> Miklos
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Mon, Jun 3, 2024 at 3:44 PM Stefan Hajnoczi <stefanha@redhat.com> 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@nvidia.com> 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.

We need to look at all the requests, if there are some other constants
that need to be transformed.

As for ioctl, the client cannot promise to transform everything, since
most are not interpreted by the kernel.  Ones which are, should be
transformed.

Thanks,
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2806 bytes --]

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@redhat.com> 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@nvidia.com> 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.

Is this what you had in mind?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Lege Wang]

Hello,

> 
> 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@nvidia.com>
> 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.
Agree, I also think we should use canonical format for cases that client and
server have different arches.

Regards,
Xiaoguang Wang
> 
> 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.
> 
> Stefan
> 
> >
> > Thanks,
> > Miklos
> >

RE: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Idan Zach]

Hello,

I just want to make sure we don't miss the following point:
For IOCTL commands, the device MUST be aware of the actual architecture of the driver/kernel because the device is required to interpret some of the incoming data.

Best Regards,

-- Idan

-----Original Message-----
From: Lege Wang <lege.wang@jaguarmicro.com> 
Sent: Tuesday, 4 June 2024 6:09
To: Stefan Hajnoczi <stefanha@redhat.com>; Miklos Szeredi <miklos@szeredi.hu>
Cc: Peter-Jan Gootzen <pgootzen@nvidia.com>; Idan Zach <izach@nvidia.com>; Yoray Zack <yorayz@nvidia.com>; virtualization@lists.linux.dev; Parav Pandit <parav@nvidia.com>; linux-fsdevel@vger.kernel.org; Bin Yang <bin.yang@jaguarmicro.com>; Max Gurtovoy <mgurtovoy@nvidia.com>; mszeredi@redhat.com; Eliav Bar-Ilan <eliavb@nvidia.com>; mst@redhat.com; Oren Duer <oren@nvidia.com>; Angus Chen <angus.chen@jaguarmicro.com>
Subject: RE: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

External email: Use caution opening links or attachments


Hello,

>
> 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@nvidia.com>
> 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.
Agree, I also think we should use canonical format for cases that client and server have different arches.

Regards,
Xiaoguang Wang
>
> 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.
>
> Stefan
>
> >
> > Thanks,
> > Miklos
> >

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Mon, 3 Jun 2024 at 17:28, Stefan Hajnoczi <stefanha@redhat.com> 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@redhat.com> 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@nvidia.com> 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.

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.

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];
+       uint32_t        arch_id;
+       uint32_t        unused[10];
 };

 #define FUSE_COMPAT_INIT_OUT_SIZE 8

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Peter-Jan Gootzen]

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@redhat.com>
> 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@redhat.com> 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@nvidia.com> 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

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Tue, 4 Jun 2024 at 10:28, Peter-Jan Gootzen <pgootzen@nvidia.com> wrote:

> Will the FUSE_CANON_ARCH then be default/required in init_in from the
> new minor onwards?

No.  It just indicates that fuse can translate constants for this
particular arch.  Also I'm not sure non-virtiofs should advertise this
(though it shouldn't hurt).

> 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).

Yes, for ioctl and also to reset the FUSE_CANON_ARCH in fuse_init_out
if the arches match.

> Who defines what the arch names are?

uname -m

It's already defined by the kernel.

> 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?

I don't care much, it's just that I don't think defining constants for
architectures really belongs in the fuse header.

> 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.

Yes.

> 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.

The point is to only translate to and from the canonical arch.

That doesn't mean that the kernel *has* to translate some obsolete
arch, because it's useless.  Only add complexity for things that are
actually useful.  And the proposed protocol supports that.

Thanks.
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Peter-Jan Gootzen]

On Tue, 2024-06-04 at 10:45 +0200, Miklos Szeredi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 4 Jun 2024 at 10:28, Peter-Jan Gootzen <pgootzen@nvidia.com>
> wrote:
> 
> > Will the FUSE_CANON_ARCH then be default/required in init_in from
> > the
> > new minor onwards?
> 
> No.  It just indicates that fuse can translate constants for this
> particular arch.  Also I'm not sure non-virtiofs should advertise this
> (though it shouldn't hurt).
> 
> > 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).
> 
> Yes, for ioctl and also to reset the FUSE_CANON_ARCH in fuse_init_out
> if the arches match.
Great, so from the perspective of the client. If the server doesn't set
the FUSE_CANON_ARCH flag, it can be either:
1. The server is the same arch as the client. All will go well.
2. The server doesn't support the canonical format and it might be a
different architecture, and the troubles that we are currently dealing
with might occur.

Option 1 is detectable if fuse_init_out.minor >= CANON_ARCH_MINOR.
Option 2 is detectable if fuse_init_out.minor < CANON_ARCH_MINOR, not
sure yet what we could do with that knowledge (maybe useful in error
logging?).

> 
> > Who defines what the arch names are?
> 
> uname -m
> 
> It's already defined by the kernel.
> 
> > 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?
> 
> I don't care much, it's just that I don't think defining constants for
> architectures really belongs in the fuse header.
> 
> > 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.
> 
> Yes.
> 
> > 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.
> 
> The point is to only translate to and from the canonical arch.
> 
> That doesn't mean that the kernel *has* to translate some obsolete
> arch, because it's useless.  Only add complexity for things that are
> actually useful.  And the proposed protocol supports that.

Great, I just wanted to prevent that we would need a monster any-to-any
arch translator, possibly on both sides.

> 
> Thanks.
> Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Tue, 4 Jun 2024 at 11:08, Peter-Jan Gootzen <pgootzen@nvidia.com> wrote:

> Option 2 is detectable if fuse_init_out.minor < CANON_ARCH_MINOR, not
> sure yet what we could do with that knowledge (maybe useful in error
> logging?).

Using the version for feature detection breaks if a feature is
backported.  So this method has been deprecated and not used on new
features.

Thanks,
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Peter-Jan Gootzen]

On Tue, 2024-06-04 at 11:18 +0200, Miklos Szeredi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 4 Jun 2024 at 11:08, Peter-Jan Gootzen <pgootzen@nvidia.com>
> wrote:
> 
> > Option 2 is detectable if fuse_init_out.minor < CANON_ARCH_MINOR,
> > not
> > sure yet what we could do with that knowledge (maybe useful in error
> > logging?).
> 
> Using the version for feature detection breaks if a feature is
> backported.  So this method has been deprecated and not used on new
> features.
Oh that is very good to know. So for new features, feature detection is
only done through the flags?

If so, then in this case (and correct me if I'm wrong),
if the client doesn't set the FUSE_CANON_ARCH flag, the server/device
should not read the arch_id.
If the server doesn't set the FUSE_CANON_ARCH flag, then the client
should assume that the server has the same arch. Because it could be
either the same arch, or not support this new feature yet while being a
different arch.

As this is in some sense a bug-fix for certain systems, would this new
feature qualify for backporting?

- Peter-Jan

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Miklos Szeredi]

On Tue, 4 Jun 2024 at 11:31, Peter-Jan Gootzen <pgootzen@nvidia.com> wrote:
>
> On Tue, 2024-06-04 at 11:18 +0200, Miklos Szeredi wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, 4 Jun 2024 at 11:08, Peter-Jan Gootzen <pgootzen@nvidia.com>
> > wrote:
> >
> > > Option 2 is detectable if fuse_init_out.minor < CANON_ARCH_MINOR,
> > > not
> > > sure yet what we could do with that knowledge (maybe useful in error
> > > logging?).
> >
> > Using the version for feature detection breaks if a feature is
> > backported.  So this method has been deprecated and not used on new
> > features.
> Oh that is very good to know. So for new features, feature detection is
> only done through the flags?
>
> If so, then in this case (and correct me if I'm wrong),
> if the client doesn't set the FUSE_CANON_ARCH flag, the server/device
> should not read the arch_id.

Since reserved fields are zeroed, it's possible to check for arch_id
being zero (meaning the client has unknown arch).

So if the client sets the arch but doesn't set FUSE_CANON_ARCH, it
would mean that it does not support translation for this particular
architecture.   The server can still check to see if the arches match,
continue if so, and error out otherwise.

> As this is in some sense a bug-fix for certain systems, would this new
> feature qualify for backporting?

Certainly.

Thanks,
Miklos

Re: Addressing architectural differences between FUSE driver and fs - Re: virtio-fs tests between host(x86) and dpu(arm64)

[Daniel Verkamp]

On Mon, Jun 3, 2024 at 6:44 AM Stefan Hajnoczi <stefanha@redhat.com> 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@nvidia.com> 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.

For the purposes of virtio, it would definitely be preferable to
define the FUSE protocol in a single, canonical,
architecture-independent form. Ideally, this definition would be
sufficient to implement a working virtio-fs client or server without
referring to any Linux headers; currently, the virtio-fs spec just
links to the Linux FUSE uapi header, which seems suboptimal for a
protocol that is meant to be OS-agnostic (and has the problem of
depending on arch-specific values, as discussed in this thread).

One possible problem that I have noticed is that the interpretation of
the max_pages field in fuse_init_out depends on the value of
PAGE_SIZE, which could differ between host and guest even within the
same architecture (e.g. ARM64 with 4K vs. 64K pages); architecture
negotiation would not help with this particular problem. It would be
nice if it could be specified in a canonical way as well, perhaps
changing the definition so that it is always in units of 4096 bytes.

Thanks,
-- Daniel