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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/15/21 11:09 AM, Laszlo Ersek wrote:
> On 01/15/21 10:34, Philippe Mathieu-Daudé wrote:
>> On 1/13/21 9:54 AM, Laszlo Ersek wrote:
>>>
>>> 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 a lot for the full explanation!
I'm glad I asked this early enough so this was fresh in your
memory :) At least we got it archived on the list.

Shouldn't QEMU announce this via a better mechanism? I.e. a
fw_cfg entry? This is so convoluted (as you said) than I'm
worried libfuse change a parameter and we end having a debug
nightmare figuring the root cause...

Anyhow this is not blocking this patch, so:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Regards,

Phil.