[RFC PATCH 0/4] aio: AIO_CONTEXT_ACQUIRE_GUARD() macro experiment

[RFC PATCH 0/4] aio: AIO_CONTEXT_ACQUIRE_GUARD() macro experiment

[Philippe Mathieu-Daudé]

Experiment to use glib g_autoptr/autofree features with
AIO context.
Since this is a RFC, only few examples are provided.

TODO: Document the macros in docs/devel/multiple-iothreads.txt

Philippe Mathieu-Daudé (4):
  block/aio: Add automatically released aio_context variants
  hw/scsi/scsi-disk: Use automatic AIO context lock
  hw/scsi/scsi-generic: Use automatic AIO context lock
  hw/block/virtio-blk: Use automatic AIO context lock

 include/block/aio.h    | 24 ++++++++++++++++++++++++
 hw/block/virtio-blk.c  | 26 ++++++++++++--------------
 hw/scsi/scsi-disk.c    | 13 ++++---------
 hw/scsi/scsi-generic.c |  6 +++---
 4 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.31.1

[RFC PATCH 1/4] block/aio: Add automatically released aio_context variants

[Philippe Mathieu-Daudé]

Similarly to commit 5626f8c6d46 ("rcu: Add automatically
released rcu_read_lock variants"):

AIO_CONTEXT_ACQUIRE_GUARD() acquires the aio context and then uses
glib's g_auto infrastructure (and thus whatever the compiler's hooks
are) to release it on all exits of the block.

WITH_AIO_CONTEXT_ACQUIRE_GUARD() is similar but is used as a wrapper
for the lock, i.e.:

   WITH_AIO_CONTEXT_ACQUIRE_GUARD() {
       stuff with context acquired
   }

Inspired-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/block/aio.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index 47fbe9d81f2..4fa5a5c2720 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -294,6 +294,30 @@ void aio_context_acquire(AioContext *ctx);
 /* Relinquish ownership of the AioContext. */
 void aio_context_release(AioContext *ctx);
 
