[PATCH 00/17] block/nvme: Various cleanups required to use multiple queues

[PATCH 00/17] block/nvme: Various cleanups required to use multiple queues

[Philippe Mathieu-Daudé]

Hi,

This series is mostly code rearrangement (cleanups) to be
able to split the hardware code from the block driver code,
to be able to use multiple queues on the same hardware, or
multiple block drivers on the same hardware.

Flushing my current patch queue.

Regards,

Phil.

Based-on: <20200625162602.700741-1-eblake@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08384.html

Philippe Mathieu-Daudé (17):
  block/nvme: Avoid further processing if trace event not enabled
  block/nvme: Let nvme_create_queue_pair() fail gracefully
  block/nvme: Define QUEUE_INDEX macros to ease code review
  block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures
  block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset
  block/nvme: Replace qemu_try_blockalign(bs) by
    qemu_try_memalign(pg_sz)
  block/nvme: Move code around
  block/nvme: Use correct type void*
  block/nvme: Remove unused argument from nvme_free_queue_pair()
  block/nvme: Simplify nvme_init_queue() arguments
  block/nvme: Simplify nvme_create_queue_pair() arguments
  block/nvme: Simplify nvme_kick trace event
  block/nvme: Simplify completion trace events
  block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE
  block/nvme: Use per-queue AIO context
  block/nvme: Check BDRVNVMeState::plugged out of nvme_kick()
  block/nvme: Check BDRVNVMeState::plugged out of
    nvme_process_completion

 block/nvme.c       | 160 ++++++++++++++++++++++++++-------------------
 block/trace-events |   8 +--
 2 files changed, 96 insertions(+), 72 deletions(-)

-- 
2.21.3

[PATCH 01/17] block/nvme: Avoid further processing if trace event not enabled

[Philippe Mathieu-Daudé]

Avoid further processing if TRACE_NVME_SUBMIT_COMMAND_RAW is
not enabled.

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

diff --git a/block/nvme.c b/block/nvme.c
index eb2f54dd9d..1e5b40f61c 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -367,6 +367,9 @@ static void nvme_trace_command(const NvmeCmd *cmd)
 {
     int i;
 
+    if (!trace_event_get_state_backends(TRACE_NVME_SUBMIT_COMMAND_RAW)) {
+        return;
+    }
     for (i = 0; i < 8; ++i) {
         uint8_t *cmdp = (uint8_t *)cmd + i * 8;
         trace_nvme_submit_command_raw(cmdp[0], cmdp[1], cmdp[2], cmdp[3],
-- 
2.21.3

Re: [PATCH 01/17] block/nvme: Avoid further processing if trace event not enabled

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:22PM +0200, Philippe Mathieu-Daudé wrote:
> Avoid further processing if TRACE_NVME_SUBMIT_COMMAND_RAW is
> not enabled.

Why?

This saves 8 trace events, each with 8 arguments. I guess the intent is
to improve performance. Did you measure an improvement?

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

Re: [PATCH 01/17] block/nvme: Avoid further processing if trace event not enabled

[Philippe Mathieu-Daudé]

On 6/26/20 12:36 PM, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2020 at 08:48:22PM +0200, Philippe Mathieu-Daudé wrote:
>> Avoid further processing if TRACE_NVME_SUBMIT_COMMAND_RAW is
>> not enabled.
> 
> Why?
> 
> This saves 8 trace events, each with 8 arguments. I guess the intent is
> to improve performance. Did you measure an improvement?

No testing, I just tried to outsmart the compiler :/

Re: [PATCH 01/17] block/nvme: Avoid further processing if trace event not enabled

[Stefan Hajnoczi]

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

On Fri, Jun 26, 2020 at 04:02:43PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/26/20 12:36 PM, Stefan Hajnoczi wrote:
> > On Thu, Jun 25, 2020 at 08:48:22PM +0200, Philippe Mathieu-Daudé wrote:
> >> Avoid further processing if TRACE_NVME_SUBMIT_COMMAND_RAW is
> >> not enabled.
> > 
> > Why?
> > 
> > This saves 8 trace events, each with 8 arguments. I guess the intent is
> > to improve performance. Did you measure an improvement?
> 
> No testing, I just tried to outsmart the compiler :/

Usually performance patches are accompanied with benchmark results.
Otherwise it's easy to modify code without making a difference or
accidentally introducing performance regressions. But this is a small
change and I'm not worried.

Please do explicitly state in the commit description that this is
intended as a performance optimization. I wasn't exactly sure about the
reason for this change.

Stefan

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

[PATCH 02/17] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Philippe Mathieu-Daudé]

As nvme_create_queue_pair() is allowed to fail, replace the
alloc() calls by try_alloc() to avoid aborting QEMU.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 1e5b40f61c..ec0dd21b6e 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -194,13 +194,21 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     int i, r;
     BDRVNVMeState *s = bs->opaque;
     Error *local_err = NULL;
-    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
+    NVMeQueuePair *q;
     uint64_t prp_list_iova;
 
+    q = g_try_new0(NVMeQueuePair, 1);
+    if (!q) {
+        return NULL;
+    }
+    q->prp_list_pages = qemu_try_blockalign0(bs,
+                                          s->page_size * NVME_QUEUE_SIZE);
+    if (!q->prp_list_pages) {
+        goto fail;
+    }
     qemu_mutex_init(&q->lock);
     q->index = idx;
     qemu_co_queue_init(&q->free_req_queue);
-    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE);
     r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
                           s->page_size * NVME_QUEUE_SIZE,
                           false, &prp_list_iova);
-- 
2.21.3

Re: [PATCH 02/17] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:23PM +0200, Philippe Mathieu-Daudé wrote:
> As nvme_create_queue_pair() is allowed to fail, replace the
> alloc() calls by try_alloc() to avoid aborting QEMU.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

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

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

[PATCH 03/17] block/nvme: Define QUEUE_INDEX macros to ease code review

[Philippe Mathieu-Daudé]

