Re: [PATCH 14/15] fuse: Implement multi-threading

[Hanna Czenczek <hreitz@redhat.com>]

On 27.03.25 13:18, Markus Armbruster wrote:
> Hanna Czenczek <hreitz@redhat.com> writes:
>
>> On 26.03.25 12:41, Markus Armbruster wrote:
>>> Hanna Czenczek <hreitz@redhat.com> writes:
>>>
>>>> On 26.03.25 06:38, Markus Armbruster wrote:
>>>>> Hanna Czenczek <hreitz@redhat.com> writes:
>>>>>
>>>>>> FUSE allows creating multiple request queues by "cloning" /dev/fuse FDs
>>>>>> (via open("/dev/fuse") + ioctl(FUSE_DEV_IOC_CLONE)).
>>>>>>
>>>>>> We can use this to implement multi-threading.
>>>>>>
>>>>>> Note that the interface presented here differs from the multi-queue
>>>>>> interface of virtio-blk: The latter maps virtqueues to iothreads, which
>>>>>> allows processing multiple virtqueues in a single iothread.  The
>>>>>> equivalent (processing multiple FDs in a single iothread) would not make
>>>>>> sense for FUSE because those FDs are used in a round-robin fashion by
>>>>>> the FUSE kernel driver.  Putting two of them into a single iothread will
>>>>>> just create a bottleneck.
>>>>>>
>>>>>> Therefore, all we need is an array of iothreads, and we will create one
>>>>>> "queue" (FD) per thread.
>>>>> [...]
>>>>>
>>>>>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>>>>>> ---
>>>>>>   qapi/block-export.json |   8 +-
>>>>>>   block/export/fuse.c    | 214 +++++++++++++++++++++++++++++++++--------
>>>>>>   2 files changed, 179 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>>>>>> index c783e01a53..0bdd5992eb 100644
>>>>>> --- a/qapi/block-export.json
>>>>>> +++ b/qapi/block-export.json
>>>>>> @@ -179,12 +179,18 @@
>>>>>>   #     mount the export with allow_other, and if that fails, try again
>>>>>>   #     without.  (since 6.1; default: auto)
>>>>>>   #
>>>>>> +# @iothreads: Enables multi-threading: Handle requests in each of the
>>>>>> +#     given iothreads (instead of the block device's iothread, or the
>>>>>> +#     export's "main" iothread).
>>>>> When does "the block device's iothread" apply, and when "the export's
>>>>> main iothread"?
>>>> Depends on where you set the iothread option.
>>> Assuming QMP users need to know (see right below), can we trust they
>>> understand which one applies when?  If not, can we provide clues?
>> I don’t understand what exactly you mean, but which one applies when has nothing to do with this option, but with the @iothread (and @fixed-iothread) option(s) on BlockExportOptions, which do document this.
> Can you point me to the spot?

Sure: 
https://www.qemu.org/docs/master/interop/qemu-qmp-ref.html#object-QMP-block-export.BlockExportOptions 
(iothread and fixed-iothread)