+static inline AioContext *aio_context_auto_acquire(AioContext *ctx)
+{
+    aio_context_acquire(ctx);
+    return ctx;
+}
+
+static inline void aio_context_auto_release(AioContext *ctx)
+{
+    aio_context_release(ctx);
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(AioContext, aio_context_auto_release)
+
+#define WITH_AIO_CONTEXT_ACQUIRE_GUARD(ctx) \
+    WITH_AIO_CONTEXT_ACQUIRE_GUARD_(glue(_aio_context_auto, __COUNTER__), ctx)
+
+#define WITH_AIO_CONTEXT_ACQUIRE_GUARD_(var, ctx) \
+    for (g_autoptr(AioContext) var = aio_context_auto_acquire(ctx); \
+        (var); aio_context_auto_release(var), (var) = NULL)
+
+#define AIO_CONTEXT_ACQUIRE_GUARD(ctx) \
+    g_autoptr(AioContext) _aio_context_auto __attribute__((unused)) \
+        = aio_context_auto_acquire(ctx)
+
 /**
  * aio_bh_schedule_oneshot_full: Allocate a new bottom half structure that will
  * run only once and as soon as possible.
-- 
2.31.1

[RFC PATCH 2/4] hw/scsi/scsi-disk: Use automatic AIO context lock

[Philippe Mathieu-Daudé]

Use the automatic AIO context acquire/release in scsi_block_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/scsi/scsi-disk.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e8a547dbb7d..fa2d8543718 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2605,7 +2605,6 @@ static int get_device_type(SCSIDiskState *s)
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-    AioContext *ctx;
     int sg_version;
     int rc;
 
@@ -2620,8 +2619,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
                           "be removed in a future version");
     }
 
-    ctx = blk_get_aio_context(s->qdev.conf.blk);
-    aio_context_acquire(ctx);
+    AIO_CONTEXT_ACQUIRE_GUARD(blk_get_aio_context(dev->conf.blk));
 
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
@@ -2630,18 +2628,18 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
         if (rc != -EPERM) {
             error_append_hint(errp, "Is this a SCSI device?\n");
         }
-        goto out;
+        return;
     }
     if (sg_version < 30000) {
         error_setg(errp, "scsi generic interface too old");
-        goto out;
+        return;
     }
 
     /* get device type from INQUIRY data */
     rc = get_device_type(s);
     if (rc < 0) {
         error_setg(errp, "INQUIRY failed");
-        goto out;
+        return;
     }
 
     /* Make a guess for the block size, we'll fix it when the guest sends.
@@ -2661,9 +2659,6 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 
     scsi_realize(&s->qdev, errp);
     scsi_generic_read_device_inquiry(&s->qdev);
-
-out:
-    aio_context_release(ctx);
 }
 
 typedef struct SCSIBlockReq {
-- 
2.31.1

Re: [RFC PATCH 2/4] hw/scsi/scsi-disk: Use automatic AIO context lock

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Use the automatic AIO context acquire/release in scsi_block_realize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I'll let the AIO people reply more thoroughly to this; but this patch is
the case I really like this mechanism for; removing the need for the
goto's and cleanup.

Dave

> ---
>  hw/scsi/scsi-disk.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e8a547dbb7d..fa2d8543718 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2605,7 +2605,6 @@ static int get_device_type(SCSIDiskState *s)
>  static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  {
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
> -    AioContext *ctx;
>      int sg_version;
>      int rc;
>  
> @@ -2620,8 +2619,7 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>                            "be removed in a future version");
>      }
>  
> -    ctx = blk_get_aio_context(s->qdev.conf.blk);
> -    aio_context_acquire(ctx);
> +    AIO_CONTEXT_ACQUIRE_GUARD(blk_get_aio_context(dev->conf.blk));
>  
>      /* check we are using a driver managing SG_IO (version 3 and after) */
>      rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
> @@ -2630,18 +2628,18 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>          if (rc != -EPERM) {
>              error_append_hint(errp, "Is this a SCSI device?\n");
>          }
> -        goto out;
> +        return;
>      }
>      if (sg_version < 30000) {
>          error_setg(errp, "scsi generic interface too old");
> -        goto out;
> +        return;
>      }
>  
>      /* get device type from INQUIRY data */
>      rc = get_device_type(s);
>      if (rc < 0) {
>          error_setg(errp, "INQUIRY failed");
> -        goto out;
> +        return;
>      }
>  
>      /* Make a guess for the block size, we'll fix it when the guest sends.
> @@ -2661,9 +2659,6 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
>  
>      scsi_realize(&s->qdev, errp);
>      scsi_generic_read_device_inquiry(&s->qdev);
> -
> -out:
> -    aio_context_release(ctx);
>  }
>  
>  typedef struct SCSIBlockReq {
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[RFC PATCH 3/4] hw/scsi/scsi-generic: Use automatic AIO context lock

[Philippe Mathieu-Daudé]

Use the automatic AIO context acquire/release in
scsi_command_complete().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/scsi/scsi-generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 665baf900e4..08ef623c030 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -114,9 +114,9 @@ static void scsi_command_complete(void *opaque, int ret)
     assert(r->req.aiocb != NULL);
     r->req.aiocb = NULL;
 
-    aio_context_acquire(blk_get_aio_context(s->conf.blk));
-    scsi_command_complete_noio(r, ret);
-    aio_context_release(blk_get_aio_context(s->conf.blk));
+    WITH_AIO_CONTEXT_ACQUIRE_GUARD(blk_get_aio_context(s->conf.blk)) {
+        scsi_command_complete_noio(r, ret);
+    }
 }
 
 static int execute_command(BlockBackend *blk,
-- 
2.31.1

[RFC PATCH 4/4] hw/block/virtio-blk: Use automatic AIO context lock

[Philippe Mathieu-Daudé]

Use the automatic AIO context acquire/release in virtio_blk_reset().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/virtio-blk.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f139cd7cc9c..2dd6428e7b3 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -896,24 +896,22 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool running,
 static void virtio_blk_reset(VirtIODevice *vdev)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
-    AioContext *ctx;
     VirtIOBlockReq *req;
 
-    ctx = blk_get_aio_context(s->blk);
-    aio_context_acquire(ctx);
-    blk_drain(s->blk);
-
-    /* We drop queued requests after blk_drain() because blk_drain() itself can
-     * produce them. */
-    while (s->rq) {
-        req = s->rq;
-        s->rq = req->next;
-        virtqueue_detach_element(req->vq, &req->elem, 0);
-        virtio_blk_free_request(req);
+    WITH_AIO_CONTEXT_ACQUIRE_GUARD(blk_get_aio_context(s->blk)) {
+        blk_drain(s->blk);
+        /*
+         * We drop queued requests after blk_drain() because
+         * blk_drain() itself can produce them.
+         */
+        while (s->rq) {
+            req = s->rq;
+            s->rq = req->next;
+            virtqueue_detach_element(req->vq, &req->elem, 0);
+            virtio_blk_free_request(req);
+        }
     }
 
-    aio_context_release(ctx);
-
     assert(!s->dataplane_started);
     blk_set_enable_write_cache(s->blk, s->original_wce);
 }