Use definitions instead of '0' or '1' indexes. Also this will
be useful when using multi-queues later.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index ec0dd21b6e..71f8cf27a8 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -89,6 +89,9 @@ typedef volatile struct {
 
 QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 
+#define QUEUE_INDEX_ADMIN   0
+#define QUEUE_INDEX_IO(n)   (1 + n)
+
 typedef struct {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
@@ -459,7 +462,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     }
     cmd.prp1 = cpu_to_le64(iova);
 
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
     }
@@ -483,7 +486,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify namespace");
         goto out;
     }
@@ -560,7 +563,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
         .cdw11 = cpu_to_le32(0x3),
     };
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
         nvme_free_queue_pair(bs, q);
         return false;
@@ -571,7 +574,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
         .cdw11 = cpu_to_le32(0x1 | (n << 16)),
     };
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
         nvme_free_queue_pair(bs, q);
         return false;
@@ -655,16 +658,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
-    if (!s->queues[0]) {
+    s->queues[QUEUE_INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
+                                                          NVME_QUEUE_SIZE,
+                                                          errp);
+    if (!s->queues[QUEUE_INDEX_ADMIN]) {
         ret = -EINVAL;
         goto out;
     }
     s->nr_queues = 1;
     QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
     s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
-    s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
-    s->regs->acq = cpu_to_le64(s->queues[0]->cq.iova);
+    s->regs->asq = cpu_to_le64(s->queues[QUEUE_INDEX_ADMIN]->sq.iova);
+    s->regs->acq = cpu_to_le64(s->queues[QUEUE_INDEX_ADMIN]->cq.iova);
 
     /* After setting up all control registers we can enable device now. */
     s->regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << 20) |
@@ -755,7 +760,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
         .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
     };
 
-    ret = nvme_cmd_sync(bs, s->queues[0], &cmd);
+    ret = nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd);
     if (ret) {
         error_setg(errp, "Failed to configure NVMe write cache");
     }
@@ -972,7 +977,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
 {
     int r;
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[QUEUE_INDEX_IO(0)];
     NVMeRequest *req;
 
     uint32_t cdw12 = (((bytes >> s->blkshift) - 1) & 0xFFFF) |
@@ -1087,7 +1092,7 @@ static coroutine_fn int nvme_co_pwritev(BlockDriverState *bs,
 static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[QUEUE_INDEX_IO(0)];
     NVMeRequest *req;
     NvmeCmd cmd = {
         .opcode = NVME_CMD_FLUSH,
@@ -1118,7 +1123,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
                                               BdrvRequestFlags flags)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[QUEUE_INDEX_IO(0)];
     NVMeRequest *req;
 
     uint32_t cdw12 = ((bytes >> s->blkshift) - 1) & 0xFFFF;
@@ -1171,7 +1176,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
                                          int bytes)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[QUEUE_INDEX_IO(0)];
     NVMeRequest *req;
     NvmeDsmRange *buf;
     QEMUIOVector local_qiov;
@@ -1300,7 +1305,7 @@ static void nvme_aio_unplug(BlockDriverState *bs)
     BDRVNVMeState *s = bs->opaque;
     assert(s->plugged);
     s->plugged = false;
-    for (i = 1; i < s->nr_queues; i++) {
+    for (i = QUEUE_INDEX_IO(0); i < s->nr_queues; i++) {
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
         nvme_kick(s, q);
-- 
2.21.3

Re: [PATCH 03/17] block/nvme: Define QUEUE_INDEX macros to ease code review

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:24PM +0200, Philippe Mathieu-Daudé wrote:
> Use definitions instead of '0' or '1' indexes. Also this will
> be useful when using multi-queues later.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)

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

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

[PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures

[Philippe Mathieu-Daudé]

We allocate an unique chunk of memory then use it for two
different structures. Introduce the 'idsz_max' variable to
hold the maximum size, to make it clearer the size is enough
to hold the two structures.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
FIXME: reword with something that makes more sense...
---
 block/nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 71f8cf27a8..ffda804a8e 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -438,6 +438,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     BDRVNVMeState *s = bs->opaque;
     NvmeIdCtrl *idctrl;
     NvmeIdNs *idns;
+    size_t idsz_max;
     NvmeLBAF *lbaf;
     uint8_t *resp;
     uint16_t oncs;
@@ -448,14 +449,15 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .cdw10 = cpu_to_le32(0x1),
     };
 
-    resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
+    idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
+    resp = qemu_try_blockalign0(bs, idsz_max);
     if (!resp) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
     idctrl = (NvmeIdCtrl *)resp;
     idns = (NvmeIdNs *)resp;
-    r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
+    r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
-- 
2.21.3

Re: [PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:25PM +0200, Philippe Mathieu-Daudé wrote:
> We allocate an unique chunk of memory then use it for two
> different structures. Introduce the 'idsz_max' variable to
> hold the maximum size, to make it clearer the size is enough
> to hold the two structures.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> FIXME: reword with something that makes more sense...
> ---
>  block/nvme.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 71f8cf27a8..ffda804a8e 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -438,6 +438,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      BDRVNVMeState *s = bs->opaque;
>      NvmeIdCtrl *idctrl;
>      NvmeIdNs *idns;
> +    size_t idsz_max;
>      NvmeLBAF *lbaf;
>      uint8_t *resp;
>      uint16_t oncs;
> @@ -448,14 +449,15 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          .cdw10 = cpu_to_le32(0x1),
>      };
>  
> -    resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> +    idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
> +    resp = qemu_try_blockalign0(bs, idsz_max);
>      if (!resp) {
>          error_setg(errp, "Cannot allocate buffer for identify response");
>          goto out;
>      }
>      idctrl = (NvmeIdCtrl *)resp;
>      idns = (NvmeIdNs *)resp;
> -    r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
> +    r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);

_nvme_check_size() has compile-time asserts that check
sizeof(NvmeIdCtrl) == sizeof(NvmeIdNs) == 4096.

I suggest the following cleanup:

  union {
      NvmeIdCtrl ctrl;
      NvmeIdNs ns;
  } *id;
  ...
  id = qemu_try_blockalign0(bs, sizeof(*id));
  ...
  r = qemu_vfio_dma_map(s->vfio, resp, sizeof(*id), true, &iova);

and accesses to idctl are replaced with id->ctrl and idns with id->ns.

This eliminates the casts, makes it clear that this data is overlapping,
and avoids the need for idsz_max.

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

Re: [PATCH 04/17] block/nvme: Be explicit we share NvmeIdCtrl / NvmeIdNs structures

[Philippe Mathieu-Daudé]

On 6/26/20 1:19 PM, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2020 at 08:48:25PM +0200, Philippe Mathieu-Daudé wrote:
>> We allocate an unique chunk of memory then use it for two
>> different structures. Introduce the 'idsz_max' variable to
>> hold the maximum size, to make it clearer the size is enough
>> to hold the two structures.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> FIXME: reword with something that makes more sense...
>> ---
>>  block/nvme.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 71f8cf27a8..ffda804a8e 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -438,6 +438,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>      BDRVNVMeState *s = bs->opaque;
>>      NvmeIdCtrl *idctrl;
>>      NvmeIdNs *idns;
>> +    size_t idsz_max;
>>      NvmeLBAF *lbaf;
>>      uint8_t *resp;
>>      uint16_t oncs;
>> @@ -448,14 +449,15 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>          .cdw10 = cpu_to_le32(0x1),
>>      };
>>  
>> -    resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
>> +    idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
>> +    resp = qemu_try_blockalign0(bs, idsz_max);
>>      if (!resp) {
>>          error_setg(errp, "Cannot allocate buffer for identify response");
>>          goto out;
>>      }
>>      idctrl = (NvmeIdCtrl *)resp;
>>      idns = (NvmeIdNs *)resp;
>> -    r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
>> +    r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);
> 
> _nvme_check_size() has compile-time asserts that check
> sizeof(NvmeIdCtrl) == sizeof(NvmeIdNs) == 4096.
> 
> I suggest the following cleanup:
> 
>   union {
>       NvmeIdCtrl ctrl;
>       NvmeIdNs ns;
>   } *id;
>   ...
>   id = qemu_try_blockalign0(bs, sizeof(*id));
>   ...
>   r = qemu_vfio_dma_map(s->vfio, resp, sizeof(*id), true, &iova);
> 
> and accesses to idctl are replaced with id->ctrl and idns with id->ns.
> 
> This eliminates the casts, makes it clear that this data is overlapping,
> and avoids the need for idsz_max.

