Re: [Qemu-block] [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 <address@hidden>
> ---
>  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 <<EOF
>        "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