>
>>>>> Is this something the QMP user needs to know?
>>>> I think so, because e.g. if you set iothread on the device and the export, you’ll get a conflict.  But if you set it there and set this option, you won’t.  This option will just override the device/export option.
>>> Do we think the doc comment sufficient for QMP users to figure this out?
>> As for conflict, BlockExportOptions.iothread and BlockExportOptions.fixed-iothread do.
>>
>> As for overriding, I do think so.  Do you not?  I’m always open to suggestions.
>>
>>> If not, can we provide clues?
>>>
>>> In particular, do we think they can go from an export failure to the
>>> setting @iothreads here?  Perhaps the error message will guide them.
>>> What is the message?
>> I don’t understand what failure you mean.
> You wrote "you'll get a conflict".  I assume this manifests as failure
> of a QMP command (let's ignore CLI to keep things simple here).

If you set the @iothread option on both the (guest) device and the 
export (and also @fixed-iothread on the export), then you’ll get an 
error.  Nothing to do with this new @iothreads option here.

> Do we think ordinary users running into that failure can figure out they
> can avoid it by setting @iothreads?

It shouldn’t affect the failure.  Setting @iothread on both device and 
export (with @fixed-iothread) will always cause an error, and should.  
Setting this option is not supposed to “fix” that configuration error.

Theoretically, setting @iothreads here could make it so that 
BlockExportOptions.iothread (and/or fixed-iothread) is ignored, because 
that thread will no longer be used for export-issued I/O; but in 
practice, setting that option (BlockExportOptions.iothread) moves that 
export and the whole BDS tree behind it to that I/O thread, so if you 
haven’t specified an I/O thread on the guest device, the guest device 
will then use that thread.  So making @iothreads silently completely 
ignore BlockExportOptions.iothread may cause surprising behavior.

Maybe we could make setting @iothreads here and the generic 
BlockExportOptions.iothread at the same time an error.  That would save 
us the explanation here.

> What's that failure's error message?

$ echo '{"execute":"qmp_capabilities"}
{"execute":"block-export-add",
  "arguments":{"type":"fuse",
"id":"exp",
"node-name":"null",
"mountpoint":"/tmp/fuse-export",
"iothread":"iothr1",
               "fixed-iothread":true}}' |
build/qemu-system-x86_64 \
     -object iothread,id=iothr0 \
     -object iothread,id=iothr1 \
     -blockdev null-co,node-name=null \
     -device virtio-blk,drive=null,iothread=iothr0 \
     -qmp stdio

{"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 9}, 
"package": "v10.0.0-rc1"}, "capabilities": ["oob"]}}
{"return": {}}
{"error": {"class": "GenericError", "desc": "Cannot change iothread of 
active block backend"}}

>
>>>>>> +#                                 For this, the FUSE FD is duplicated so
>>>>>> +#     there is one FD per iothread.  (since 10.1)
>>>>> Is the file descriptor duplication something the QMP user needs to know?
>>>> I found this technical detail interesting, i.e. how multiqueue is implemented for FUSE.  Compare virtio devices, for which we make it clear that virtqueues are mapped to I/O threads (not just in documentation, but actually in option naming).  Is it something they must not know?
>>> Interesting to whom?
>>>
>>> Users of QMP?  Then explaining it in the doc comment (and thus the QEMU
>>> QMP Reference Manual) is proper.
>> Yes, QEMU users.  I find this information interesting to users because virtio explains how multiqueue works there (see IOThreadVirtQueueMapping in virtio.json), and this explains that for FUSE exports, there are no virt queues, but requests come from that FD, which explains implicitly why this doesn’t use the IOThreadVirtQueueMapping type.
>>
>> In fact, if anything, I would even expand on the explanation to say that requests are generally distributed in a round-robin fashion across FUSE FDs regardless of where they originate from, contrasting with virtqueues, which are generally tied to vCPUs.
> Up to you.  I lack context to judge.
>
> Making yourself or your fellow experts understand how this works is not
> the same as making users understand how to use it.  Making me understand
> is not the same either, but it might be closer.

This part of the documentation would concern itself less with “how to 
use it”, and more “when to use it”: This round-robin distribution of 
requests across FDs means that even if I/O is run in a single thread, 
using multiple threads for the export may improve performance (as shown 
in the commit message) – in contrast to virtqueue-based systems.  So I 
think that’s important information to users.

Hanna

>
> If this isn't useful to you, let me know, and I'll shut up :)
>
>> Hanna
>>
>>> Just developers?  Then the doc comment is the wrong spot.
>>>
>>> The QEMU QMP Reference Manual is for users of QMP.  It's dense reading.
>>> Information the users are not expected to need / understand makes that
>>> worse.
>>>
>>>> Hanna
>>>>
>>>>>> +#
>>>>>>     # Since: 6.0
>>>>>>     ##
>>>>>>     { 'struct': 'BlockExportOptionsFuse',
>>>>>>       'data': { 'mountpoint': 'str',
>>>>>>                 '*growable': 'bool',
>>>>>> -            '*allow-other': 'FuseExportAllowOther' },
>>>>>> +            '*allow-other': 'FuseExportAllowOther',
>>>>>> +            '*iothreads': ['str'] },
>>>>>>       'if': 'CONFIG_FUSE' }
>>>>>>       ##
>>>>> [...]
>>>>>