block/io_uring: revert "Use io_uring_register_ring_fd() to skip fd operations"

[PATCH] block/io_uring: revert "Use io_uring_register_ring_fd() to skip fd operations"

Posted by Sam Li 1 year, 7 months ago

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1193

The commit "Use io_uring_register_ring_fd() to skip fd operations" broke
when booting a guest with iothread and io_uring. That is because the
io_uring_register_ring_fd() call is made from the main thread instead of
IOThread where io_uring_submit() is called. It can not be guaranteed
to register the ring fd in the correct thread or unregister the same ring
fd if the IOThread is disabled. This optimization is not critical so we
will revert previous commit.

This reverts commit e2848bc574fe2715c694bf8fe9a1ba7f78a1125a
and 77e3f038af1764983087e3551a0fde9951952c4d.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
 block/io_uring.c | 13 +------------
 meson.build      |  1 -
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/block/io_uring.c b/block/io_uring.c
index a1760152e0..973e15d876 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -11,7 +11,6 @@
 #include "qemu/osdep.h"
 #include <liburing.h>
 #include "block/aio.h"
-#include "qemu/error-report.h"
 #include "qemu/queue.h"
 #include "block/block.h"
 #include "block/raw-aio.h"
@@ -19,7 +18,6 @@
 #include "qapi/error.h"
 #include "trace.h"
 
-
 /* io_uring ring size */
 #define MAX_ENTRIES 128
 
@@ -432,17 +430,8 @@ LuringState *luring_init(Error **errp)
     }
 
     ioq_init(&s->io_q);
-#ifdef CONFIG_LIBURING_REGISTER_RING_FD
-    if (io_uring_register_ring_fd(&s->ring) < 0) {
-        /*
-         * Only warn about this error: we will fallback to the non-optimized
-         * io_uring operations.
-         */
-        warn_report("failed to register linux io_uring ring file descriptor");
-    }
-#endif
-
     return s;
+
 }
 
 void luring_cleanup(LuringState *s)
diff --git a/meson.build b/meson.build
index 3885fc1076..63cfb844cf 100644
--- a/meson.build
+++ b/meson.build
@@ -1793,7 +1793,6 @@ config_host_data.set('CONFIG_LIBNFS', libnfs.found())
 config_host_data.set('CONFIG_LIBSSH', libssh.found())
 config_host_data.set('CONFIG_LINUX_AIO', libaio.found())
 config_host_data.set('CONFIG_LINUX_IO_URING', linux_io_uring.found())
-config_host_data.set('CONFIG_LIBURING_REGISTER_RING_FD', cc.has_function('io_uring_register_ring_fd', prefix: '#include <liburing.h>', dependencies:linux_io_uring))
 config_host_data.set('CONFIG_LIBPMEM', libpmem.found())
 config_host_data.set('CONFIG_NUMA', numa.found())
 config_host_data.set('CONFIG_OPENGL', opengl.found())
-- 
2.37.3

Re: [PATCH] block/io_uring: revert "Use io_uring_register_ring_fd() to skip fd operations"

Posted by Kevin Wolf 1 year, 6 months ago

Am 24.09.2022 um 16:48 hat Sam Li geschrieben:
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1193
> 
> The commit "Use io_uring_register_ring_fd() to skip fd operations" broke
> when booting a guest with iothread and io_uring. That is because the
> io_uring_register_ring_fd() call is made from the main thread instead of
> IOThread where io_uring_submit() is called. It can not be guaranteed
> to register the ring fd in the correct thread or unregister the same ring
> fd if the IOThread is disabled. This optimization is not critical so we
> will revert previous commit.
> 
> This reverts commit e2848bc574fe2715c694bf8fe9a1ba7f78a1125a
> and 77e3f038af1764983087e3551a0fde9951952c4d.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>

Thanks, applied to the block branch.

Kevin

Re: [PATCH] block/io_uring: revert "Use io_uring_register_ring_fd() to skip fd operations"

Posted by Stefano Garzarella 1 year, 7 months ago