-- 
2.31.1

Re: [RFC PATCH 0/4] aio: AIO_CONTEXT_ACQUIRE_GUARD() macro experiment

[Stefan Hajnoczi]

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

On Tue, Oct 05, 2021 at 08:58:03PM +0200, Philippe Mathieu-Daudé wrote:
> Experiment to use glib g_autoptr/autofree features with
> AIO context.
> Since this is a RFC, only few examples are provided.
> 
> TODO: Document the macros in docs/devel/multiple-iothreads.txt
> 
> Philippe Mathieu-Daudé (4):
>   block/aio: Add automatically released aio_context variants
>   hw/scsi/scsi-disk: Use automatic AIO context lock
>   hw/scsi/scsi-generic: Use automatic AIO context lock
>   hw/block/virtio-blk: Use automatic AIO context lock
> 
>  include/block/aio.h    | 24 ++++++++++++++++++++++++
>  hw/block/virtio-blk.c  | 26 ++++++++++++--------------
>  hw/scsi/scsi-disk.c    | 13 ++++---------
>  hw/scsi/scsi-generic.c |  6 +++---
>  4 files changed, 43 insertions(+), 26 deletions(-)

This is nice. Two things:

1. Emanuele is working on eliminating aio_context_acquire/release(), so
   a big effort to convert existing code to using guards could be wasted
   energy and cause conflicts with his patches.

2. A few callers anticipate that the AioContext of their BDS may change
   between acquire/release. Care needs to be taken when converting to
   preserve the semantics but most instances should be safe to convert.

Stefan

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

Re: [RFC PATCH 0/4] aio: AIO_CONTEXT_ACQUIRE_GUARD() macro experiment

[Philippe Mathieu-Daudé]

On 10/7/21 15:15, Stefan Hajnoczi wrote:
> On Tue, Oct 05, 2021 at 08:58:03PM +0200, Philippe Mathieu-Daudé wrote:
>> Experiment to use glib g_autoptr/autofree features with
>> AIO context.
>> Since this is a RFC, only few examples are provided.
>>
>> TODO: Document the macros in docs/devel/multiple-iothreads.txt
>>
>> Philippe Mathieu-Daudé (4):
>>   block/aio: Add automatically released aio_context variants
>>   hw/scsi/scsi-disk: Use automatic AIO context lock
>>   hw/scsi/scsi-generic: Use automatic AIO context lock
>>   hw/block/virtio-blk: Use automatic AIO context lock
>>
>>  include/block/aio.h    | 24 ++++++++++++++++++++++++
>>  hw/block/virtio-blk.c  | 26 ++++++++++++--------------
>>  hw/scsi/scsi-disk.c    | 13 ++++---------
>>  hw/scsi/scsi-generic.c |  6 +++---
>>  4 files changed, 43 insertions(+), 26 deletions(-)
> 
> This is nice. Two things:
> 
> 1. Emanuele is working on eliminating aio_context_acquire/release(), so
>    a big effort to convert existing code to using guards could be wasted
>    energy and cause conflicts with his patches.

Thanks for the update, I'll wait Emanuele effort to land.

> 2. A few callers anticipate that the AioContext of their BDS may change
>    between acquire/release. Care needs to be taken when converting to
>    preserve the semantics but most instances should be safe to convert.
> 
> Stefan
>