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

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

Posted by Dirk Müller 2 years ago

This allows $qemu --help runtime configure checks for detecting
the host support.

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] Only advertise aio=io_uring if support is actually available

Posted by Daniel P. Berrangé 2 years ago

On Tue, Apr 19, 2022 at 07:19:31PM +0200, Dirk Müller wrote:
> This allows $qemu --help runtime configure checks for detecting
> the host support.

Note: detecting features by parsing --help output is something that
is explicitly a non-goal for QEMU. The only supported interface for
detecting features is QMP. We can't actively prevent people writing
code that parses --help, but if such parsing breaks on arrival of
new QEMU releases that is not considered a bug on the QEMU side.

That all said, the patch itself is OK, because for human targetted
interactive usage, it is desirable for --help to be representative
of what's available.

IOW, I'm just complaining about the commit message justification
here & warning against writing tools to use --help :-)

> 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

If we're going to conditionalize this, then we really ought to be
address it fully, because 'native' is also platform specific.

IOW, we would end up needing something more like this:

           .help = "host AIO implementation (threads"
 #if defined(WIN32) || defined(CONFIG_LINUX_AIO)
                   ", native"
 #endif
 #if defined(CONFIG_LINUX_IO_URING)
                   ", io_uring"
 #else
                   "),"

admittedly pretty ugly

>          },
>          {
>              .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
> 
> 

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] Only advertise aio=io_uring if support is actually available

Posted by Dirk Müller 1 year, 12 months ago

On Mittwoch, 20. April 2022 10:33:38 CEST Daniel P. Berrangé wrote:

Hi Daniel,

> If we're going to conditionalize this, then we really ought to be
> address it fully, because 'native' is also platform specific.

Good point. I can do that as well. 

> IOW, we would end up needing something more like this:
> 
>            .help = "host AIO implementation (threads"
>  #if defined(WIN32) || defined(CONFIG_LINUX_AIO)
>                    ", native"
>  #endif
>  #if defined(CONFIG_LINUX_IO_URING)
>                    ", io_uring"
>  #else
>                    "),"
> 
> admittedly pretty ugly


the only other option I came up with so far is something like this:

+#if defined(CONFIG_LINUX_AIO)
+#define IF_CONFIG_LINUX_AIO(X) X
+#else
+#define IF_CONFIG_LINUX_AIO(X)
+#endif
+
+#if defined(CONFIG_LINUX_IO_URING)
+#define IF_CONFIG_LINUX_IO_URING(X) X
+#else
+#define IF_CONFIG_LINUX_IO_URING(X)
+#endif
+


and then use it like


-            .help = "host AIO implementation (threads"
-#ifdef CONFIG_LINUX_AIO
-            ", native"
-#endif
-#ifdef CONFIG_LINUX_IO_URING
-            ", io_uring"
-#endif
-            ")",
+            .help = "host AIO implementation (threads" IF_CONFIG_LINUX_AIO(", 
native") IF_CONFIG_LINUX_IO_URING(", io_uring") ")",


WDYT, is that worth it?

Thanks,
Dirk

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

Posted by Dirk Müller 2 years ago

On Mittwoch, 20. April 2022 10:33:38 CEST Daniel P. Berrangé wrote:

Hi Daniel,

> That all said, the patch itself is OK, because for human targetted
> interactive usage, it is desirable for --help to be representative
> of what's available.
> 
> IOW, I'm just complaining about the commit message justification
> here & warning against writing tools to use --help :-)

understood, thank you for the good feedback, I've sent a v2 with a reworded 
commit message. 

Greetings,
Dirk

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

Posted by Eric Blake 2 years ago

On Tue, Apr 19, 2022 at 07:19:31PM +0200, Dirk Müller wrote:
> This allows $qemu --help runtime configure checks for detecting
> the host support.

--help is human-parseable, and thus not stable text.  Relying on it
for detecting host support is not always ideal, but anything is better
than nothing, so I'm not opposed to this patch.

Reviewed-by: Eric Blake <eblake@redhat.com>

Since qemu-nbd is affected, I'm happy to queue this through my NBD
tree now that 7.1 development has opened, if no one else queues it
elsewhere first.

> 
> 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