Re: [Qemu-devel] [PATCH v5 3/9] block: Add VFIO based NVMe driver

[Stefan Hajnoczi]

On Fri, Jan 12, 2018 at 04:55:49PM +0800, Fam Zheng wrote:
> +    if (progress) {
> +        /* Notify the device so it can post more completions. */
> +        smp_mb_release();
> +        *q->cq.doorbell = cpu_to_le32(q->cq.head);
> +        if (!qemu_co_queue_empty(&q->free_req_queue)) {
> +            aio_bh_schedule_oneshot(s->aio_context, nvme_free_req_queue_cb, 
> q);
> +        }

This is not thread-safe because the queue producer does:

1   qemu_mutex_unlock(&q->lock);
2   qemu_co_queue_wait(&q->free_req_queue, NULL);
3   qemu_mutex_lock(&q->lock);

We fail to call nvme_free_req_queue_cb() when if
(!qemu_co_queue_empty(&q->free_req_queue)) runs after 1 but before 2.

This is only an issue if one thread runs the queue producer and another
runs the consumer code.  I don't know if that will ever be the case,
even with multiqueue, but I wanted to point this out so you and Paolo
can decide.

> +static int nvme_file_open(BlockDriverState *bs, QDict *options, int flags,
> +                          Error **errp)
> +{
> +    const char *device;
> +    QemuOpts *opts;
> +    int namespace;
> +
> +    opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &error_abort);
> +    device = qemu_opt_get(opts, NVME_BLOCK_OPT_DEVICE);
> +    if (!device) {
> +        error_setg(errp, "'" NVME_BLOCK_OPT_DEVICE "' option is required");
> +        qemu_opts_del(opts);
> +        return -EINVAL;
> +    }
> +
> +    namespace = qemu_opt_get_number(opts, NVME_BLOCK_OPT_NAMESPACE, 1);
> +    nvme_init(bs, device, namespace, errp);
> +
> +    qemu_opts_del(opts);
> +    bs->supported_write_flags = BDRV_REQ_FUA;
> +    if (nvme_enable_disable_write_cache(bs, !(flags & BDRV_O_NOCACHE), 
> errp)) {
> +        return -EINVAL;

Everything allocated in nvme_init() is leaked.

signature.asc
Description: PGP signature