On Sat, Sep 24, 2022 at 10:48:15PM +0800, Sam Li wrote:
>Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1193
>
>The commit "Use io_uring_register_ring_fd() to skip fd operations" broke
>when booting a guest with iothread and io_uring. That is because the
>io_uring_register_ring_fd() call is made from the main thread instead of
>IOThread where io_uring_submit() is called. It can not be guaranteed
>to register the ring fd in the correct thread or unregister the same ring
>fd if the IOThread is disabled. This optimization is not critical so we
>will revert previous commit.

Right, maybe we can call it on the first submit.

>
>This reverts commit e2848bc574fe2715c694bf8fe9a1ba7f78a1125a
>and 77e3f038af1764983087e3551a0fde9951952c4d.

Yep, better to revert for now:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>


Should we queue this for stable?

Thanks,
Stefano

>
>Signed-off-by: Sam Li <faithilikerun@gmail.com>
>---
> block/io_uring.c | 13 +------------
> meson.build      |  1 -
> 2 files changed, 1 insertion(+), 13 deletions(-)
>
>diff --git a/block/io_uring.c b/block/io_uring.c
>index a1760152e0..973e15d876 100644
>--- a/block/io_uring.c
>+++ b/block/io_uring.c
>@@ -11,7 +11,6 @@
> #include "qemu/osdep.h"
> #include <liburing.h>
> #include "block/aio.h"
>-#include "qemu/error-report.h"
> #include "qemu/queue.h"
> #include "block/block.h"
> #include "block/raw-aio.h"
>@@ -19,7 +18,6 @@
> #include "qapi/error.h"
> #include "trace.h"
>
>-
> /* io_uring ring size */
> #define MAX_ENTRIES 128
>
>@@ -432,17 +430,8 @@ LuringState *luring_init(Error **errp)
>     }
>
>     ioq_init(&s->io_q);
>-#ifdef CONFIG_LIBURING_REGISTER_RING_FD
>-    if (io_uring_register_ring_fd(&s->ring) < 0) {
>-        /*
>-         * Only warn about this error: we will fallback to the non-optimized
>-         * io_uring operations.
>-         */
>-        warn_report("failed to register linux io_uring ring file descriptor");
>-    }
>-#endif
>-
>     return s;
>+
> }
>
> void luring_cleanup(LuringState *s)
>diff --git a/meson.build b/meson.build
>index 3885fc1076..63cfb844cf 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -1793,7 +1793,6 @@ config_host_data.set('CONFIG_LIBNFS', libnfs.found())
> config_host_data.set('CONFIG_LIBSSH', libssh.found())
> config_host_data.set('CONFIG_LINUX_AIO', libaio.found())
> config_host_data.set('CONFIG_LINUX_IO_URING', linux_io_uring.found())
>-config_host_data.set('CONFIG_LIBURING_REGISTER_RING_FD', cc.has_function('io_uring_register_ring_fd', prefix: '#include <liburing.h>', dependencies:linux_io_uring))
> config_host_data.set('CONFIG_LIBPMEM', libpmem.found())
> config_host_data.set('CONFIG_NUMA', numa.found())
> config_host_data.set('CONFIG_OPENGL', opengl.found())
>-- 
>2.37.3
>

Re: [PATCH] block/io_uring: revert "Use io_uring_register_ring_fd() to skip fd operations"

Posted by Dario Faggioli 1 year, 7 months ago

Yes, we did hit this bug as well, in the QEMU 7.1 package, for openSUSE
Tumbleweed (more info
here: https://bugzilla.suse.com/show_bug.cgi?id=1204082)

FWIW, I can confirm that applying this patch fixes the issue, so this
can have:

On Sat, 2022-09-24 at 22:48 +0800, Sam Li wrote:
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1193
> 
> [...]
>
> This reverts commit e2848bc574fe2715c694bf8fe9a1ba7f78a1125a
> and 77e3f038af1764983087e3551a0fde9951952c4d.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
>
Tested-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)