[PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[Stefano Garzarella]

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.

This series add the `aio-max-batch` option to the file backend,
and use it in laio_co_submit() and laio_io_unplug() to limit the
Linux AIO batch size more than the limit set by the AIO context.

Stefano Garzarella (3):
  file-posix: add `aio-max-batch` option
  linux-aio: add `dev_max_batch` parameter to laio_co_submit()
  linux-aio: add `dev_max_batch` parameter to laio_io_unplug()

 qapi/block-core.json    |  5 +++++
 include/block/raw-aio.h |  6 ++++--
 block/file-posix.c      | 14 ++++++++++++--
 block/linux-aio.c       | 38 +++++++++++++++++++++++++++-----------
 4 files changed, 48 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH 1/3] file-posix: add `aio-max-batch` option

[Stefano Garzarella]

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>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 qapi/block-core.json | 5 +++++
 block/file-posix.c   | 9 +++++++++
 2 files changed, 14 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index c8ce1d9d5d..1a8ed325bc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2851,6 +2851,10 @@
 #              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 in an AIO backend batch that
+#                 contains request from this block device. 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)
@@ -2879,6 +2883,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 d81e15efa4..da50e94034 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);
-- 
2.31.1

[PATCH 2/3] linux-aio: add `dev_max_batch` parameter to laio_co_submit()

[Stefano Garzarella]

This new parameter can be used by block devices to limit the
Linux AIO batch size more than the limit set by the AIO context.

file-posix backend supports this, passing its `aio-max-batch` option
previously added.

Add an helper function to calculate the maximum batch size.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/block/raw-aio.h |  3 ++-
 block/file-posix.c      |  3 ++-
 block/linux-aio.c       | 30 ++++++++++++++++++++++--------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 251b10d273..ebd042fa27 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -51,7 +51,8 @@ typedef struct LinuxAioState LinuxAioState;
 LinuxAioState *laio_init(Error **errp);
 void laio_cleanup(LinuxAioState *s);
 int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-                                uint64_t offset, QEMUIOVector *qiov, int type);
+                                uint64_t offset, QEMUIOVector *qiov, int type,
+                                uint64_t dev_max_batch);
 void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
 void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
 void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
diff --git a/block/file-posix.c b/block/file-posix.c
index da50e94034..614c725235 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2066,7 +2066,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
     } 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);
+        return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
+                              s->aio_max_batch);
 #endif
     }
 
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 0dab507b71..88b44fee72 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -334,6 +334,23 @@ static void ioq_submit(LinuxAioState *s)
     }
 }
 
+static uint64_t laio_max_batch(LinuxAioState *s, uint64_t dev_max_batch)
+{
+    uint64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
+
+    /*
+     * AIO context can be shared between multiple block devices, so
+     * `dev_max_batch` allows reducing the batch size for latency-sensitive
+     * devices.
+     */
+    max_batch = MIN_NON_ZERO(dev_max_batch, max_batch);
+
+    /* limit the batch with the number of available events */
+    max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch);
+
+    return max_batch;
+}
+
 void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
 {
     s->io_q.plugged++;
@@ -349,15 +366,11 @@ void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s)
 }
 
 static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
-                          int type)
+                          int type, uint64_t dev_max_batch)
 {
     LinuxAioState *s = laiocb->ctx;
     struct iocb *iocbs = &laiocb->iocb;
     QEMUIOVector *qiov = laiocb->qiov;
-    int64_t max_batch = s->aio_context->aio_max_batch ?: DEFAULT_MAX_BATCH;
-
-    /* limit the batch with the number of available events */
-    max_batch = MIN_NON_ZERO(MAX_EVENTS - s->io_q.in_flight, max_batch);
 
     switch (type) {
     case QEMU_AIO_WRITE:
@@ -378,7 +391,7 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
     s->io_q.in_queue++;
     if (!s->io_q.blocked &&
         (!s->io_q.plugged ||
-         s->io_q.in_queue >= max_batch)) {
+         s->io_q.in_queue >= laio_max_batch(s, dev_max_batch))) {
         ioq_submit(s);
     }
 
@@ -386,7 +399,8 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
 }
 
 int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