Clever idea, thanks!

[PATCH 05/17] block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset

[Philippe Mathieu-Daudé]

In the next commit we'll get ride of qemu_try_blockalign().
To ease review, first replace qemu_try_blockalign0() by explicit
calls to qemu_try_blockalign() and memset().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index ffda804a8e..bdddcd975d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -158,12 +158,12 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
 
     bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
     q->head = q->tail = 0;
-    q->queue = qemu_try_blockalign0(bs, bytes);
-
+    q->queue = qemu_try_blockalign(bs, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
         return;
     }
+    memset(q->queue, 0, bytes);
     r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
     if (r) {
         error_setg(errp, "Cannot map queue");
@@ -204,11 +204,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     if (!q) {
         return NULL;
     }
-    q->prp_list_pages = qemu_try_blockalign0(bs,
+    q->prp_list_pages = qemu_try_blockalign(bs,
                                           s->page_size * NVME_QUEUE_SIZE);
     if (!q->prp_list_pages) {
         goto fail;
     }
+    memset(q->prp_list_pages, 0, s->page_size * NVME_QUEUE_SIZE);
     qemu_mutex_init(&q->lock);
     q->index = idx;
     qemu_co_queue_init(&q->free_req_queue);
@@ -450,7 +451,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     };
 
     idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
-    resp = qemu_try_blockalign0(bs, idsz_max);
+    resp = qemu_try_blockalign(bs, idsz_max);
     if (!resp) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
@@ -462,6 +463,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
     }
+
+    memset(resp, 0, sizeof(NvmeIdCtrl));
     cmd.prp1 = cpu_to_le64(iova);
 
     if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
@@ -484,7 +487,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
     s->supports_discard = !!(oncs & NVME_ONCS_DSM);
 
-    memset(resp, 0, 4096);
+    memset(resp, 0, sizeof(NvmeIdNs));
 
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
@@ -1202,11 +1205,11 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
 
     assert(s->nr_queues > 1);
 
-    buf = qemu_try_blockalign0(bs, s->page_size);
+    buf = qemu_try_blockalign(bs, s->page_size);
     if (!buf) {
         return -ENOMEM;
     }
-
+    memset(buf, 0, s->page_size);
     buf->nlb = cpu_to_le32(bytes >> s->blkshift);
     buf->slba = cpu_to_le64(offset >> s->blkshift);
     buf->cattr = 0;
-- 
2.21.3

Re: [PATCH 05/17] block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:26PM +0200, Philippe Mathieu-Daudé wrote:
> In the next commit we'll get ride of qemu_try_blockalign().

s/ride/rid/

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

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

[PATCH 06/17] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Philippe Mathieu-Daudé]

qemu_try_blockalign() is a generic API that call back to the
block driver to return its page alignment. As we call from
within the very same driver, we already know to page alignment
stored in our state. Remove indirections and use the value from
BDRVNVMeState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index bdddcd975d..cec9ace3dd 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -158,7 +158,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
 
     bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
     q->head = q->tail = 0;
