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

[Keith Busch <keith.busch@intel.com>]

On Mon, Jan 30, 2017 at 07:13:51PM +0100, Christoph Hellwig wrote:
> Support deallocating of LBAs using the DSM command by wiring it up to
> the qemu discard implementation.  The other DSM operations which are
> purely advisory are ignored for now.
> 
> Based on an implementation by Keith Busch in the qemu-nvme.git repository,
> but rewritten to use the qemu AIO infrastructure properly to not block
> the main thread on discard requests, and cleaned up a little bit.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for doing this. I've one comment below, but I don't think it should
block this unless you see a good way to fix it.

Reviewed-by: Keith Busch <keith.busch@intel.com>

> +    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;
> +        }
> +
> +        req->aio_inflight++;
> +        req->aiocb = blk_aio_pdiscard(n->conf.blk, slba << data_shift,
> +                                      nlb << data_shift, nvme_discard_cb, req);

Overwriting the request's aiocb here for a multiple ranges is potentially
a problem if we have to cancel the request later. That scenario is
extremely unlikely, though, so I don't think it's worth addressing.