nvme_get_free_req has very difference semantics when called in
coroutine context (when it waits) and in non-coroutine context
(when it doesn't).  Split the two cases to make it clear what
is being requested.

Cc: qemu-block@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/nvme.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 01fb28aa63..092c1f2f8e 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -293,34 +293,42 @@ static void nvme_kick(NVMeQueuePair *q)
     q->need_kick = 0;
 }
 
-/* Find a free request element if any, otherwise:
- * a) if in coroutine context, try to wait for one to become available;
- * b) if not in coroutine, return NULL;
- */
-static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
+static NVMeRequest *nvme_get_free_req_nofail_locked(NVMeQueuePair *q)
 {
     NVMeRequest *req;
 
-    qemu_mutex_lock(&q->lock);
-
-    while (q->free_req_head == -1) {
-        if (qemu_in_coroutine()) {
-            trace_nvme_free_req_queue_wait(q->s, q->index);
-            qemu_co_queue_wait(&q->free_req_queue, &q->lock);
-        } else {
-            qemu_mutex_unlock(&q->lock);
-            return NULL;
-        }
-    }
-
     req = &q->reqs[q->free_req_head];
     q->free_req_head = req->free_req_next;
     req->free_req_next = -1;
-
-    qemu_mutex_unlock(&q->lock);
     return req;
 }
 
+/* Return a free request element if any, otherwise return NULL.  */
+static NVMeRequest *nvme_get_free_req_nowait(NVMeQueuePair *q)
+{
+    QEMU_LOCK_GUARD(&q->lock);
+    if (q->free_req_head == -1) {
+        return NULL;
+    }
+    return nvme_get_free_req_nofail_locked(q);
+}
+
+/*
+ * Return a free request element if any, otherwise wait
+ * for one to become available
+ */
+static coroutine_fn NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
+{
+    QEMU_LOCK_GUARD(&q->lock);
+
+    while (q->free_req_head == -1) {
+       trace_nvme_free_req_queue_wait(q->s, q->index);
+       qemu_co_queue_wait(&q->free_req_queue, &q->lock);
+    }
+
+    return nvme_get_free_req_nofail_locked(q);
+}
+
 /* With q->lock */
 static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req)
 {
@@ -506,7 +514,7 @@ static int nvme_admin_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
     AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     int ret = -EINPROGRESS;
-    req = nvme_get_free_req(q);
+    req = nvme_get_free_req_nowait(q);
     if (!req) {
         return -EBUSY;
     }
-- 
2.36.0

On Sat, May 14, 2022 at 11:31 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> nvme_get_free_req has very difference semantics when called in
> coroutine context (when it waits) and in non-coroutine context
> (when it doesn't).  Split the two cases to make it clear what
> is being requested.
>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nvme.c | 48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 01fb28aa63..092c1f2f8e 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -293,34 +293,42 @@ static void nvme_kick(NVMeQueuePair *q)
>      q->need_kick = 0;
>  }
>
> -/* Find a free request element if any, otherwise:
> - * a) if in coroutine context, try to wait for one to become available;
> - * b) if not in coroutine, return NULL;
> - */
> -static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
> +static NVMeRequest *nvme_get_free_req_nofail_locked(NVMeQueuePair *q)
>  {
>      NVMeRequest *req;
>
> -    qemu_mutex_lock(&q->lock);
> -
> -    while (q->free_req_head == -1) {
> -        if (qemu_in_coroutine()) {
> -            trace_nvme_free_req_queue_wait(q->s, q->index);
> -            qemu_co_queue_wait(&q->free_req_queue, &q->lock);
> -        } else {
> -            qemu_mutex_unlock(&q->lock);
> -            return NULL;
> -        }
> -    }
> -
>      req = &q->reqs[q->free_req_head];
>      q->free_req_head = req->free_req_next;
>      req->free_req_next = -1;
> -
> -    qemu_mutex_unlock(&q->lock);
>      return req;
>  }
>
> +/* Return a free request element if any, otherwise return NULL.  */
> +static NVMeRequest *nvme_get_free_req_nowait(NVMeQueuePair *q)
> +{
> +    QEMU_LOCK_GUARD(&q->lock);
> +    if (q->free_req_head == -1) {
> +        return NULL;
> +    }
> +    return nvme_get_free_req_nofail_locked(q);
> +}
> +
> +/*
> + * Return a free request element if any, otherwise wait
> + * for one to become available
> + */

Might be a bit more precise as: "Wait for a free request to become
available and return it."

> +static coroutine_fn NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
> +{
> +    QEMU_LOCK_GUARD(&q->lock);
> +
> +    while (q->free_req_head == -1) {
> +       trace_nvme_free_req_queue_wait(q->s, q->index);
> +       qemu_co_queue_wait(&q->free_req_queue, &q->lock);
> +    }
> +
> +    return nvme_get_free_req_nofail_locked(q);
> +}
> +
>  /* With q->lock */
>  static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req)
>  {
> @@ -506,7 +514,7 @@ static int nvme_admin_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
>      AioContext *aio_context = bdrv_get_aio_context(bs);
>      NVMeRequest *req;
>      int ret = -EINPROGRESS;
> -    req = nvme_get_free_req(q);
> +    req = nvme_get_free_req_nowait(q);
>      if (!req) {
>          return -EBUSY;
>      }
> --
> 2.36.0
>
>

Reviewed-by: Alberto Faria <afaria@redhat.com>