-    q->queue = qemu_try_blockalign(bs, bytes);
+    q->queue = qemu_try_memalign(s->page_size, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
         return;
@@ -204,7 +204,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     if (!q) {
         return NULL;
     }
-    q->prp_list_pages = qemu_try_blockalign(bs,
+    q->prp_list_pages = qemu_try_memalign(s->page_size,
                                           s->page_size * NVME_QUEUE_SIZE);
     if (!q->prp_list_pages) {
         goto fail;
@@ -451,7 +451,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     };
 
     idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
-    resp = qemu_try_blockalign(bs, idsz_max);
+    resp = qemu_try_memalign(s->page_size, idsz_max);
     if (!resp) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
@@ -1061,7 +1061,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         return nvme_co_prw_aligned(bs, offset, bytes, qiov, is_write, flags);
     }
     trace_nvme_prw_buffered(s, offset, bytes, qiov->niov, is_write);
-    buf = qemu_try_blockalign(bs, bytes);
+    buf = qemu_try_memalign(s->page_size, bytes);
 
     if (!buf) {
         return -ENOMEM;
@@ -1205,7 +1205,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
 
     assert(s->nr_queues > 1);
 
-    buf = qemu_try_blockalign(bs, s->page_size);
+    buf = qemu_try_memalign(s->page_size, s->page_size);
     if (!buf) {
         return -ENOMEM;
     }
-- 
2.21.3

Re: [PATCH 06/17] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:27PM +0200, Philippe Mathieu-Daudé wrote:
> qemu_try_blockalign() is a generic API that call back to the
> block driver to return its page alignment. As we call from
> within the very same driver, we already know to page alignment
> stored in our state. Remove indirections and use the value from
> BDRVNVMeState.

The higher-level qemu_try_blockalign() API does not require all callers
to be aware of the memory alignment details. It seems like a
disadvantage to duplicate that knowledge throughout the code, even if
it's in the same driver source code.

Is there an advantage to this patch that I've missed?

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

Re: [PATCH 06/17] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Philippe Mathieu-Daudé]

On 6/26/20 2:24 PM, Stefan Hajnoczi wrote:
> On Thu, Jun 25, 2020 at 08:48:27PM +0200, Philippe Mathieu-Daudé wrote:
>> qemu_try_blockalign() is a generic API that call back to the
>> block driver to return its page alignment. As we call from
>> within the very same driver, we already know to page alignment
>> stored in our state. Remove indirections and use the value from
>> BDRVNVMeState.
> 
> The higher-level qemu_try_blockalign() API does not require all callers
> to be aware of the memory alignment details. It seems like a
> disadvantage to duplicate that knowledge throughout the code, even if
> it's in the same driver source code.
> 
> Is there an advantage to this patch that I've missed?

This is required to later remove the BlockDriverState argument,
so nvme_init_queue() is per-hardware, not per-block-driver.

Re: [PATCH 06/17] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Stefan Hajnoczi]

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

On Fri, Jun 26, 2020 at 02:48:55PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/26/20 2:24 PM, Stefan Hajnoczi wrote:
> > On Thu, Jun 25, 2020 at 08:48:27PM +0200, Philippe Mathieu-Daudé wrote:
> >> qemu_try_blockalign() is a generic API that call back to the
> >> block driver to return its page alignment. As we call from
> >> within the very same driver, we already know to page alignment
> >> stored in our state. Remove indirections and use the value from
> >> BDRVNVMeState.
> > 
> > The higher-level qemu_try_blockalign() API does not require all callers
> > to be aware of the memory alignment details. It seems like a
> > disadvantage to duplicate that knowledge throughout the code, even if
> > it's in the same driver source code.
> > 
> > Is there an advantage to this patch that I've missed?
> 
> This is required to later remove the BlockDriverState argument,
> so nvme_init_queue() is per-hardware, not per-block-driver.

Makes sense. Please include this in the commit description.

Thanks,
Stefan

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

[PATCH 07/17] block/nvme: Move code around

[Philippe Mathieu-Daudé]

Move assignments previous to where the assigned variable is used,
to make the nvme_identify() body easier to review. No logical change.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index cec9ace3dd..1bba496294 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -456,17 +456,15 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
-    idctrl = (NvmeIdCtrl *)resp;
-    idns = (NvmeIdNs *)resp;
     r = qemu_vfio_dma_map(s->vfio, resp, idsz_max, true, &iova);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
     }
 
-    memset(resp, 0, sizeof(NvmeIdCtrl));
+    idctrl = (NvmeIdCtrl *)resp;
+    memset(idctrl, 0, sizeof(NvmeIdCtrl));
     cmd.prp1 = cpu_to_le64(iova);
-
     if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
@@ -487,8 +485,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
     s->supports_discard = !!(oncs & NVME_ONCS_DSM);
 
-    memset(resp, 0, sizeof(NvmeIdNs));
-
+    idns = (NvmeIdNs *)resp;
+    memset(idns, 0, sizeof(NvmeIdNs));
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
     if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
-- 
2.21.3

Re: [PATCH 07/17] block/nvme: Move code around

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:28PM +0200, Philippe Mathieu-Daudé wrote:
> Move assignments previous to where the assigned variable is used,
> to make the nvme_identify() body easier to review. No logical change.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

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

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

[PATCH 08/17] block/nvme: Use correct type void*

[Philippe Mathieu-Daudé]

qemu_try_memalign() returns a void*, qemu_vfio_dma_map() consumes
a void*. Drop the confusing uint8_t* type.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 1bba496294..095a8ec024 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -441,7 +441,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     NvmeIdNs *idns;
     size_t idsz_max;
     NvmeLBAF *lbaf;
-    uint8_t *resp;
+    void *resp;
     uint16_t oncs;
     int r;
     uint64_t iova;
-- 
2.21.3

Re: [PATCH 08/17] block/nvme: Use correct type void*

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:29PM +0200, Philippe Mathieu-Daudé wrote:
> qemu_try_memalign() returns a void*, qemu_vfio_dma_map() consumes
> a void*. Drop the confusing uint8_t* type.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[PATCH 09/17] block/nvme: Remove unused argument from nvme_free_queue_pair()

[Philippe Mathieu-Daudé]

nvme_free_queue_pair() doesn't use BlockDriverState, remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 095a8ec024..f87f157dc0 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -170,7 +170,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
     }
 }
 
-static void nvme_free_queue_pair(BlockDriverState *bs, NVMeQueuePair *q)
+static void nvme_free_queue_pair(NVMeQueuePair *q)
 {
     qemu_vfree(q->prp_list_pages);
     qemu_vfree(q->sq.queue);
@@ -241,7 +241,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
 
     return q;
 fail:
-    nvme_free_queue_pair(bs, q);
+    nvme_free_queue_pair(q);
     return NULL;
 }
 
@@ -568,7 +568,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     };
     if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
