Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] nvme: implement the DSM command

[Stefan Hajnoczi]

On Mon, Jan 30, 2017 at 07:13:51PM +0100, Christoph Hellwig wrote:
> +static uint16_t nvme_dsm_discard(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd 
> *cmd,
> +    NvmeRequest *req)
> +{
> +    uint16_t nr = (le32_to_cpu(cmd->cdw10) & 0xff) + 1;
> +    uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
> +    uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds - BDRV_SECTOR_BITS;
> +    NvmeDsmRange *range;
> +    QEMUSGList qsg;
> +    int i;
> +
> +    range = g_new(NvmeDsmRange, nr);
> +
> +    if (nvme_map_prp(&qsg, le64_to_cpu(cmd->prp1), le64_to_cpu(cmd->prp2),
> +            sizeof(range), n)) {
> +        goto out_free_range;
> +    }
> +
> +    if (dma_buf_write((uint8_t *)range, sizeof(range), &qsg)) {

Did you mean sizeof(*range) * nr?

> +        goto out_destroy_qsg;
> +    }
> +
> +    qemu_sglist_destroy(&qsg);
> +
> +    req->status = NVME_SUCCESS;
> +    req->has_sg = false;
> +    req->aio_inflight = 1;
> +
> +    for (i = 0; i < nr; i++) {
> +        uint64_t slba = le64_to_cpu(range[i].slba);
> +        uint32_t nlb = le32_to_cpu(range[i].nlb);
> +
> +        if (slba + nlb > le64_to_cpu(ns->id_ns.nsze)) {
> +            return NVME_LBA_RANGE | NVME_DNR;

aio requests are potentially in flight, range is still allocated, and
req->aio_inflight still needs to be decremented once.  Is there cleanup
code missing here at least so range will be freed?

> +        }
> +
> +        req->aio_inflight++;
> +        req->aiocb = blk_aio_pdiscard(n->conf.blk, slba << data_shift,
> +                                      nlb << data_shift, nvme_discard_cb, 
> req);
> +    }
> +
> +    g_free(range);
> +
> +    if (--req->aio_inflight > 0) {
> +        return NVME_NO_COMPLETE;
> +    }
> +
> +    return NVME_SUCCESS;
> +
> +out_destroy_qsg:
> +    qemu_sglist_destroy(&qsg);
> +out_free_range:
> +    g_free(range);
> +    return NVME_INVALID_FIELD | NVME_DNR;
> +}

signature.asc
Description: PGP signature