| Message ID | 20191025160444.31632-4-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: add Linux io_uring AIO engine | expand |
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
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
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
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)