-        nvme_free_queue_pair(bs, q);
+        nvme_free_queue_pair(q);
         return false;
     }
     cmd = (NvmeCmd) {
@@ -579,7 +579,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     };
     if (nvme_cmd_sync(bs, s->queues[QUEUE_INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
-        nvme_free_queue_pair(bs, q);
+        nvme_free_queue_pair(q);
         return false;
     }
     s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
@@ -776,7 +776,7 @@ static void nvme_close(BlockDriverState *bs)
     BDRVNVMeState *s = bs->opaque;
 
     for (i = 0; i < s->nr_queues; ++i) {
-        nvme_free_queue_pair(bs, s->queues[i]);
+        nvme_free_queue_pair(s->queues[i]);
     }
     g_free(s->queues);
     aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
-- 
2.21.3

Re: [PATCH 09/17] block/nvme: Remove unused argument from nvme_free_queue_pair()

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:30PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_free_queue_pair() doesn't use BlockDriverState, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

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

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

[PATCH 10/17] block/nvme: Simplify nvme_init_queue() arguments

[Philippe Mathieu-Daudé]

nvme_init_queue() doesn't require BlockDriverState anymore.
Replace it by BDRVNVMeState to simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index f87f157dc0..8b6cf4c34b 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -149,10 +149,9 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
+static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
                             int nentries, int entry_bytes, Error **errp)
 {
-    BDRVNVMeState *s = bs->opaque;
     size_t bytes;
     int r;
 
@@ -225,14 +224,14 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
         req->prp_list_page = q->prp_list_pages + i * s->page_size;
         req->prp_list_iova = prp_list_iova + i * s->page_size;
     }
-    nvme_init_queue(bs, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
+    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
     }
     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
 
-    nvme_init_queue(bs, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
+    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
-- 
2.21.3

Re: [PATCH 10/17] block/nvme: Simplify nvme_init_queue() arguments

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:31PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_init_queue() doesn't require BlockDriverState anymore.
> Replace it by BDRVNVMeState to simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

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

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

[PATCH 11/17] block/nvme: Simplify nvme_create_queue_pair() arguments

[Philippe Mathieu-Daudé]

nvme_create_queue_pair() doesn't require BlockDriverState anymore.
Replace it by BDRVNVMeState to simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8b6cf4c34b..1b7b23cea4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -189,12 +189,11 @@ static void nvme_free_req_queue_cb(void *opaque)
     qemu_mutex_unlock(&q->lock);
 }
 
-static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
+static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                                              int idx, int size,
                                              Error **errp)
 {
     int i, r;
-    BDRVNVMeState *s = bs->opaque;
     Error *local_err = NULL;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
@@ -555,7 +554,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     NvmeCmd cmd;
     int queue_size = NVME_QUEUE_SIZE;
 
-    q = nvme_create_queue_pair(bs, n, queue_size, errp);
+    q = nvme_create_queue_pair(s, n, queue_size, errp);
     if (!q) {
         return false;
     }
@@ -660,7 +659,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[QUEUE_INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
+    s->queues[QUEUE_INDEX_ADMIN] = nvme_create_queue_pair(s, 0,
                                                           NVME_QUEUE_SIZE,
                                                           errp);
     if (!s->queues[QUEUE_INDEX_ADMIN]) {
-- 
2.21.3

Re: [PATCH 11/17] block/nvme: Simplify nvme_create_queue_pair() arguments

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:32PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_create_queue_pair() doesn't require BlockDriverState anymore.
> Replace it by BDRVNVMeState to simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

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

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

[PATCH 12/17] block/nvme: Simplify nvme_kick trace event

[Philippe Mathieu-Daudé]

The queues are tied to the hardware, logging the block
driver using them is irrelevant.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c       | 2 +-
 block/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 1b7b23cea4..4d2f31a9b3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -249,7 +249,7 @@ static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)
     if (s->plugged || !q->need_kick) {
         return;
     }
-    trace_nvme_kick(s, q->index);
+    trace_nvme_kick(q->index);
     assert(!(q->sq.tail & 0xFF00));
     /* Fence the write to submission queue entry before notifying the device. */
     smp_wmb();
diff --git a/block/trace-events b/block/trace-events
index 29dff8881c..f0c476110b 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -154,7 +154,7 @@ vxhs_close(char *vdisk_guid) "Closing vdisk %s"
 vxhs_get_creds(const char *cacert, const char *client_key, const char *client_cert) "cacert %s, client_key %s, client_cert %s"
 
 # nvme.c
-nvme_kick(void *s, int queue) "s %p queue %d"
+nvme_kick(int queue) "queue %d"
 nvme_dma_flush_queue_wait(void *s) "s %p"
 nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"
 nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"
-- 
2.21.3

Re: [PATCH 12/17] block/nvme: Simplify nvme_kick trace event

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:33PM +0200, Philippe Mathieu-Daudé wrote:
> The queues are tied to the hardware, logging the block
> driver using them is irrelevant.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 2 +-
>  block/trace-events | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 1b7b23cea4..4d2f31a9b3 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -249,7 +249,7 @@ static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)
>      if (s->plugged || !q->need_kick) {
>          return;
>      }
> -    trace_nvme_kick(s, q->index);
> +    trace_nvme_kick(q->index);
>      assert(!(q->sq.tail & 0xFF00));
>      /* Fence the write to submission queue entry before notifying the device. */
>      smp_wmb();
> diff --git a/block/trace-events b/block/trace-events
> index 29dff8881c..f0c476110b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -154,7 +154,7 @@ vxhs_close(char *vdisk_guid) "Closing vdisk %s"
>  vxhs_get_creds(const char *cacert, const char *client_key, const char *client_cert) "cacert %s, client_key %s, client_cert %s"
>  
>  # nvme.c
> -nvme_kick(void *s, int queue) "s %p queue %d"
> +nvme_kick(int queue) "queue %d"

BDRVNVMeState is included so it's possible to differentiate between
multiple nvme driver instances. Simply tracing the queue number is not
enough if you have multiple nvme driver instances.

I suggest leaving this change until there is a hardware state object
that can be traced instead of the BDRVNVMeState.

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

[PATCH 13/17] block/nvme: Simplify completion trace events

[Philippe Mathieu-Daudé]

The queues are tied to the hardware, logging the block
driver using them is irrelevant.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c       | 6 +++---
 block/trace-events | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 4d2f31a9b3..7b983ba4e1 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -323,9 +323,9 @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
     NVMeRequest req;
     NvmeCqe *c;
 
-    trace_nvme_process_completion(s, q->index, q->inflight);
+    trace_nvme_process_completion(q->index, q->inflight);
     if (q->busy || s->plugged) {
-        trace_nvme_process_completion_queue_busy(s, q->index);
+        trace_nvme_process_completion_queue_busy(q->index);
         return false;
     }
     q->busy = true;
@@ -347,7 +347,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
             continue;
         }
         assert(cid <= NVME_QUEUE_SIZE);
