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