Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Eric Blake]

On 09/22/2016 05:25 AM, Kevin Wolf wrote:

>>>  
>>> +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
>>> +{
>>> +const char *aio = qemu_opt_get(opts, "aio");
>>> +if (!aio) {
>>> +return !!(flags & BDRV_O_NATIVE_AIO);
>>> +} else if (!strcmp(aio, "native")) {
>>> +return true;
>>> +} else if (!strcmp(aio, "threads")) {
>>> +return false;
>>> +}
>>> +
>>> +error_setg(errp, "invalid aio option");
>>> +return false;
>>> +}
>>
>> Is there somewhere common to share this, to avoid duplication?
> 
> I don't know where I would put it. This is a driver-specific option, so
> it doesn't belong in the generic block layer. It's just that two drivers
> happen to provide the same option currently. If we add another backend
> to raw-posix, raw-win32 wouldn't get the new option, so maybe leaving
> them separate is the best anyway.
> 

Fair enough...

> I guess I could do something like this to make the "duplicated" code
> look somewhat smaller, or at least condensed into a single statement:
> 
> BlockdevAioOptions aio =
> qapi_enum_parse(BlockdevAioOptions_lookup,
> qemu_opt_get(opts, "aio"),
> BLOCKDEV_AIO_OPTIONS__MAX,
> (flags & BDRV_O_NATIVE_AIO) ?
> BLOCKDEV_AIO_OPTIONS_NATIVE :
> BLOCKDEV_AIO_OPTIONS_THREADS);
> s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
> 
> Would you consider this an improvement?

Yes, that looks nicer, because it's not hand-rolling the parse. If, in
the future, we do diverge and add a mode in posix that is not available
in win32, that just means we would have two separate QAPI enums, but
keep the qapi_enum_parse() in place for no additional if/else branches.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Kevin Wolf]

