nvme: rewrite discard support

From: Ming Lin <[email protected]>

This rewrites nvme_setup_discard() similar as sd_setup_discard_cmnd(). And moves it to common code so fabrics driver can also use it.

---
 drivers/nvme/host/nvme.h | 26 ++++++++++++++++++++
 drivers/nvme/host/pci.c  | 64 +++++++++++++++++++++---------------------------
 2 files changed, 54 insertions(+), 36 deletions(-)

index 75982b9..c757746 100644
@@ -231,6 +231,32 @@ static inline void nvme_setup_rw(struct nvme_ns *ns, struct request *req,
 	cmnd->rw.dsmgmt = cpu_to_le32(dsmgmt);
 }
 
+static inline int nvme_setup_discard(struct nvme_ns *ns,
+		struct request *req, struct nvme_command *cmnd)
+{
+	struct nvme_dsm_range *range;
+	struct page *page;
+
+	page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+	if (!page)
+		return BLKPREP_DEFER;
+
+	range = page_address(page);
+	range->cattr = cpu_to_le32(0);
+	range->nlb = cpu_to_le32(blk_rq_bytes(req) >> ns->lba_shift);
+	range->slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+
+	memset(cmnd, 0, sizeof(*cmnd));
+	cmnd->dsm.opcode = nvme_cmd_dsm;
+	cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
+	cmnd->dsm.nr = 0;
+	cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
+
+	req->completion_data = page;
+	blk_add_request_payload(req, page, sizeof(struct nvme_dsm_range));
+
+	return 0;
+}
 
 static inline int nvme_error_status(u16 status)
 {

1.9.1

Initial thoughts on this, I see the appeal to having a generic NVMe discard setup, but I liked the existing implementation not needing a special case for discard check on every IO in the completion handler.

Allocating a page for just 16 bytes of data seems a bit wasteful even if it is short-lived. When it was allocating 256b from the DMA pool, the smaller amount of additional data seemed worth it for not creating a conditional completion.

How about moving the prp_small_pool to the core(and rename it to nvme_small_pool)?

Then DISCARD can also use this pool. So we don't need a special case check in the completion handler.

This looks fine now, although if we'd ever support SGLs for PCIe it wouldn't work anymore either way.

I think we should just do a 16 byte (or rather cache line size) kmalloc, as there is no need for a mempool for discards. Would a version of that be fine with you? Because discard is currently the only thing preventing us from having a split between transport and commanset specific setup for NVMe. And as things like Fabrics and LighNVM show I think a clean split there would be rather benefitical in the long run.

We only used the mempool for the driver to leverage normal IO clean up so that discard isn't special.

It looks like SCSI's special handling lead to what the code comments describe as a "horrible hack", so that's not convincing this is a good example to follow.

Nah, the "horrible hack" is adding a payload to the request. We went over and over this and everyone but Kent agrees it's the least horrible option to handle discards in generic code. NVMe side steps this a bit by not trying to handle this in common code, but that only works as long as it's a single tightly integrated driver, not for a layer architecture like SCSI, or what NVMe is increasingly moving to.

Fair enough. Every discard payload is specific to the protocol, so understood on the need. General consensus prevails.

Let's see the next version. It'd be nice if we can get away with allocating only the necessary amount for the payload.

Final comment on the original patch: it shouldn't return BLKPREP_DEFER when out of memory as it's not applicable for blk-mq drivers.