-        trace_nvme_complete_command(s, q->index, cid);
+        trace_nvme_complete_command(q->index, cid);
         preq = &q->reqs[cid - 1];
         req = *preq;
         assert(req.cid == cid);
diff --git a/block/trace-events b/block/trace-events
index f0c476110b..8c29818093 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -157,9 +157,9 @@ vxhs_get_creds(const char *cacert, const char *client_key, const char *client_ce
 nvme_kick(int queue) "queue %d"
 nvme_dma_flush_queue_wait(void *s) "s %p"
 nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"
-nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"
-nvme_process_completion_queue_busy(void *s, int index) "s %p queue %d"
-nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
+nvme_process_completion(int index, int inflight) "queue %d inflight %d"
+nvme_process_completion_queue_busy(int index) "queue %d"
+nvme_complete_command(int index, int cid) "queue %d cid %d"
 nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"
 nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"
 nvme_handle_event(void *s) "s %p"
-- 
2.21.3

Re: [PATCH 13/17] block/nvme: Simplify completion trace events

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:34PM +0200, Philippe Mathieu-Daudé wrote:
> The queues are tied to the hardware, logging the block
> driver using them is irrelevant.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c       | 6 +++---
>  block/trace-events | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)

This patch also needs to wait until there is a hardware state pointer
that can be included in the trace event.

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

[PATCH 14/17] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Philippe Mathieu-Daudé]

BDRV_POLL_WHILE() is defined as:

  #define BDRV_POLL_WHILE(bs, cond) ({          \
      BlockDriverState *bs_ = (bs);             \
      AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
                     cond); })

As we will remove the BlockDriverState use in the next commit,
start by using the exploded version of BDRV_POLL_WHILE().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 7b983ba4e1..ac933cafd0 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -419,6 +419,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
 static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
                          NvmeCmd *cmd)
 {
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     BDRVNVMeState *s = bs->opaque;
     int ret = -EINPROGRESS;
@@ -428,7 +429,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
     }
     nvme_submit_command(s, q, req, cmd, nvme_cmd_sync_cb, &ret);
 
-    BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);
+    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);
     return ret;
 }
 
-- 
2.21.3

Re: [PATCH 14/17] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:35PM +0200, Philippe Mathieu-Daudé wrote:
> BDRV_POLL_WHILE() is defined as:
> 
>   #define BDRV_POLL_WHILE(bs, cond) ({          \
>       BlockDriverState *bs_ = (bs);             \
>       AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
>                      cond); })
> 
> As we will remove the BlockDriverState use in the next commit,
> start by using the exploded version of BDRV_POLL_WHILE().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

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

[RFC PATCH 15/17] block/nvme: Use per-queue AIO context

[Philippe Mathieu-Daudé]

To be able to use multiple queues on the same hardware,
we need to have each queue able to receive IRQ notifications
in the correct AIO context.
The context has to be proper to each queue, not to the block
driver. Move aio_context from BDRVNVMeState to NVMeQueuePair.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because I'm not familiar with AIO context

 block/nvme.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index ac933cafd0..0f7cc568ef 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -51,6 +51,7 @@ typedef struct {
 } NVMeRequest;
 
 typedef struct {
+    AioContext *aio_context;
     CoQueue     free_req_queue;
     QemuMutex   lock;
 
@@ -93,7 +94,6 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 #define QUEUE_INDEX_IO(n)   (1 + n)
 
 typedef struct {
-    AioContext *aio_context;
     QEMUVFIOState *vfio;
     NVMeRegs *regs;
     /* The submission/completion queue pairs.
@@ -190,6 +190,7 @@ static void nvme_free_req_queue_cb(void *opaque)
 }
 
 static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
+                                             AioContext *aio_context,
                                              int idx, int size,
                                              Error **errp)
 {
@@ -207,6 +208,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
     if (!q->prp_list_pages) {
         goto fail;
     }
+    q->aio_context = aio_context;
     memset(q->prp_list_pages, 0, s->page_size * NVME_QUEUE_SIZE);
     qemu_mutex_init(&q->lock);
     q->index = idx;
@@ -365,7 +367,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
         smp_mb_release();
         *q->cq.doorbell = cpu_to_le32(q->cq.head);
         if (!qemu_co_queue_empty(&q->free_req_queue)) {
-            replay_bh_schedule_oneshot_event(s->aio_context,
+            replay_bh_schedule_oneshot_event(q->aio_context,
                                              nvme_free_req_queue_cb, q);
         }
     }
@@ -419,7 +421,6 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
 static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
                          NvmeCmd *cmd)
 {
-    AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     BDRVNVMeState *s = bs->opaque;
     int ret = -EINPROGRESS;
@@ -429,7 +430,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
     }
     nvme_submit_command(s, q, req, cmd, nvme_cmd_sync_cb, &ret);
 
-    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);
+    AIO_WAIT_WHILE(q->aio_context, ret == -EINPROGRESS);
     return ret;
 }
 
@@ -547,7 +548,8 @@ static void nvme_handle_event(EventNotifier *n)
     nvme_poll_queues(s);
 }
 
-static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
+static bool nvme_add_io_queue(BlockDriverState *bs,
+                              AioContext *aio_context, Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
     int n = s->nr_queues;
@@ -555,7 +557,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     NvmeCmd cmd;
     int queue_size = NVME_QUEUE_SIZE;
 
-    q = nvme_create_queue_pair(s, n, queue_size, errp);
+    q = nvme_create_queue_pair(s, aio_context, n, queue_size, errp);
     if (!q) {
         return false;
     }
@@ -600,6 +602,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
                      Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     int ret;
     uint64_t cap;
     uint64_t timeout_ms;
@@ -610,7 +613,6 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     qemu_co_queue_init(&s->dma_flush_queue);
     s->device = g_strdup(device);
     s->nsid = namespace;
-    s->aio_context = bdrv_get_aio_context(bs);
     ret = event_notifier_init(&s->irq_notifier, 0);
     if (ret) {
         error_setg(errp, "Failed to init event notifier");
@@ -660,7 +662,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[QUEUE_INDEX_ADMIN] = nvme_create_queue_pair(s, 0,
+    s->queues[QUEUE_INDEX_ADMIN] = nvme_create_queue_pair(s, aio_context, 0,
                                                           NVME_QUEUE_SIZE,
                                                           errp);
     if (!s->queues[QUEUE_INDEX_ADMIN]) {
@@ -695,7 +697,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     if (ret) {
         goto out;
     }
-    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
+    aio_set_event_notifier(aio_context, &s->irq_notifier,
                            false, nvme_handle_event, nvme_poll_cb);
 
     nvme_identify(bs, namespace, &local_err);
@@ -706,7 +708,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     }
 
     /* Set up command queues. */
-    if (!nvme_add_io_queue(bs, errp)) {
+    if (!nvme_add_io_queue(bs, aio_context, errp)) {
         ret = -EIO;
     }
 out:
@@ -775,11 +777,11 @@ static void nvme_close(BlockDriverState *bs)
     BDRVNVMeState *s = bs->opaque;
 
     for (i = 0; i < s->nr_queues; ++i) {
+        aio_set_event_notifier(s->queues[i]->aio_context,
+                               &s->irq_notifier, false, NULL, NULL);
         nvme_free_queue_pair(s->queues[i]);
     }
     g_free(s->queues);
-    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
-                           false, NULL, NULL);
     event_notifier_cleanup(&s->irq_notifier);
     qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
     qemu_vfio_close(s->vfio);
@@ -992,7 +994,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
         .cdw12 = cpu_to_le32(cdw12),
     };
     NVMeCoData data = {
-        .ctx = bdrv_get_aio_context(bs),
+        .ctx = ioq->aio_context,
         .ret = -EINPROGRESS,
     };
 
@@ -1101,7 +1103,7 @@ static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
         .nsid = cpu_to_le32(s->nsid),
     };
     NVMeCoData data = {
-        .ctx = bdrv_get_aio_context(bs),
+        .ctx = ioq->aio_context,
         .ret = -EINPROGRESS,
     };
 
