| Message ID | 1474405737-2092-4-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? > } 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.
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
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.
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, &s->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)); laio_io_unplug(bs, aio); } diff --git a/block/raw-win32.c b/block/raw-win32.c index 56f45fe..61bb78e 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -252,7 +252,8 @@ static void raw_probe_alignment(BlockDriverState *bs, Error **errp) } } -static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) +static void raw_parse_flags(int flags, bool use_aio, int *access_flags, + DWORD *overlapped) { assert(access_flags != NULL); assert(overlapped != NULL); @@ -264,7 +265,7 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped) } *overlapped = FILE_ATTRIBUTE_NORMAL; - if (flags & BDRV_O_NATIVE_AIO) { + if (use_aio) { *overlapped |= FILE_FLAG_OVERLAPPED; } if (flags & BDRV_O_NOCACHE) { @@ -292,10 +293,30 @@ 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 */ } }, }; +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; +} + static int raw_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { @@ -305,6 +326,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, QemuOpts *opts; Error *local_err = NULL; const char *filename; + bool use_aio; int ret; s->type = FTYPE_FILE; @@ -319,7 +341,14 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, filename = qemu_opt_get(opts, "filename"); - raw_parse_flags(flags, &access_flags, &overlapped); + use_aio = get_aio_option(opts, flags, &local_err); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + raw_parse_flags(flags, use_aio, &access_flags, &overlapped); if (filename[0] && filename[1] == ':') { snprintf(s->drive_path, sizeof(s->drive_path), "%c:\\", filename[0]); @@ -346,7 +375,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } - if (flags & BDRV_O_NATIVE_AIO) { + if (use_aio) { s->aio = win32_aio_init(); if (s->aio == NULL) { CloseHandle(s->hfile); @@ -647,6 +676,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, Error *local_err = NULL; const char *filename; + bool use_aio; QemuOpts *opts = qemu_opts_create(&raw_runtime_opts, NULL, 0, &error_abort); @@ -659,6 +689,16 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, filename = qemu_opt_get(opts, "filename"); + use_aio = get_aio_option(opts, flags, &local_err); + if (!local_err && use_aio) { + error_setg(&local_err, "AIO is not supported on Windows host devices"); + } + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto done; + } + if (strstart(filename, "/dev/cdrom", NULL)) { if (find_cdrom(device_name, sizeof(device_name)) < 0) { error_setg(errp, "Could not open CD-ROM drive"); @@ -677,7 +717,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, } s->type = find_device_type(bs, filename); - raw_parse_flags(flags, &access_flags, &overlapped); + raw_parse_flags(flags, use_aio, &access_flags, &overlapped); create_flags = OPEN_EXISTING; diff --git a/qapi/block-core.json b/qapi/block-core.json index 0939011..126b9ca 100644 --- a/qapi/block-core.json +++ 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' } } ## # @BlockdevOptionsNull @@ -2232,7 +2234,6 @@ # This option is required on the top level of blockdev-add. # @discard: #optional discard-related options (default: ignore) # @cache: #optional cache-related options -# @aio: #optional AIO backend (default: threads) # @read-only: #optional whether the block device should be read-only # (default: false) # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) @@ -2247,7 +2248,6 @@ '*node-name': 'str', '*discard': 'BlockdevDiscardOptions', '*cache': 'BlockdevCacheOptions', - '*aio': 'BlockdevAioOptions', '*read-only': 'bool', '*detect-zeroes': 'BlockdevDetectZeroesOptions' }, 'discriminator': 'driver', diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index 5c04577..b1ac71f 100755 --- a/tests/qemu-iotests/087 +++ 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" } } }
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(-)