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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4516 bytes --]

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 <kwolf@redhat.com>
> > ---
> >  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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]