| Message ID | 20211026162346.253081-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-aio: allow block devices to limit aio-max-batch | expand |
Stefano Garzarella <sgarzare@redhat.com> writes: > Commit d7ddd0a161 ("linux-aio: limit the batch size using > `aio-max-batch` parameter") added a way to limit the batch size > of Linux AIO backend for the entire AIO context. > > The same AIO context can be shared by multiple devices, so > latency-sensitive devices may want to limit the batch size even > more to avoid increasing latency. > > For this reason we add the `aio-max-batch` option to the file > backend, which will be used by the next commits to limit the size of > batches including requests generated by this device. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > Notes: > v2: > - @aio-max-batch documentation rewrite [Stefan, Kevin] > > qapi/block-core.json | 7 +++++++ > block/file-posix.c | 9 +++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 6d3217abb6..fef76b0ea2 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2896,6 +2896,12 @@ > # for this device (default: none, forward the commands via SG_IO; > # since 2.11) > # @aio: AIO backend (default: threads) (since: 2.8) > +# @aio-max-batch: maximum number of requests to batch together into a single > +# submission in the AIO backend. The smallest value between > +# this and the aio-max-batch value of the IOThread object is > +# chosen. > +# 0 means that the AIO backend will handle it automatically. > +# (default: 0, since 6.2) "(default 0) (since 6.2)" seems to be more common. > # @locking: whether to enable file locking. If set to 'auto', only enable > # when Open File Descriptor (OFD) locking API is available > # (default: auto, since 2.10) > @@ -2924,6 +2930,7 @@ > '*pr-manager': 'str', > '*locking': 'OnOffAuto', > '*aio': 'BlockdevAioOptions', > + '*aio-max-batch': 'int', > '*drop-cache': {'type': 'bool', > 'if': 'CONFIG_LINUX'}, > '*x-check-cache-dropped': 'bool' },
On Tue, Oct 26, 2021 at 06:23:44PM +0200, Stefano Garzarella wrote: > Commit d7ddd0a161 ("linux-aio: limit the batch size using > `aio-max-batch` parameter") added a way to limit the batch size > of Linux AIO backend for the entire AIO context. > > The same AIO context can be shared by multiple devices, so > latency-sensitive devices may want to limit the batch size even > more to avoid increasing latency. > > For this reason we add the `aio-max-batch` option to the file > backend, which will be used by the next commits to limit the size of > batches including requests generated by this device. > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > Notes: > v2: > - @aio-max-batch documentation rewrite [Stefan, Kevin] > > qapi/block-core.json | 7 +++++++ > block/file-posix.c | 9 +++++++++ > 2 files changed, 16 insertions(+) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Wed, Oct 27, 2021 at 06:28:51AM +0200, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> writes: > >> Commit d7ddd0a161 ("linux-aio: limit the batch size using >> `aio-max-batch` parameter") added a way to limit the batch size >> of Linux AIO backend for the entire AIO context. >> >> The same AIO context can be shared by multiple devices, so >> latency-sensitive devices may want to limit the batch size even >> more to avoid increasing latency. >> >> For this reason we add the `aio-max-batch` option to the file >> backend, which will be used by the next commits to limit the size of >> batches including requests generated by this device. >> >> Suggested-by: Kevin Wolf <kwolf@redhat.com> >> Reviewed-by: Kevin Wolf <kwolf@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> >> Notes: >> v2: >> - @aio-max-batch documentation rewrite [Stefan, Kevin] >> >> qapi/block-core.json | 7 +++++++ >> block/file-posix.c | 9 +++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 6d3217abb6..fef76b0ea2 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2896,6 +2896,12 @@ >> # for this device (default: none, forward the commands via SG_IO; >> # since 2.11) >> # @aio: AIO backend (default: threads) (since: 2.8) >> +# @aio-max-batch: maximum number of requests to batch together into a single >> +# submission in the AIO backend. The smallest value between >> +# this and the aio-max-batch value of the IOThread object is >> +# chosen. >> +# 0 means that the AIO backend will handle it automatically. >> +# (default: 0, since 6.2) > >"(default 0) (since 6.2)" seems to be more common. Indeed I wasn't sure, so I followed @drop-cache, the last one added in @BlockdevOptionsFile. I'll fix in v3 :-) Thanks, Stefano
Am 27.10.2021 um 11:23 hat Stefano Garzarella geschrieben: > On Wed, Oct 27, 2021 at 06:28:51AM +0200, Markus Armbruster wrote: > > Stefano Garzarella <sgarzare@redhat.com> writes: > > > > > Commit d7ddd0a161 ("linux-aio: limit the batch size using > > > `aio-max-batch` parameter") added a way to limit the batch size > > > of Linux AIO backend for the entire AIO context. > > > > > > The same AIO context can be shared by multiple devices, so > > > latency-sensitive devices may want to limit the batch size even > > > more to avoid increasing latency. > > > > > > For this reason we add the `aio-max-batch` option to the file > > > backend, which will be used by the next commits to limit the size of > > > batches including requests generated by this device. > > > > > > Suggested-by: Kevin Wolf <kwolf@redhat.com> > > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > > > > Notes: > > > v2: > > > - @aio-max-batch documentation rewrite [Stefan, Kevin] > > > > > > qapi/block-core.json | 7 +++++++ > > > block/file-posix.c | 9 +++++++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index 6d3217abb6..fef76b0ea2 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -2896,6 +2896,12 @@ > > > # for this device (default: none, forward the commands via SG_IO; > > > # since 2.11) > > > # @aio: AIO backend (default: threads) (since: 2.8) > > > +# @aio-max-batch: maximum number of requests to batch together into a single > > > +# submission in the AIO backend. The smallest value between > > > +# this and the aio-max-batch value of the IOThread object is > > > +# chosen. > > > +# 0 means that the AIO backend will handle it automatically. > > > +# (default: 0, since 6.2) > > > > "(default 0) (since 6.2)" seems to be more common. > > Indeed I wasn't sure, so I followed @drop-cache, the last one added in > @BlockdevOptionsFile. Actually, I think your style is more common, both globally and in block-*: $ git grep -i '[,;] since' qapi/ | wc -l 17 $ git grep -i '[,;] since' qapi/block* | wc -l 12 Compared to: $ git grep -i ') (since' qapi/ | wc -l 14 $ git grep -i ') (since' qapi/block* | wc -l 7 Also a few instances with "(since: ...; default: ...)", but none in that order with separate brackets. So I'd rather merge this version if this is the only comment. Kevin
On Wed, Oct 27, 2021 at 03:06:37PM +0200, Kevin Wolf wrote: >Am 27.10.2021 um 11:23 hat Stefano Garzarella geschrieben: >> On Wed, Oct 27, 2021 at 06:28:51AM +0200, Markus Armbruster wrote: >> > Stefano Garzarella <sgarzare@redhat.com> writes: >> > >> > > Commit d7ddd0a161 ("linux-aio: limit the batch size using >> > > `aio-max-batch` parameter") added a way to limit the batch size >> > > of Linux AIO backend for the entire AIO context. >> > > >> > > The same AIO context can be shared by multiple devices, so >> > > latency-sensitive devices may want to limit the batch size even >> > > more to avoid increasing latency. >> > > >> > > For this reason we add the `aio-max-batch` option to the file >> > > backend, which will be used by the next commits to limit the size of >> > > batches including requests generated by this device. >> > > >> > > Suggested-by: Kevin Wolf <kwolf@redhat.com> >> > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > --- >> > > >> > > Notes: >> > > v2: >> > > - @aio-max-batch documentation rewrite [Stefan, Kevin] >> > > >> > > qapi/block-core.json | 7 +++++++ >> > > block/file-posix.c | 9 +++++++++ >> > > 2 files changed, 16 insertions(+) >> > > >> > > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > > index 6d3217abb6..fef76b0ea2 100644 >> > > --- a/qapi/block-core.json >> > > +++ b/qapi/block-core.json >> > > @@ -2896,6 +2896,12 @@ >> > > # for this device (default: none, forward the commands via SG_IO; >> > > # since 2.11) >> > > # @aio: AIO backend (default: threads) (since: 2.8) >> > > +# @aio-max-batch: maximum number of requests to batch together into a single >> > > +# submission in the AIO backend. The smallest value between >> > > +# this and the aio-max-batch value of the IOThread object is >> > > +# chosen. >> > > +# 0 means that the AIO backend will handle it automatically. >> > > +# (default: 0, since 6.2) >> > >> > "(default 0) (since 6.2)" seems to be more common. >> >> Indeed I wasn't sure, so I followed @drop-cache, the last one added in >> @BlockdevOptionsFile. > >Actually, I think your style is more common, both globally and in >block-*: > > $ git grep -i '[,;] since' qapi/ | wc -l > 17 > $ git grep -i '[,;] since' qapi/block* | wc -l > 12 > >Compared to: > > $ git grep -i ') (since' qapi/ | wc -l > 14 > $ git grep -i ') (since' qapi/block* | wc -l > 7 > Thanks for checking! >Also a few instances with "(since: ...; default: ...)", but none in >that >order with separate brackets. > >So I'd rather merge this version if this is the only comment. Honestly I don't have a strong opinion. If Markus agree, I think we can merge this version. Thanks, Stefano
On Wed, Oct 27, 2021 at 03:06:37PM +0200, Kevin Wolf wrote: >Am 27.10.2021 um 11:23 hat Stefano Garzarella geschrieben: >> On Wed, Oct 27, 2021 at 06:28:51AM +0200, Markus Armbruster wrote: >> > Stefano Garzarella <sgarzare@redhat.com> writes: >> > >> > > Commit d7ddd0a161 ("linux-aio: limit the batch size using >> > > `aio-max-batch` parameter") added a way to limit the batch size >> > > of Linux AIO backend for the entire AIO context. >> > > >> > > The same AIO context can be shared by multiple devices, so >> > > latency-sensitive devices may want to limit the batch size even >> > > more to avoid increasing latency. >> > > >> > > For this reason we add the `aio-max-batch` option to the file >> > > backend, which will be used by the next commits to limit the size of >> > > batches including requests generated by this device. >> > > >> > > Suggested-by: Kevin Wolf <kwolf@redhat.com> >> > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > --- >> > > >> > > Notes: >> > > v2: >> > > - @aio-max-batch documentation rewrite [Stefan, Kevin] >> > > >> > > qapi/block-core.json | 7 +++++++ >> > > block/file-posix.c | 9 +++++++++ >> > > 2 files changed, 16 insertions(+) >> > > >> > > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > > index 6d3217abb6..fef76b0ea2 100644 >> > > --- a/qapi/block-core.json >> > > +++ b/qapi/block-core.json >> > > @@ -2896,6 +2896,12 @@ >> > > # for this device (default: none, forward the commands via SG_IO; >> > > # since 2.11) >> > > # @aio: AIO backend (default: threads) (since: 2.8) >> > > +# @aio-max-batch: maximum number of requests to batch together into a single >> > > +# submission in the AIO backend. The smallest value between >> > > +# this and the aio-max-batch value of the IOThread object is >> > > +# chosen. >> > > +# 0 means that the AIO backend will handle it automatically. >> > > +# (default: 0, since 6.2) >> > >> > "(default 0) (since 6.2)" seems to be more common. >> >> Indeed I wasn't sure, so I followed @drop-cache, the last one added in >> @BlockdevOptionsFile. > >Actually, I think your style is more common, both globally and in >block-*: > > $ git grep -i '[,;] since' qapi/ | wc -l > 17 > $ git grep -i '[,;] since' qapi/block* | wc -l > 12 > >Compared to: > > $ git grep -i ') (since' qapi/ | wc -l > 14 > $ git grep -i ') (since' qapi/block* | wc -l > 7 > >Also a few instances with "(since: ...; default: ...)", but none in that >order with separate brackets. > >So I'd rather merge this version if this is the only comment. Kevin, since today is the soft freeze, are you planning on queuing this series? If not, I'll add to my todo list to resend it for the next release. Thanks, Stefano
diff --git a/qapi/block-core.json b/qapi/block-core.json index 6d3217abb6..fef76b0ea2 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2896,6 +2896,12 @@ # for this device (default: none, forward the commands via SG_IO; # since 2.11) # @aio: AIO backend (default: threads) (since: 2.8) +# @aio-max-batch: maximum number of requests to batch together into a single +# submission in the AIO backend. The smallest value between +# this and the aio-max-batch value of the IOThread object is +# chosen. +# 0 means that the AIO backend will handle it automatically. +# (default: 0, since 6.2) # @locking: whether to enable file locking. If set to 'auto', only enable # when Open File Descriptor (OFD) locking API is available # (default: auto, since 2.10) @@ -2924,6 +2930,7 @@ '*pr-manager': 'str', '*locking': 'OnOffAuto', '*aio': 'BlockdevAioOptions', + '*aio-max-batch': 'int', '*drop-cache': {'type': 'bool', 'if': 'CONFIG_LINUX'}, '*x-check-cache-dropped': 'bool' }, diff --git a/block/file-posix.c b/block/file-posix.c index 53be0bdc1b..d655fd0c45 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -150,6 +150,8 @@ typedef struct BDRVRawState { uint64_t locked_perm; uint64_t locked_shared_perm; + uint64_t aio_max_batch; + int perm_change_fd; int perm_change_flags; BDRVReopenState *reopen_state; @@ -530,6 +532,11 @@ static QemuOptsList raw_runtime_opts = { .type = QEMU_OPT_STRING, .help = "host AIO implementation (threads, native, io_uring)", }, + { + .name = "aio-max-batch", + .type = QEMU_OPT_NUMBER, + .help = "AIO max batch size (0 = auto handled by AIO backend, default: 0)", + }, { .name = "locking", .type = QEMU_OPT_STRING, @@ -609,6 +616,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING); #endif + s->aio_max_batch = qemu_opt_get_number(opts, "aio-max-batch", 0); + locking = qapi_enum_parse(&OnOffAuto_lookup, qemu_opt_get(opts, "locking"), ON_OFF_AUTO_AUTO, &local_err);
Commit d7ddd0a161 ("linux-aio: limit the batch size using `aio-max-batch` parameter") added a way to limit the batch size of Linux AIO backend for the entire AIO context. The same AIO context can be shared by multiple devices, so latency-sensitive devices may want to limit the batch size even more to avoid increasing latency. For this reason we add the `aio-max-batch` option to the file backend, which will be used by the next commits to limit the size of batches including requests generated by this device. Suggested-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- Notes: v2: - @aio-max-batch documentation rewrite [Stefan, Kevin] qapi/block-core.json | 7 +++++++ block/file-posix.c | 9 +++++++++ 2 files changed, 16 insertions(+)