Am 21.09.2016 um 00:27 hat Eric Blake geschrieben:
> On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> > The option whether or not to use a native AIO interface really isn't a
> > generic option for all drivers, but only applies to the native file
> > protocols. This patch moves the option in blockdev-add to the
> > appropriate places (raw-posix and raw-win32).
> > 
> > We still have to keep the flag BDRV_O_NATIVE_AIO for compatibility
> > because so far the AIO option was usually specified on the wrong layer
> > (the top-level format driver, which didn't even look at it) and then
> > inherited by the protocol driver (where it was actually used). We can't
> > forbid this use except in new interfaces.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/raw-posix.c  | 45 -
> >  block/raw-win32.c  | 50 
> > +-
> >  qapi/block-core.json   |  6 +++---
> >  tests/qemu-iotests/087 |  4 ++--
> >  4 files changed, 78 insertions(+), 27 deletions(-)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 6ed7547..beb86a3 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -143,6 +143,7 @@ typedef struct BDRVRawState {
> >  bool has_discard:1;
> >  bool has_write_zeroes:1;
> >  bool discard_zeroes:1;
> > +bool use_linux_aio:1;
> >  bool has_fallocate;
> >  bool needs_alignment;
> 
> Not this patch's concern, but why are some of our bools packed in a
> bitfield and others used with a full byte?  Does the performance vs.
> size tradeoff even matter for any of the members concerned?

Probably not. Also, we have 5 bytes of padding at the end of this
struct, so not using a bitfield would make it exactly the same size. And
if we care about the size, we are already wasting 4 bytes with the
misaligned size_t buf_align.

Anyway, I just did what most other fields are doing.

> >  } BDRVRawState;
> > @@ -367,18 +368,6 @@ static void raw_parse_flags(int bdrv_flags, int 
> > *open_flags)
> >  }
> >  }
> >  
> > -#ifdef CONFIG_LINUX_AIO
> > -static bool raw_use_aio(int bdrv_flags)
> > -{
> > -/*
> > - * Currently Linux do AIO only for files opened with O_DIRECT
> > - * specified so check NOCACHE flag too
> 
> Nice; we're getting rid of some awkward grammar.
> 
> > @@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> > *options,
> >  goto fail;
> >  }
> >  
> > +aio = qemu_opt_get(opts, "aio");
> > +if (!aio) {
> > +s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
> > +} else if (!strcmp(aio, "native")) {
> > +s->use_linux_aio = true;
> > +} else if (!strcmp(aio, "threads")) {
> > +s->use_linux_aio = false;
> > +} else {
> > +   error_setg(errp, "invalid aio option");
> > +   ret = -EINVAL;
> > +   goto fail;
> > +}
> > +
> 
> > +++ b/block/raw-win32.c
> 
> >  
> > +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
> > +{
> > +const char *aio = qemu_opt_get(opts, "aio");
> > +if (!aio) {
> > +return !!(flags & BDRV_O_NATIVE_AIO);
> > +} else if (!strcmp(aio, "native")) {
> > +return true;
> > +} else if (!strcmp(aio, "threads")) {
> > +return false;
> > +}
> > +
> > +error_setg(errp, "invalid aio option");
> > +return false;
> > +}
> 
> Is there somewhere common to share this, to avoid duplication?

I don't know where I would put it. This is a driver-specific option, so
it doesn't belong in the generic block layer. It's just that two drivers
happen to provide the same option currently. If we add another backend
to raw-posix, raw-win32 wouldn't get the new option, so maybe leaving
them separate is the best anyway.

I guess I could do something like this to make the "duplicated" code
look somewhat smaller, or at least condensed into a single statement:

BlockdevAioOptions aio =
qapi_enum_parse(BlockdevAioOptions_lookup,
qemu_opt_get(opts, "aio"),
BLOCKDEV_AIO_OPTIONS__MAX,
(flags & BDRV_O_NATIVE_AIO) ?
BLOCKDEV_AIO_OPTIONS_NATIVE :
BLOCKDEV_AIO_OPTIONS_THREADS);
s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);

Would you consider this an improvement?

Kevin


pgp3toL5UXu9x.pgp
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Eric Blake]

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> The option whether or not to use a native AIO interface really isn't a
> generic option for all drivers, but only applies to the native file
> protocols. This patch moves the option in blockdev-add to the
> appropriate places (raw-posix and raw-win32).
> 
> We still have to keep the flag BDRV_O_NATIVE_AIO for compatibility
> because so far the AIO option was usually specified on the wrong layer
> (the top-level format driver, which didn't even look at it) and then
> inherited by the protocol driver (where it was actually used). We can't
> forbid this use except in new interfaces.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/raw-posix.c  | 45 -
>  block/raw-win32.c  | 50 
> +-
>  qapi/block-core.json   |  6 +++---
>  tests/qemu-iotests/087 |  4 ++--
>  4 files changed, 78 insertions(+), 27 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6ed7547..beb86a3 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -143,6 +143,7 @@ typedef struct BDRVRawState {
>  bool has_discard:1;
>  bool has_write_zeroes:1;
>  bool discard_zeroes:1;
> +bool use_linux_aio:1;
>  bool has_fallocate;
>  bool needs_alignment;

Not this patch's concern, but why are some of our bools packed in a
bitfield and others used with a full byte?  Does the performance vs.
size tradeoff even matter for any of the members concerned?

>  } BDRVRawState;
> @@ -367,18 +368,6 @@ static void raw_parse_flags(int bdrv_flags, int 
> *open_flags)
>  }
>  }
>  
> -#ifdef CONFIG_LINUX_AIO
> -static bool raw_use_aio(int bdrv_flags)
> -{
> -/*
> - * Currently Linux do AIO only for files opened with O_DIRECT
> - * specified so check NOCACHE flag too

Nice; we're getting rid of some awkward grammar.

> @@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>  goto fail;
>  }
>  
> +aio = qemu_opt_get(opts, "aio");
> +if (!aio) {
> +s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
> +} else if (!strcmp(aio, "native")) {
> +s->use_linux_aio = true;
> +} else if (!strcmp(aio, "threads")) {
> +s->use_linux_aio = false;
> +} else {
> +   error_setg(errp, "invalid aio option");
> +   ret = -EINVAL;
> +   goto fail;
> +}
> +

> +++ b/block/raw-win32.c

>  
> +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
> +{
> +const char *aio = qemu_opt_get(opts, "aio");
> +if (!aio) {
> +return !!(flags & BDRV_O_NATIVE_AIO);
> +} else if (!strcmp(aio, "native")) {
> +return true;
> +} else if (!strcmp(aio, "threads")) {
> +return false;
> +}
> +
> +error_setg(errp, "invalid aio option");
> +return false;
> +}

Is there somewhere common to share this, to avoid duplication?

> +++ b/qapi/block-core.json
> @@ -1724,11 +1724,13 @@
>  # Driver specific block device options for the file backend.
>  #
>  # @filename:path to the image file
> +# @aio: #optional AIO backend (default: threads)
>  #
>  # Since: 1.7
>  ##
>  { 'struct': 'BlockdevOptionsFile',
> -  'data': { 'filename': 'str' } }
> +  'data': { 'filename': 'str',
> +'*aio': 'BlockdevAioOptions' } }
>  

> +++ b/tests/qemu-iotests/087
> @@ -117,10 +117,10 @@ run_qemu <"options": {
>  "driver": "$IMGFMT",
>  "node-name": "disk",
> -"aio": "native",
>  "file": {
>  "driver": "file",
> -"filename": "$TEST_IMG"
> +"filename": "$TEST_IMG",
> +"aio": "native"


Looks like a reasonable relocation. And more evidence that blockdev-add
is not yet usable; hopefully we get there in 2.8.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Kevin Wolf]

The option whether or not to use a native AIO interface really isn't a
generic option for all drivers, but only applies to the native file
protocols. This patch moves the option in blockdev-add to the
appropriate places (raw-posix and raw-win32).

We still have to keep the flag BDRV_O_NATIVE_AIO for compatibility
because so far the AIO option was usually specified on the wrong layer
(the top-level format driver, which didn't even look at it) and then
inherited by the protocol driver (where it was actually used). We can't
forbid this use except in new interfaces.

Signed-off-by: Kevin Wolf 
---
 block/raw-posix.c  | 45 -
 block/raw-win32.c  | 50 +-
 qapi/block-core.json   |  6 +++---
 tests/qemu-iotests/087 |  4 ++--
 4 files changed, 78 insertions(+), 27 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6ed7547..beb86a3 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -143,6 +143,7 @@ typedef struct BDRVRawState {
 bool has_discard:1;
 bool has_write_zeroes:1;
 bool discard_zeroes:1;
+bool use_linux_aio:1;
 bool has_fallocate;
 bool needs_alignment;
 } BDRVRawState;
@@ -367,18 +368,6 @@ static void raw_parse_flags(int bdrv_flags, int 
*open_flags)
 }
 }
 
-#ifdef CONFIG_LINUX_AIO
-static bool raw_use_aio(int bdrv_flags)
-{
-/*
- * Currently Linux do AIO only for files opened with O_DIRECT
- * specified so check NOCACHE flag too
- */
-return (bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
- (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO);
-}
-#endif
-
 static void raw_parse_filename(const char *filename, QDict *options,
Error **errp)
 {
@@ -399,6 +388,11 @@ static QemuOptsList raw_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "File name of the image",
 },
+{
+.name = "aio",
+.type = QEMU_OPT_STRING,
+.help = "host AIO implementation (threads, native)",
+},
 { /* end of list */ }
 },
 };
