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 <kw...@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
pgp3toL5UXu9x.pgp
Description: PGP signature