-                                uint64_t offset, QEMUIOVector *qiov, int type)
+                                uint64_t offset, QEMUIOVector *qiov, int type,
+                                uint64_t dev_max_batch)
 {
     int ret;
     struct qemu_laiocb laiocb = {
@@ -398,7 +412,7 @@ int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
         .qiov       = qiov,
     };
 
-    ret = laio_do_submit(fd, &laiocb, offset, type);
+    ret = laio_do_submit(fd, &laiocb, offset, type, dev_max_batch);
     if (ret < 0) {
         return ret;
     }
-- 
2.31.1

[PATCH 3/3] linux-aio: add `dev_max_batch` parameter to laio_io_unplug()

[Stefano Garzarella]

Between the submission of a request and the unplug, other devices
with larger limits may have been queued new requests without flushing
the batch.

Using the new `dev_max_batch` parameter, laio_io_unplug() can check
if the batch exceeds the device limit to flush the current batch.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/block/raw-aio.h | 3 ++-
 block/file-posix.c      | 2 +-
 block/linux-aio.c       | 8 +++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index ebd042fa27..21fc10c4c9 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -56,7 +56,8 @@ int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
 void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
 void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
 void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s);
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+                    uint64_t dev_max_batch);
 #endif
 /* io_uring.c - Linux io_uring implementation */
 #ifdef CONFIG_LINUX_IO_URING
diff --git a/block/file-posix.c b/block/file-posix.c
index 614c725235..f31a75a715 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2125,7 +2125,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
 #ifdef CONFIG_LINUX_AIO
     if (s->use_linux_aio) {
         LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
-        laio_io_unplug(bs, aio);
+        laio_io_unplug(bs, aio, s->aio_max_batch);
     }
 #endif
 #ifdef CONFIG_LINUX_IO_URING
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 88b44fee72..f53ae72e21 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -356,11 +356,13 @@ void laio_io_plug(BlockDriverState *bs, LinuxAioState *s)
     s->io_q.plugged++;
 }
 
-void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s)
+void laio_io_unplug(BlockDriverState *bs, LinuxAioState *s,
+                    uint64_t dev_max_batch)
 {
     assert(s->io_q.plugged);
-    if (--s->io_q.plugged == 0 &&
-        !s->io_q.blocked && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
+    if (s->io_q.in_queue >= laio_max_batch(s, dev_max_batch) ||
+        (--s->io_q.plugged == 0 &&
+         !s->io_q.blocked && !QSIMPLEQ_EMPTY(&s->io_q.pending))) {
         ioq_submit(s);
     }
 }
-- 
2.31.1

Re: [PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[Stefano Garzarella]

Kind ping :-)

Thanks,
Stefano

On Thu, Sep 23, 2021 at 4:31 PM Stefano Garzarella <sgarzare@redhat.com> 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.
>
> This series add the `aio-max-batch` option to the file backend,
> and use it in laio_co_submit() and laio_io_unplug() to limit the
> Linux AIO batch size more than the limit set by the AIO context.
>
> Stefano Garzarella (3):
>   file-posix: add `aio-max-batch` option
>   linux-aio: add `dev_max_batch` parameter to laio_co_submit()
>   linux-aio: add `dev_max_batch` parameter to laio_io_unplug()
>
>  qapi/block-core.json    |  5 +++++
>  include/block/raw-aio.h |  6 ++++--
>  block/file-posix.c      | 14 ++++++++++++--
>  block/linux-aio.c       | 38 +++++++++++++++++++++++++++-----------
>  4 files changed, 48 insertions(+), 15 deletions(-)
>
> --
> 2.31.1
>
>

Re: [PATCH 1/3] file-posix: add `aio-max-batch` option

[Stefan Hajnoczi]

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

On Thu, Sep 23, 2021 at 04:30:58PM +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>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  qapi/block-core.json | 5 +++++
>  block/file-posix.c   | 9 +++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c8ce1d9d5d..1a8ed325bc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2851,6 +2851,10 @@
>  #              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 in an AIO backend batch that
> +#                 contains request from this block device. 0 means that the

The first sentence is a little unclear. I guess s/request/requests/ but
that still doesn't doesn't fully explain how this works.

Does the AIO backend use the minimum aio-max-batch value of all its
blockdevs?

Maybe:

  maximum number of requests to batch together into a single submission
  in the AIO backend. If multiple BlockdevOptionsFile sharing an AIO
  backend have different values the smallest value is chosen. ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] linux-aio: add `dev_max_batch` parameter to laio_co_submit()