@@ -1142,7 +1144,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
     };
 
     NVMeCoData data = {
-        .ctx = bdrv_get_aio_context(bs),
+        .ctx = ioq->aio_context,
         .ret = -EINPROGRESS,
     };
 
@@ -1192,7 +1194,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
     };
 
     NVMeCoData data = {
-        .ctx = bdrv_get_aio_context(bs),
+        .ctx = ioq->aio_context,
         .ret = -EINPROGRESS,
     };
 
@@ -1289,7 +1291,6 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
 {
     BDRVNVMeState *s = bs->opaque;
 
-    s->aio_context = new_context;
     aio_set_event_notifier(new_context, &s->irq_notifier,
                            false, nvme_handle_event, nvme_poll_cb);
 }
-- 
2.21.3

Re: [RFC PATCH 15/17] block/nvme: Use per-queue AIO context

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:36PM +0200, Philippe Mathieu-Daudé wrote:
> To be able to use multiple queues on the same hardware,
> we need to have each queue able to receive IRQ notifications
> in the correct AIO context.
> The context has to be proper to each queue, not to the block
> driver. Move aio_context from BDRVNVMeState to NVMeQueuePair.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because I'm not familiar with AIO context

This patch looks incomplete because there is still only 1
s->irq_notifier but the code already performs aio_set_event_notifer()
calls for each queue in nvme_close().

Either there should be 1 irq_notifier and singleton
aio_set_event_notifier() calls or irq_notifiers should really be
per-queue.

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

Re: [RFC PATCH 15/17] block/nvme: Use per-queue AIO context

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:36PM +0200, Philippe Mathieu-Daudé wrote:
> To be able to use multiple queues on the same hardware,
> we need to have each queue able to receive IRQ notifications
> in the correct AIO context.
> The context has to be proper to each queue, not to the block
> driver. Move aio_context from BDRVNVMeState to NVMeQueuePair.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because I'm not familiar with AIO context

To keep things simple I suggest only doing Step 1 in this patch series.

Step 1:
The existing irq_notifier handler re-enters the request coroutine from a
BH scheduled in the BlockDriverState's AioContext. It doesn't matter
where the irq_notifier is handled, the completions will run in their
respective BlockDriverState AioContexts. This means that two
BlockDriverStates with different AioContexts sharing a single hardware
state will work correctly with just a single hardware queue. Therefore
multiqueue support is not required to support multiple BDSes with
different AioContexts.

Step 2:
Better performance can be achieved by creating multiple hardware
queuepairs, each with its own irq_notifier. During request submission a
int queue_idx_from_aio_context(AioContext *ctx) mapping function selects
a hardware queue. Hopefully that hardware queue's irq_notifier is
handled in the same Aiocontext for best performance, but there might be
cases where there are more BDS AioContexts than nvme hw queues.

Step 3:
When the QEMU block layer has multiqueue support then we'll no longer
map the BlockDriverState AioContext to a queue index but instead use
qemu_get_current_aio_context(). At this point a single BDS can process
I/O in multiple AioContexts and hardware queuepairs.

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

[PATCH 16/17] block/nvme: Check BDRVNVMeState::plugged out of nvme_kick()

[Philippe Mathieu-Daudé]

The queues are tied to the hardware, not to the block driver.
As this function doesn't need to know about the BDRVNVMeState,
move the 'plugged' check to the caller.
Since in nvme_aio_unplug() we know that s->plugged is false,
we don't need the check.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 0f7cc568ef..b335dfdb73 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -246,9 +246,9 @@ fail:
 }
 
 /* With q->lock */
