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

[Kevin Wolf <kwolf@redhat.com>]

Am 27.03.2025 um 14:45 hat Hanna Czenczek geschrieben:
> 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.

This raises the question if the better interface wouldn't be to change
the BlockExportOptions.iothread from 'str' to an alternate between 'str'
and ['str'], allowing the user to specify multiple iothreads in the
already existing related option without creating conflicting options.

In the long run, I would be surprised if FUSE remained the only export
supporting multiple iothreads. At least the virtio based ones
(vhost-user-blk and VDUSE) even have precedence in the virtio-blk device
itself, so while I don't know how much interest there is in actually
implementing it, in theory we know it makes sense.

Kevin