[Stefan Hajnoczi]

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

On Thu, Sep 23, 2021 at 04:30:59PM +0200, Stefano Garzarella wrote:
> This new parameter can be used by block devices to limit the
> Linux AIO batch size more than the limit set by the AIO context.
> 
> file-posix backend supports this, passing its `aio-max-batch` option
> previously added.
> 
> Add an helper function to calculate the maximum batch size.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  include/block/raw-aio.h |  3 ++-
>  block/file-posix.c      |  3 ++-
>  block/linux-aio.c       | 30 ++++++++++++++++++++++--------
>  3 files changed, 26 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] linux-aio: add `dev_max_batch` parameter to laio_io_unplug()

[Stefan Hajnoczi]

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

On Thu, Sep 23, 2021 at 04:31:00PM +0200, Stefano Garzarella wrote:
> Between the submission of a request and the unplug, other devices
> with larger limits may have been queued new requests without flushing
> the batch.
> 
> Using the new `dev_max_batch` parameter, laio_io_unplug() can check
> if the batch exceeds the device limit to flush the current batch.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  include/block/raw-aio.h | 3 ++-
>  block/file-posix.c      | 2 +-
>  block/linux-aio.c       | 8 +++++---
>  3 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[Stefan Hajnoczi]

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

On Thu, Sep 23, 2021 at 04:30:57PM +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.
> 
> This series add the `aio-max-batch` option to the file backend,
> and use it in laio_co_submit() and laio_io_unplug() to limit the
> Linux AIO batch size more than the limit set by the AIO context.
> 
> Stefano Garzarella (3):
>   file-posix: add `aio-max-batch` option
>   linux-aio: add `dev_max_batch` parameter to laio_co_submit()
>   linux-aio: add `dev_max_batch` parameter to laio_io_unplug()
> 
>  qapi/block-core.json    |  5 +++++
>  include/block/raw-aio.h |  6 ++++--
>  block/file-posix.c      | 14 ++++++++++++--
>  block/linux-aio.c       | 38 +++++++++++++++++++++++++++-----------
>  4 files changed, 48 insertions(+), 15 deletions(-)

Sorry for the slow review. Comments posted!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] file-posix: add `aio-max-batch` option

[Stefano Garzarella]

On Thu, Oct 21, 2021 at 04:18:41PM +0100, Stefan Hajnoczi wrote:
>On Thu, Sep 23, 2021 at 04:30:58PM +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>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  qapi/block-core.json | 5 +++++
>>  block/file-posix.c   | 9 +++++++++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c8ce1d9d5d..1a8ed325bc 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2851,6 +2851,10 @@
>>  #              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 in an AIO backend batch that
>> +#                 contains request from this block device. 0 means that the
>
>The first sentence is a little unclear. I guess s/request/requests/ but
>that still doesn't doesn't fully explain how this works.
>
>Does the AIO backend use the minimum aio-max-batch value of all its
>blockdevs?

It's a little simpler to avoid having to recalculate the minimum for 
each attach/release of blockdevs.

When the blockdev does submit or unplug, the queue is flushed if the 
number of requests in the batch is greater or equal then the smallest 
aio-max-batch value of the blockdev and the AIO context.

>
>Maybe:
>
>  maximum number of requests to batch together into a single submission
>  in the AIO backend. If multiple BlockdevOptionsFile sharing an AIO
>  backend have different values the smallest value is chosen. ...

Whath about this:

   maximum number of requests to batch together into a single submission
   in the AIO backend. The smallest value between this and AIO context's 
   aio-max-batch value is chosen. ...

Thanks,
Stefano

Re: [PATCH 1/3] file-posix: add `aio-max-batch` option

[Kevin Wolf]

