[v2] io_uring: add Linux io_uring AIO engine

[PATCH v2 03/15] block/block: add BDRV flag for io_uring

Posted by Stefan Hajnoczi 6 years, 1 month ago

From: Aarushi Mehta <mehta.aaru20@gmail.com>

Signed-off-by: Aarushi Mehta <mehta.aaru20@gmail.com>
Reviewed-by: Maxim Levitsky <maximlevitsky@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/block.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/block/block.h b/include/block/block.h
index 89606bd9f8..bdb48dcd1b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -126,6 +126,7 @@ typedef struct HDGeometry {
                                       ignoring the format layer */
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
 #define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
+#define BDRV_O_IO_URING    0x40000 /* use io_uring instead of the thread pool */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
-- 
2.21.0

Re: [PATCH v2 03/15] block/block: add BDRV flag for io_uring

Posted by Kevin Wolf 6 years, 1 month ago

Am 25.10.2019 um 18:04 hat Stefan Hajnoczi geschrieben:
> From: Aarushi Mehta <mehta.aaru20@gmail.com>
> 
> Signed-off-by: Aarushi Mehta <mehta.aaru20@gmail.com>
> Reviewed-by: Maxim Levitsky <maximlevitsky@gmail.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/block/block.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 89606bd9f8..bdb48dcd1b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -126,6 +126,7 @@ typedef struct HDGeometry {
>                                        ignoring the format layer */
>  #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
>  #define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
> +#define BDRV_O_IO_URING    0x40000 /* use io_uring instead of the thread pool */

Why do we need a new (global!) flag rather than a driver-specific option
for file-posix? This looks wrong to me, the flag has no sensible meaning
for any other driver.

(Yes, BDRV_O_NATIVE_AIO is wrong, too, but compatibility means we can't
remove it easily.)

Kevin

Re: [PATCH v2 03/15] block/block: add BDRV flag for io_uring

Posted by Max Reitz 6 years, 1 month ago

On 11.11.19 11:57, Kevin Wolf wrote:
> Am 25.10.2019 um 18:04 hat Stefan Hajnoczi geschrieben:
>> From: Aarushi Mehta <mehta.aaru20@gmail.com>
>>
>> Signed-off-by: Aarushi Mehta <mehta.aaru20@gmail.com>
>> Reviewed-by: Maxim Levitsky <maximlevitsky@gmail.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/block/block.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 89606bd9f8..bdb48dcd1b 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -126,6 +126,7 @@ typedef struct HDGeometry {
>>                                        ignoring the format layer */
>>  #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
>>  #define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
>> +#define BDRV_O_IO_URING    0x40000 /* use io_uring instead of the thread pool */
> 
> Why do we need a new (global!) flag rather than a driver-specific option
> for file-posix? This looks wrong to me, the flag has no sensible meaning
> for any other driver.
> 
> (Yes, BDRV_O_NATIVE_AIO is wrong, too, but compatibility means we can't
> remove it easily.)

To add to that, Kevin and me had a short talk on IRC, and it seemed like
the reason would be that it’s easier to use this way than an option
would be.

To me, the problem seems to be that it’s only easier to use because of
the option inheritance we have in the block layer (so you can set this
flag on a qcow2 node and its file node will have it, too).  But
naturally this inheritance is a bit of magic (because qemu has to guess
where you probably want the flag to end up), so I’d too rather avoid
adding to it.

OTOH, I wonder whether ease of use is really that important here.  Would
people who want to use io_uring really care about a command line that’s
going to be a bit more complicated than just
-drive file=foo.img,format=$imgfmt,aio=io_uring,if=$interface?  (Namely
file.aio in this case, or maybe even a full-on block graph definition.)

Max

Re: [PATCH v2 03/15] block/block: add BDRV flag for io_uring

Posted by Stefan Hajnoczi 6 years, 1 month ago

On Mon, Nov 11, 2019 at 05:25:04PM +0100, Max Reitz wrote:
> On 11.11.19 11:57, Kevin Wolf wrote:
> > Am 25.10.2019 um 18:04 hat Stefan Hajnoczi geschrieben:
> >> From: Aarushi Mehta <mehta.aaru20@gmail.com>
> >>
> >> Signed-off-by: Aarushi Mehta <mehta.aaru20@gmail.com>
> >> Reviewed-by: Maxim Levitsky <maximlevitsky@gmail.com>
> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> ---
> >>  include/block/block.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/block/block.h b/include/block/block.h
> >> index 89606bd9f8..bdb48dcd1b 100644
> >> --- a/include/block/block.h
> >> +++ b/include/block/block.h
> >> @@ -126,6 +126,7 @@ typedef struct HDGeometry {
> >>                                        ignoring the format layer */
> >>  #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
> >>  #define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
> >> +#define BDRV_O_IO_URING    0x40000 /* use io_uring instead of the thread pool */
> > 
> > Why do we need a new (global!) flag rather than a driver-specific option
> > for file-posix? This looks wrong to me, the flag has no sensible meaning
> > for any other driver.
> > 
> > (Yes, BDRV_O_NATIVE_AIO is wrong, too, but compatibility means we can't
> > remove it easily.)
> 
> To add to that, Kevin and me had a short talk on IRC, and it seemed like
> the reason would be that it’s easier to use this way than an option
> would be.
> 
> To me, the problem seems to be that it’s only easier to use because of
> the option inheritance we have in the block layer (so you can set this
> flag on a qcow2 node and its file node will have it, too).  But
> naturally this inheritance is a bit of magic (because qemu has to guess
> where you probably want the flag to end up), so I’d too rather avoid
> adding to it.
> 
> OTOH, I wonder whether ease of use is really that important here.  Would
> people who want to use io_uring really care about a command line that’s
> going to be a bit more complicated than just
> -drive file=foo.img,format=$imgfmt,aio=io_uring,if=$interface?  (Namely
> file.aio in this case, or maybe even a full-on block graph definition.)

Thanks for the idea.  I agree it's cleaner to avoid a new global flag.

I'll take a look at fixing this and let you know if I hit any problems.

Stefan