[v2] Only advertise aio=io_uring if support is actually available

[PATCH v2] Only advertise aio=io_uring if support is actually available

Posted by Dirk Müller 2 years ago

Change --help output for aio option to only list the aio backend options that
are actually available. io_uring is an optional, linux only backend
option so hide it for cases where it isn't there.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Dirk Müller <dmueller@suse.de>
---
 block/file-posix.c | 4 ++++
 qemu-nbd.c         | 4 ++++
 qemu-options.hx    | 6 +++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 39a3d6dbe6..aec4763862 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -544,7 +544,11 @@ static QemuOptsList raw_runtime_opts = {
         {
             .name = "aio",
             .type = QEMU_OPT_STRING,
+#ifdef CONFIG_LINUX_IO_URING
             .help = "host AIO implementation (threads, native, io_uring)",
+#else
+            .help = "host AIO implementation (threads, native)",
+#endif
         },
         {
             .name = "aio-max-batch",
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 713e7557a9..4634a0fc42 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -147,7 +147,11 @@ static void usage(const char *name)
 "      --cache=MODE          set cache mode used to access the disk image, the\n"
 "                            valid options are: 'none', 'writeback' (default),\n"
 "                            'writethrough', 'directsync' and 'unsafe'\n"
+#ifdef CONFIG_LINUX_IO_URING
 "      --aio=MODE            set AIO mode (native, io_uring or threads)\n"
+#else
+"      --aio=MODE            set AIO mode (native or threads)\n"
+#endif
 "      --discard=MODE        set discard mode (ignore, unmap)\n"
 "      --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
 "      --image-opts          treat FILE as a full set of image options\n"
diff --git a/qemu-options.hx b/qemu-options.hx
index 34e9b32a5c..973125cfca 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1338,7 +1338,11 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,snapshot=on|off][,rerror=ignore|stop|report]\n"
     "       [,werror=ignore|stop|report|enospc][,id=name]\n"
-    "       [,aio=threads|native|io_uring]\n"
+    "       [,aio=threads|native"
+#if defined(CONFIG_LINUX_IO_URING)
+    "|io_uring"
+#endif
+    "]\n"
     "       [,readonly=on|off][,copy-on-read=on|off]\n"
     "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
     "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
-- 
2.35.3

Re: [PATCH v2] Only advertise aio=io_uring if support is actually available

Posted by Eric Blake 2 years ago

On Thu, Apr 21, 2022 at 06:50:48PM +0200, Dirk Müller wrote:
> Change --help output for aio option to only list the aio backend options that
> are actually available. io_uring is an optional, linux only backend
> option so hide it for cases where it isn't there.

As pointed out by Dan, this commit message is not quite accurate.  It
hides only one of the two conditional options, but 'native' is also a
Linux-only optional backend (CONFIG_LINUX_AIO).

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Dirk Müller <dmueller@suse.de>
> ---
>  block/file-posix.c | 4 ++++
>  qemu-nbd.c         | 4 ++++
>  qemu-options.hx    | 6 +++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] Only advertise aio=io_uring if support is actually available

Posted by Eric Blake 2 years ago

On Thu, Apr 21, 2022 at 02:27:55PM -0500, Eric Blake wrote:
> On Thu, Apr 21, 2022 at 06:50:48PM +0200, Dirk Müller wrote:
> > Change --help output for aio option to only list the aio backend options that
> > are actually available. io_uring is an optional, linux only backend
> > option so hide it for cases where it isn't there.
> 
> As pointed out by Dan, this commit message is not quite accurate.  It
> hides only one of the two conditional options, but 'native' is also a
> Linux-only optional backend (CONFIG_LINUX_AIO).

Stepping back a bit - we already said that making --help
machine-parseable is a non-goal, and so the intent of this patch is
for human readers.  But adding an #ifdef ladder to show all 4 possible
combinations gets hairy; adding another option turns it into 8
combinations.  Is there a better way?

