Re: [PATCH v2 02/10] OvmfPkg: raise PcdShellFileOperationSize to 128KB

["Laszlo Ersek" <lersek@redhat.com>]

On 01/15/21 10:34, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 1/13/21 9:54 AM, Laszlo Ersek wrote:
>> Some UEFI shell commands read and write files in chunks. The chunk size is
>> given by "PcdShellFileOperationSize", whose default in
>> "ShellPkg/ShellPkg.dec" is 4KB (0x1000).
>>
>> The virtio-fs daemon of QEMU advertizes a 128KB maximum buffer size by
>> default, for the FUSE_WRITE operation.
>
> I delayed this patch review because I couldn't find where this value
> is advertized in QEMU (virtiofsd is very new to me). Can you enlighten
> me please?

Yes, absolutely. It's really difficult to find. I actually started to
capture all that information in what would be commit 6f7bc7196ff2
("OvmfPkg/VirtioFsDxe: implement the wrapper function for FUSE_WRITE",
2020-12-21), or perhaps in this patch even -- but in the end, it was
such a divergent set of commits, originating actually in the "libfuse"
project, from which virtiofsd was forked, that I decided to drop it.

So, in virtiofsd, you actually have to look for the *page count* that
corresponds to 128KB -- FUSE_DEFAULT_MAX_PAGES_PER_REQ (32).

In the fuse_session_new() function, we have

    se->conn.max_write = UINT_MAX;
    se->bufsize = FUSE_MAX_MAX_PAGES * getpagesize() + FUSE_BUFFER_HEADER_SIZE;

In the do_init() function, which handles the FUSE_INIT request, we have

    size_t bufsize = se->bufsize;

then

    if (!(arg->flags & FUSE_MAX_PAGES)) {
        size_t max_bufsize = FUSE_DEFAULT_MAX_PAGES_PER_REQ * getpagesize() +
                             FUSE_BUFFER_HEADER_SIZE;
        if (bufsize > max_bufsize) {
            bufsize = max_bufsize;
        }
    }

and later

    if (se->conn.max_write > bufsize - FUSE_BUFFER_HEADER_SIZE) {
        se->conn.max_write = bufsize - FUSE_BUFFER_HEADER_SIZE;
    }

and then

    outarg.max_write = se->conn.max_write;

It's super convoluted, because a bunch of flags are handled, and they
all control the buffer size one way or another, and so the various
limits are converted and enforced back and forth. In practice, the
virtio-fs driver in OVMF does not set the FUSE_MAX_PAGES flag in the
FUSE_INIT request, and so in virtiofsd, we first reduce "bufsize" from

  FUSE_MAX_MAX_PAGES             * getpagesize() + FUSE_BUFFER_HEADER_SIZE;

to

  FUSE_DEFAULT_MAX_PAGES_PER_REQ * getpagesize() + FUSE_BUFFER_HEADER_SIZE;

and then calculate "max_write" by subtracting FUSE_BUFFER_HEADER_SIZE.
The result is

  FUSE_DEFAULT_MAX_PAGES_PER_REQ * getpagesize()

which is 128 KB.


In the libfuse project <https://github.com/libfuse/libfuse.git>, one
related commit is:

  https://github.com/libfuse/libfuse/commit/027d0d17c8a4

and before that,

  https://github.com/libfuse/libfuse/commit/97f4a9cb4fc69

If you try to peek even before that, you end up with historical commits
like

  https://github.com/libfuse/libfuse/commit/065f222cd5850

So basically the *origin* of max_write=128KB is untraceable :) But, at
least the present behavior can be found; it comes from libfuse commit
027d0d17c8a4.


Approaching the same topic from the FUSE client in the Linux kernel,
commit 5da784cce430 ("fuse: add max_pages to init_out", 2018-10-01) is
relevant; it also points to the following discussion on LKML:
<https://lkml.org/lkml/2012/7/5/136>.

Again it's convoluted; the direct answer to your question is,
"max_write=128KB comes from FUSE_DEFAULT_MAX_PAGES_PER_REQ, because the
FUSE_MAX_PAGES flag is not requested in the FUSE_INT operation".

Thanks,
Laszlo

>
>> By raising PcdShellFileOperationSize 32-fold, the number of FUSE write
>> requests shrinks proportionately, when writing large files. And when a
>> Virtio Filesystem is not used, a 128KB chunk size is still not
>> particularly wasteful.
>>
>> Some ad-hoc measurements on my laptop, using OVMF:
>>
>> - The time it takes to copy a ~270MB file from a Virtio Filesystem to the
>>   same Virtio Filesystem improves from ~9 seconds to ~1 second.
>>
>> - The time it takes to compare two identical ~270MB files on the same
>>   Virtio Filesystem improves from ~11 seconds to ~3 seconds.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3125
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>
>> Notes:
>>     v2:
>>     - no changes
>>     - pick up Ard's A-b
>>
>>  OvmfPkg/OvmfPkgIa32.dsc    | 2 ++
>>  OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
>>  OvmfPkg/OvmfPkgX64.dsc     | 2 ++
>>  3 files changed, 6 insertions(+)
>