Am 25.10.2021 um 12:20 hat Stefano Garzarella geschrieben:
> On Thu, Oct 21, 2021 at 04:18:41PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 23, 2021 at 04:30:58PM +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>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  qapi/block-core.json | 5 +++++
> > >  block/file-posix.c   | 9 +++++++++
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index c8ce1d9d5d..1a8ed325bc 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -2851,6 +2851,10 @@
> > >  #              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 in an AIO backend batch that
> > > +#                 contains request from this block device. 0 means that the
> > 
> > The first sentence is a little unclear. I guess s/request/requests/ but
> > that still doesn't doesn't fully explain how this works.
> > 
> > Does the AIO backend use the minimum aio-max-batch value of all its
> > blockdevs?
> 
> It's a little simpler to avoid having to recalculate the minimum for each
> attach/release of blockdevs.
> 
> When the blockdev does submit or unplug, the queue is flushed if the number
> of requests in the batch is greater or equal then the smallest aio-max-batch
> value of the blockdev and the AIO context.
> 
> > 
> > Maybe:
> > 
> >  maximum number of requests to batch together into a single submission
> >  in the AIO backend. If multiple BlockdevOptionsFile sharing an AIO
> >  backend have different values the smallest value is chosen. ...
> 
> Whath about this:
> 
>   maximum number of requests to batch together into a single submission
>   in the AIO backend. The smallest value between this and AIO context's
> aio-max-batch value is chosen. ...

I like this, except that AioContexts are an implementation detail. I
think we should refer to the iothread object instead, which is the user
visible interface to AioContexts.

Kevin

Re: [PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[Kevin Wolf]

Am 23.09.2021 um 16:30 hat Stefano Garzarella geschrieben:
> 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.
> 
> This series add the `aio-max-batch` option to the file backend,
> and use it in laio_co_submit() and laio_io_unplug() to limit the
> Linux AIO batch size more than the limit set by the AIO context.

Looks like you're only going to improve the wording for the QAPI
documentation for v2, so feel free to add:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 1/3] file-posix: add `aio-max-batch` option

[Stefano Garzarella]

On Mon, Oct 25, 2021 at 04:04:21PM +0200, Kevin Wolf wrote:
>Am 25.10.2021 um 12:20 hat Stefano Garzarella geschrieben:
>> On Thu, Oct 21, 2021 at 04:18:41PM +0100, Stefan Hajnoczi wrote:
>> > On Thu, Sep 23, 2021 at 04:30:58PM +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>
>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > ---
>> > >  qapi/block-core.json | 5 +++++
>> > >  block/file-posix.c   | 9 +++++++++
>> > >  2 files changed, 14 insertions(+)
>> > >
>> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > > index c8ce1d9d5d..1a8ed325bc 100644
>> > > --- a/qapi/block-core.json
>> > > +++ b/qapi/block-core.json
>> > > @@ -2851,6 +2851,10 @@
>> > >  #              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 in an AIO backend batch that
>> > > +#                 contains request from this block device. 0 means that the
>> >
>> > The first sentence is a little unclear. I guess s/request/requests/ but
>> > that still doesn't doesn't fully explain how this works.
>> >
>> > Does the AIO backend use the minimum aio-max-batch value of all its
>> > blockdevs?
>>
>> It's a little simpler to avoid having to recalculate the minimum for each
>> attach/release of blockdevs.
>>
>> When the blockdev does submit or unplug, the queue is flushed if the number
>> of requests in the batch is greater or equal then the smallest aio-max-batch
>> value of the blockdev and the AIO context.
>>
>> >
>> > Maybe:
>> >
>> >  maximum number of requests to batch together into a single submission
>> >  in the AIO backend. If multiple BlockdevOptionsFile sharing an AIO
>> >  backend have different values the smallest value is chosen. ...
>>
>> Whath about this:
>>
>>   maximum number of requests to batch together into a single submission
>>   in the AIO backend. The smallest value between this and AIO context's
>> aio-max-batch value is chosen. ...
>
>I like this, except that AioContexts are an implementation detail. I
>think we should refer to the iothread object instead, which is the user
>visible interface to AioContexts.

Yep, definitely better:

   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. ...


Thanks,
Stefano

Re: [PATCH 0/3] linux-aio: allow block devices to limit aio-max-batch

[Stefano Garzarella]

On Mon, Oct 25, 2021 at 04:16:54PM +0200, Kevin Wolf wrote:
>Am 23.09.2021 um 16:30 hat Stefano Garzarella geschrieben:
>> 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.
>>
>> This series add the `aio-max-batch` option to the file backend,
>> and use it in laio_co_submit() and laio_io_unplug() to limit the
>> Linux AIO batch size more than the limit set by the AIO context.
>
>Looks like you're only going to improve the wording for the QAPI
>documentation for v2, so feel free to add:
>
>Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>

Yep, I'll add your R-b in the v2 :-)

Thanks,
Stefano