Am 27.03.2025 um 14:45 hat Hanna Czenczek geschrieben:
> On 27.03.25 13:18, Markus Armbruster wrote:
> > Hanna Czenczek <hre...@redhat.com> writes:
> >
> > > On 26.03.25 12:41, Markus Armbruster wrote:
> > > > Hanna Czenczek <hre...@redhat.com> writes:
> > > >
> > > > > On 26.03.25 06:38, Markus Armbruster wrote:
> > > > > > Hanna Czenczek <hre...@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 <hre...@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