What if we just document ALL strings possible in at least one build
(no #ifdef ladder in the help text), but add --aio=help as a way to do
a runtime list of which aio modes are understood by THIS build?  It
would match how we have --device=help for qemu proper, and may even be
able to reuse some of that framework code (for parsing out when help
is requested).

Yes, that would be a bigger patch, but it may also be easier to
maintain down the road.  And even though there is an #ifdef ladder at
runtime, it only has O(n) growth rather than O(n^2) for each possible
combination of which options are enabled, and would appear only once
in the runtime rather than duplicated across each place which
documents similar help text across multiple utilities.

> 
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Dirk Müller <dmueller@suse.de>
> > ---
> >  block/file-posix.c | 4 ++++
> >  qemu-nbd.c         | 4 ++++
> >  qemu-options.hx    | 6 +++++-
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> > 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] Only advertise aio=io_uring if support is actually available

Posted by Daniel P. Berrangé 1 year, 12 months ago

On Fri, Apr 22, 2022 at 03:30:46PM -0500, Eric Blake wrote:
> On Thu, Apr 21, 2022 at 02:27:55PM -0500, Eric Blake wrote:
> > On Thu, Apr 21, 2022 at 06:50:48PM +0200, Dirk Müller wrote:
> > > Change --help output for aio option to only list the aio backend options that
> > > are actually available. io_uring is an optional, linux only backend
> > > option so hide it for cases where it isn't there.
> > 
> > As pointed out by Dan, this commit message is not quite accurate.  It
> > hides only one of the two conditional options, but 'native' is also a
> > Linux-only optional backend (CONFIG_LINUX_AIO).
> 
> Stepping back a bit - we already said that making --help
> machine-parseable is a non-goal, and so the intent of this patch is
> for human readers.  But adding an #ifdef ladder to show all 4 possible
> combinations gets hairy; adding another option turns it into 8
> combinations.  Is there a better way
> 
> What if we just document ALL strings possible in at least one build
> (no #ifdef ladder in the help text), but add --aio=help as a way to do
> a runtime list of which aio modes are understood by THIS build?  It
> would match how we have --device=help for qemu proper, and may even be
> able to reuse some of that framework code (for parsing out when help
> is requested).

Did you literally mean '--aio=help' ?  'aio' is just an option
for disks, and I don't think we want new top levelk options. So
would have to be '-drive ....,aio=help', or '--blockdev ....aio=help'
the latter gets tricky with nested json syntax though, so not sure
it is nicer.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] Only advertise aio=io_uring if support is actually available

Posted by Eric Blake 1 year, 12 months ago

On Mon, Apr 25, 2022 at 12:19:08PM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 22, 2022 at 03:30:46PM -0500, Eric Blake wrote:
> > On Thu, Apr 21, 2022 at 02:27:55PM -0500, Eric Blake wrote:
> > > On Thu, Apr 21, 2022 at 06:50:48PM +0200, Dirk Müller wrote:
> > > > Change --help output for aio option to only list the aio backend options that
> > > > are actually available. io_uring is an optional, linux only backend
> > > > option so hide it for cases where it isn't there.
> > > 
> > > As pointed out by Dan, this commit message is not quite accurate.  It
> > > hides only one of the two conditional options, but 'native' is also a
> > > Linux-only optional backend (CONFIG_LINUX_AIO).
> > 
> > Stepping back a bit - we already said that making --help
> > machine-parseable is a non-goal, and so the intent of this patch is
> > for human readers.  But adding an #ifdef ladder to show all 4 possible
> > combinations gets hairy; adding another option turns it into 8
> > combinations.  Is there a better way
> > 
> > What if we just document ALL strings possible in at least one build
> > (no #ifdef ladder in the help text), but add --aio=help as a way to do
> > a runtime list of which aio modes are understood by THIS build?  It
> > would match how we have --device=help for qemu proper, and may even be
> > able to reuse some of that framework code (for parsing out when help
> > is requested).
> 
> Did you literally mean '--aio=help' ?  'aio' is just an option
> for disks, and I don't think we want new top levelk options. So
> would have to be '-drive ....,aio=help', or '--blockdev ....aio=help'
> the latter gets tricky with nested json syntax though, so not sure
> it is nicer.

For qemu-nbd, it literally is:

      --aio=MODE            set AIO mode (native, io_uring or threads)

so adding --aio=help is already a top-level option there.  But for
other utilities, you are right that it is a per-device option, and not
a top-level thing (qemu-nbd is weird because it exposes exactly one
device).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org