-static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)
+static void nvme_kick(NVMeQueuePair *q)
 {
-    if (s->plugged || !q->need_kick) {
+    if (!q->need_kick) {
         return;
     }
     trace_nvme_kick(q->index);
@@ -406,7 +406,9 @@ static void nvme_submit_command(BDRVNVMeState *s, NVMeQueuePair *q,
            q->sq.tail * NVME_SQ_ENTRY_BYTES, cmd, sizeof(*cmd));
     q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;
     q->need_kick++;
-    nvme_kick(s, q);
+    if (!s->plugged) {
+        nvme_kick(q);
+    }
     nvme_process_completion(s, q);
     qemu_mutex_unlock(&q->lock);
 }
@@ -1311,7 +1313,7 @@ static void nvme_aio_unplug(BlockDriverState *bs)
     for (i = QUEUE_INDEX_IO(0); i < s->nr_queues; i++) {
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
-        nvme_kick(s, q);
+        nvme_kick(q);
         nvme_process_completion(s, q);
         qemu_mutex_unlock(&q->lock);
     }
-- 
2.21.3

Re: [PATCH 16/17] block/nvme: Check BDRVNVMeState::plugged out of nvme_kick()

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:37PM +0200, Philippe Mathieu-Daudé wrote:
> The queues are tied to the hardware, not to the block driver.
> As this function doesn't need to know about the BDRVNVMeState,
> move the 'plugged' check to the caller.
> Since in nvme_aio_unplug() we know that s->plugged is false,
> we don't need the check.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

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

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

[PATCH 17/17] block/nvme: Check BDRVNVMeState::plugged out of nvme_process_completion

[Philippe Mathieu-Daudé]

The queues are tied to the hardware, not to the block driver.
As this function doesn't need to know about the BDRVNVMeState,
move the 'plugged' check to the caller.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index b335dfdb73..03658776f4 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -318,7 +318,7 @@ static inline int nvme_translate_error(const NvmeCqe *c)
 }
 
 /* With q->lock */
-static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
+static bool nvme_process_completion(NVMeQueuePair *q)
 {
     bool progress = false;
     NVMeRequest *preq;
@@ -326,7 +326,7 @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)
     NvmeCqe *c;
 
     trace_nvme_process_completion(q->index, q->inflight);
-    if (q->busy || s->plugged) {
+    if (q->busy) {
         trace_nvme_process_completion_queue_busy(q->index);
         return false;
     }
@@ -408,8 +408,8 @@ static void nvme_submit_command(BDRVNVMeState *s, NVMeQueuePair *q,
     q->need_kick++;
     if (!s->plugged) {
         nvme_kick(q);
+        nvme_process_completion(q);
     }
-    nvme_process_completion(s, q);
     qemu_mutex_unlock(&q->lock);
 }
 
@@ -529,10 +529,13 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
     bool progress = false;
     int i;
 
+    if (s->plugged) {
+        return false;
+    }
     for (i = 0; i < s->nr_queues; i++) {
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
-        while (nvme_process_completion(s, q)) {
+        while (nvme_process_completion(q)) {
             /* Keep polling */
             progress = true;
         }
@@ -1314,7 +1317,7 @@ static void nvme_aio_unplug(BlockDriverState *bs)
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
         nvme_kick(q);
-        nvme_process_completion(s, q);
+        nvme_process_completion(q);
         qemu_mutex_unlock(&q->lock);
     }
 }
-- 
2.21.3

Re: [PATCH 17/17] block/nvme: Check BDRVNVMeState::plugged out of nvme_process_completion

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2020 at 08:48:38PM +0200, Philippe Mathieu-Daudé wrote:
> @@ -529,10 +529,13 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
>      bool progress = false;
>      int i;
>  
> +    if (s->plugged) {
> +        return false;
> +    }
>      for (i = 0; i < s->nr_queues; i++) {
>          NVMeQueuePair *q = s->queues[i];
>          qemu_mutex_lock(&q->lock);
> -        while (nvme_process_completion(s, q)) {
> +        while (nvme_process_completion(q)) {
>              /* Keep polling */
>              progress = true;
>          }

This code transformation is correct but I hope plugged can be removed
from the completion code path in the future since its purpose is for
batching submissions.

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

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

Re: [PATCH 00/17] block/nvme: Various cleanups required to use multiple queues

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200625184838.28172-1-philmd@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/write-threshold.o
  CC      block/backup.o
/tmp/qemu-test/src/block/nvme.c: In function 'nvme_identify':
/tmp/qemu-test/src/block/nvme.c:455:5: error: implicit declaration of function 'MAX_CONST' [-Werror=implicit-function-declaration]
     idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
     ^
/tmp/qemu-test/src/block/nvme.c:455:5: error: nested extern declaration of 'MAX_CONST' [-Werror=nested-externs]
cc1: all warnings being treated as errors
make: *** [block/nvme.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 669, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=30c72f7825134f5ba69d45b0cec6f72a', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-hnn82ywr/src/docker-src.2020-06-25-15.24.11.6586:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=30c72f7825134f5ba69d45b0cec6f72a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-hnn82ywr/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m55.230s
user    0m8.517s


The full log is available at
http://patchew.org/logs/20200625184838.28172-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 00/17] block/nvme: Various cleanups required to use multiple queues

[Philippe Mathieu-Daudé]

On 6/25/20 9:27 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200625184838.28172-1-philmd@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   CC      block/write-threshold.o
>   CC      block/backup.o
> /tmp/qemu-test/src/block/nvme.c: In function 'nvme_identify':
> /tmp/qemu-test/src/block/nvme.c:455:5: error: implicit declaration of function 'MAX_CONST' [-Werror=implicit-function-declaration]
>      idsz_max = MAX_CONST(sizeof(NvmeIdCtrl), sizeof(NvmeIdNs));
>      ^

I include in the cover:

Based-on: <20200625162602.700741-1-eblake@redhat.com>

Is patchew confused?

Ah, I get 404 on:
https://patchew.org/QEMU/20200625162602.700741-1-eblake@redhat.com/

So the mail got lost, 'Based-on' tag is ignored and the series still
applied?