Re: [PATCH v3 21/21] fuse: Increase MAX_WRITE_SIZE with a second buffer

[Hanna Czenczek <hreitz@redhat.com>]

On 23.10.25 17:11, Kevin Wolf wrote:
> Am 01.07.2025 um 13:44 hat Hanna Czenczek geschrieben:
>> We probably want to support larger write sizes than just 4k; 64k seems
>> nice.  However, we cannot read partial requests from the FUSE FD, we
>> always have to read requests in full; so our read buffer must be large
>> enough to accommodate potential 64k writes if we want to support that.
>>
>> Always allocating FuseRequest objects with 64k buffers in them seems
>> wasteful, though.  But we can get around the issue by splitting the
>> buffer into two and using readv(): One part will hold all normal (up to
>> 4k) write requests and all other requests, and a second part (the
>> "spill-over buffer") will be used only for larger write requests.  Each
>> FuseQueue has its own spill-over buffer, and only if we find it used
>> when reading a request will we move its ownership into the FuseRequest
>> object and allocate a new spill-over buffer for the queue.
>>
>> This way, we get to support "large" write sizes without having to
>> allocate big buffers when they aren't used.
>>
>> Also, this even reduces the size of the FuseRequest objects because the
>> read buffer has to have at least FUSE_MIN_READ_BUFFER (8192) bytes; but
>> the requests we support are not quite so large (except for >4k writes),
>> so until now, we basically had to have useless padding in there.
>>
>> With the spill-over buffer added, the FUSE_MIN_READ_BUFFER requirement
>> is easily met and we can decrease the size of the buffer portion that is
>> right inside of FuseRequest.
>>
>> As for benchmarks, the benefit of this patch can be shown easily by
>> writing a 4G image (with qemu-img convert) to a FUSE export:
>> - Before this patch: Takes 25.6 s (14.4 s with -t none)
>> - After this patch: Takes 4.5 s (5.5 s with -t none)
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> The commit message seems outdated, there is no such thing as a
> FuseRequest object.
>
> I agree with the idea of allocating a separate buffer for the data to be
> written. I'm not so sure that the approach taken here with combining an
> in-place and a spillover buffer does actually do much for us in exchange
> for the additional complexity.
>
> The allocation + memcpy for in_place buf in fuse_co_write() bothers me a
> bit. I'd rather have a buffer for the data to write that can be directly
> used. And let's be real, we already allocate a 1 MB stack per request. I
> don't think 64k more or less make a big difference, but it would allow
> us to save the memcpy() for 4k requests and additionally an allocation
> for larger requests.
>
> The tradeoff when you use an iov for the buffer in FuseQueue that is
> only big enough for the header and fuse_write_in and then directly the
> per-request buffer that is owned by the coroutine is that for requests
> that are larger than fuse_write_in, you'll have to copy the rest back
> from the data buffer first. This seems to be only fuse_setattr_in, which
> shouldn't be a hot path at all, and only a few bytes.

So I understand that first, you disagree with “Always allocating 
FuseRequest objects with 64k buffers in them seems wasteful, though.” 
I.e. to just use a 64k buffer per request.  OK, fair.

Second, you suggest to improve performance by having an aligned 64k data 
buffer separate from the request metadata buffer to save on memcpy().  I 
did consider this, but discarded it because of I was afraid of the 
complexity.  Actually probably too afraid.

I’ll take a look, it actually can’t be too hard.  (Just have two 
buffers; make the metadata buffer long enough to capture all request 
headers, but only pass the sizeof(fuse_write_in)-long head into readv(), 
then check the request opcode.  If metadata spilled over to the data 
buffer, copy it back into the “shadowed” metadata buffer tail.)

Hanna

> All that said, what you have is an obvious improvement over limiting
> write requests to 4k.
>
> Kevin
>