Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

[Keith Busch <kbusch@kernel.org>]

On Mon, Jan 25, 2021 at 09:42:31PM +0100, Klaus Jensen wrote:
> @@ -1644,10 +1679,56 @@ static uint16_t nvme_compare(NvmeCtrl *n, NvmeRequest *req)
>  
>  static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
>  {
> -    block_acct_start(blk_get_stats(req->ns->blkconf.blk), &req->acct, 0,
> -                     BLOCK_ACCT_FLUSH);
> -    req->aiocb = blk_aio_flush(req->ns->blkconf.blk, nvme_rw_cb, req);
> -    return NVME_NO_COMPLETE;
> +    uint32_t nsid = le32_to_cpu(req->cmd.nsid);
> +    uintptr_t *num_flushes = (uintptr_t *)&req->opaque;
> +    uint16_t status;
> +    struct nvme_aio_flush_ctx *ctx;
> +    NvmeNamespace *ns;
> +
> +    trace_pci_nvme_flush(nvme_cid(req), nsid);
> +
> +    if (nsid != NVME_NSID_BROADCAST) {
> +        req->ns = nvme_ns(n, nsid);
> +        if (unlikely(!req->ns)) {
> +            return NVME_INVALID_FIELD | NVME_DNR;
> +        }
> +
> +        block_acct_start(blk_get_stats(req->ns->blkconf.blk), &req->acct, 0,
> +                         BLOCK_ACCT_FLUSH);
> +        req->aiocb = blk_aio_flush(req->ns->blkconf.blk, nvme_rw_cb, req);
> +        return NVME_NO_COMPLETE;
> +    }
> +
> +    /* 1-initialize; see comment in nvme_dsm */
> +    *num_flushes = 1;
> +
> +    for (int i = 1; i <= n->num_namespaces; i++) {
> +        ns = nvme_ns(n, i);
> +        if (!ns) {
> +            continue;
> +        }
> +
> +        ctx = g_new(struct nvme_aio_flush_ctx, 1);
> +        ctx->req = req;
> +        ctx->ns = ns;
> +
> +        (*num_flushes)++;
> +
> +        block_acct_start(blk_get_stats(ns->blkconf.blk), &ctx->acct, 0,
> +                         BLOCK_ACCT_FLUSH);
> +        req->aiocb = blk_aio_flush(ns->blkconf.blk, nvme_aio_flush_cb, ctx);
> +    }

Overwriting req->aiocb with the most recent flush request doesn't seem
right.

This whole implementation would be much simpler with the synchronous
blk_flush() routine instead of the AIO equivalent. This is not really a
performant feature, so I don't think it's critical to get these
operations happening in parallel. What do you think?