@@ -410,6 +404,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 QemuOpts *opts;
 Error *local_err = NULL;
 const char *filename = NULL;
+const char *aio;
 int fd, ret;
 struct stat st;
 
@@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 goto fail;
 }
 
+aio = qemu_opt_get(opts, "aio");
+if (!aio) {
+s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
+} else if (!strcmp(aio, "native")) {
+s->use_linux_aio = true;
+} else if (!strcmp(aio, "threads")) {
+s->use_linux_aio = false;
+} else {
+   error_setg(errp, "invalid aio option");
+   ret = -EINVAL;
+   goto fail;
+}
+
 s->open_flags = open_flags;
 raw_parse_flags(bdrv_flags, >open_flags);
 
@@ -444,14 +452,15 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 s->fd = fd;
 
 #ifdef CONFIG_LINUX_AIO
-if (!raw_use_aio(bdrv_flags) && (bdrv_flags & BDRV_O_NATIVE_AIO)) {
+ /* Currently Linux does AIO only for files opened with O_DIRECT */
+if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
 error_setg(errp, "aio=native was specified, but it requires "
  "cache.direct=on, which was not specified.");
 ret = -EINVAL;
 goto fail;
 }
 #else
-if (bdrv_flags & BDRV_O_NATIVE_AIO) {
+if (s->use_linux_aio) {
 error_setg(errp, "aio=native was specified, but is not supported "
  "in this build.");
 ret = -EINVAL;
@@ -1256,7 +1265,7 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, 
uint64_t offset,
 if (!bdrv_qiov_is_aligned(bs, qiov)) {
 type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
-} else if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+} else if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
 assert(qiov->size == bytes);
 return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
@@ -1285,7 +1294,8 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 static void raw_aio_plug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
-if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+BDRVRawState *s = bs->opaque;
+if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
 laio_io_plug(bs, aio);
 }
@@ -1295,7 +1305,8 @@ static void raw_aio_plug(BlockDriverState *bs)
 static void raw_aio_unplug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
-if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+BDRVRawState *s = bs->opaque;
+if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));