NVMe: Add discard support for capable devices
Subsystems (2)
PATCH 1/1 NVMe: Add discard support for capable devices
This adds discard support to block queues if the nvme device is capable of deallocating blocks as indicated by the controller's optional command support. A discard flagged bio request will submit an NVMe deallocate Data Set Management command for the requested blocks.
--- drivers/block/nvme.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/nvme.h | 32 +++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 0 deletions(-)
drivers/block/nvme.c +54 -0
@@ -79,6 +79,7 @@ struct nvme_dev {char model[40 ];char firmware_rev[8 ];u32 max_hw_sectors; +u16 oncs; };/*
@@ -334,6 +335,7 @@ nvme_alloc_iod(unsigned nseg, unsigned nbytes, gfp_t gfp) iod->offset = offsetof(struct nvme_iod, sg[nseg]); iod->npages =-1 ; iod->length = nbytes; + iod->nents =0 ; }return iod;
@@ -505,6 +507,39 @@ static int nvme_map_bio(struct device *dev, struct nvme_iod *iod,return length; } +static int nvme_submit_discard (struct nvme_queue *nvmeq,struct nvme_ns *ns, +struct bio *bio,struct nvme_iod *iod,int cmdid) +{ +struct nvme_dsm_range *range; +struct nvme_command *cmnd = &nvmeq->sq_cmds[nvmeq->sq_tail]; + + range =dma_pool_alloc (nvmeq->dev->prp_small_pool,GFP_ATOMIC , + &iod->first_dma); +if (range ==NULL ) +return -ENOMEM ; + +iod_list (iod)[0 ] = (__le64 *)range; + iod->npages =0 ; + + range->cattr =cpu_to_le32 (0 ); + range->nlb =cpu_to_le32 (bio->bi_size >> ns->lba_shift); + range->slba =cpu_to_le64 (bio->bi_sector >> (ns->lba_shift -9 )); + +memset (cmnd,0 ,sizeof (*cmnd)); + cmnd->dsm.opcode = nvme_cmd_dsm; + cmnd->dsm.command_id = cmdid; + cmnd->dsm.nsid =cpu_to_le32 (ns->ns_id); + cmnd->dsm.prp1 =cpu_to_le64 (iod->first_dma); + cmnd->dsm.nr =0 ; + cmnd->dsm.attributes =cpu_to_le32 (NVME_DSMGMT_AD ); + +if (++nvmeq->sq_tail == nvmeq->q_depth) + nvmeq->sq_tail =0 ; +writel (nvmeq->sq_tail, nvmeq->q_db); + +return 0 ; +} +static int nvme_submit_flush (struct nvme_queue *nvmeq,struct nvme_ns *ns,int cmdid) {
@@ -562,6 +597,8 @@ static int nvme_submit_bio_queue(struct nvme_queue *nvmeq, struct nvme_ns *ns,if (unlikely (cmdid <0 )) goto free_iod; +if (bio->bi_rw &REQ_DISCARD ) +return nvme_submit_discard (nvmeq, ns, bio, iod, cmdid);if ((bio->bi_rw &REQ_FLUSH ) && !psegs)return nvme_submit_flush (nvmeq, ns, cmdid);
@@ -1316,6 +1353,19 @@ static void nvme_put_ns_idx(int index)spin_unlock (&dev_list_lock); } +static void nvme_config_discard (struct nvme_ns *ns) +{ +u32 logical_block_size =queue_logical_block_size (ns->queue); +sector_t nr_sectors =get_capacity (ns->disk); + + ns->queue->limits.discard_zeroes_data =0 ; + ns->queue->limits.discard_alignment = logical_block_size; + ns->queue->limits.discard_granularity = logical_block_size; + ns->queue->limits.max_discard_sectors = (u32 )min_t (u64, nr_sectors, +0xffffffff ); +queue_flag_set_unlocked (QUEUE_FLAG_DISCARD , ns->queue); +} +static struct nvme_ns *nvme_alloc_ns (struct nvme_dev *dev,int nsid,struct nvme_id_ns *id,struct nvme_lba_range_type *rt) {
@@ -1361,6 +1411,9 @@ static struct nvme_ns *nvme_alloc_ns(struct nvme_dev *dev, int nsid,sprintf (disk->disk_name,"nvme%dn%d" , dev->instance, nsid);set_capacity (disk,le64_to_cpup (&id->nsze) << (ns->lba_shift -9 )); +if (dev->oncs &NVME_CTRL_ONCS_DSM ) +nvme_config_discard (ns); +return ns; out_free_queue:
@@ -1489,6 +1542,7 @@ static int __devinit nvme_dev_add(struct nvme_dev *dev) ctrl = mem; nn =le32_to_cpup (&ctrl->nn); + dev->oncs =le16_to_cpup (&ctrl->oncs);memcpy (dev->serial, ctrl->sn,sizeof (ctrl->sn));memcpy (dev->model, ctrl->mn,sizeof (ctrl->mn));memcpy (dev->firmware_rev, ctrl->fr,sizeof (ctrl->fr));
include/linux/nvme.h +32 -0
@@ -107,6 +107,12 @@ struct nvme_id_ctrl {__u8 vs[1024 ]; }; +enum { +NVME_CTRL_ONCS_COMPARE =1 <<0 , +NVME_CTRL_ONCS_WRITE_UNCORRECTABLE =1 <<1 , +NVME_CTRL_ONCS_DSM =1 <<2 , +}; +struct nvme_lbaf {__le16 ms;__u8 ds;
@@ -218,6 +224,31 @@ enum { NVME_RW_DSM_COMPRESSED = 1 << 7, }; +struct nvme_dsm_cmd { +__u8 opcode; +__u8 flags; +__u16 command_id; +__le32 nsid; +__u64 rsvd2[2 ]; +__le64 prp1; +__le64 prp2; +__le32 nr; +__le32 attributes; +__u32 rsvd12[4 ]; +}; + +enum { +NVME_DSMGMT_IDR =1 <<0 , +NVME_DSMGMT_IDW =1 <<1 , +NVME_DSMGMT_AD =1 <<2 , +}; + +struct nvme_dsm_range { +__le32 cattr; +__le32 nlb; +__le64 slba; +}; +/* Admin commands */ enum nvme_admin_opcode {
@@ -344,6 +375,7 @@ struct nvme_command {struct nvme_create_sq create_sq;struct nvme_delete_queue delete_queue;struct nvme_download_firmware dlfw; +struct nvme_dsm_cmd dsm; }; };
Signature · 1 line
1.7.0.4
PATCH 1/1 NVMe: Add discard support for capable devices
Quoted 4 lines · Keith Busch
This adds discard support to block queues if the nvme device is capable of deallocating blocks as indicated by the controller's optional command support. A discard flagged bio request will submit an NVMe deallocate Data Set Management command for the requested blocks.
Thanks for this.
drivers/block/nvme.c nvme_alloc_iod(unsigned nseg, unsigned nbytes, gfp_t gfp) · line 341
Quoted 8 lines
drivers/block/nvme.c +1
-0@@ -334,6 +335,7 @@ nvme_alloc_iod(unsigned nseg, unsigned nbytes, gfp_t gfp) iod->offset = offsetof(struct nvme_iod, sg[nseg]); iod->npages =-1 ; iod->length = nbytes; + iod->nents =0 ; }return iod;
Are you initialising nents here to prevent having to do so in nvme_submit_discard? I think we're relying on undefined behaviour of dma_unmap_sg() in bio_completion() -- ie to be a no-op when nents is 0. Probably we should check it explicitly; ie:
if (iod->nents) dma_unmap_sg(...);
We're already relying on that behaviour for the case of submitting a flush without data, so can you split this into two patches; one that initialises iod->nents to 0 and checks it, then a second one that adds the discard functionality?
drivers/block/nvme.c static int nvme_map_bio(struct device *dev, struct nvme_iod *iod, · line 514
Quoted 9 lines
drivers/block/nvme.c +5
-0@@ -505,6 +507,39 @@ static int nvme_map_bio(struct device *dev, struct nvme_iod *iod,return length; } +static int nvme_submit_discard (struct nvme_queue *nvmeq,struct nvme_ns *ns, +struct bio *bio,struct nvme_iod *iod,int cmdid) +{ +struct nvme_dsm_range *range; +struct nvme_command *cmnd = &nvmeq->sq_cmds[nvmeq->sq_tail];
Could we have a comment somewhere (possibly here) about how we're reusing the prp small pool for this purpose instead of creating a special pool? Maybe include the justification that it's not worth creating a new pool to dole out these 16-byte ranges.
drivers/block/nvme.c static int nvme_map_bio(struct device *dev, struct nvme_iod *iod, · line 519
Quoted 4 lines
drivers/block/nvme.c +4
-0+ range =dma_pool_alloc (nvmeq->dev->prp_small_pool,GFP_ATOMIC , + &iod->first_dma); +if (range ==NULL ) +return -ENOMEM ;
Usual style is
if (!range)
But ... we've got a problem here. If this allocation fails, we leak the nvme_iod and the cmdid. We need to change the code below (skip down to [A]):
Review static int nvme_map_bio(struct device *dev, struct nvme_iod *iod, · line 520
Quoted 2 lines
(quoted diff) +2
-0+ + iod_list(iod)[0] = (__le64 *)range;
Brave. I like it :-)
Quoted 25 lines
drivers/block/nvme.c +21
-0+ iod->npages =0 ; + + range->cattr =cpu_to_le32 (0 ); + range->nlb =cpu_to_le32 (bio->bi_size >> ns->lba_shift); + range->slba =cpu_to_le64 (bio->bi_sector >> (ns->lba_shift -9 )); + +memset (cmnd,0 ,sizeof (*cmnd)); + cmnd->dsm.opcode = nvme_cmd_dsm; + cmnd->dsm.command_id = cmdid; + cmnd->dsm.nsid =cpu_to_le32 (ns->ns_id); + cmnd->dsm.prp1 =cpu_to_le64 (iod->first_dma); + cmnd->dsm.nr =0 ; + cmnd->dsm.attributes =cpu_to_le32 (NVME_DSMGMT_AD ); + +if (++nvmeq->sq_tail == nvmeq->q_depth) + nvmeq->sq_tail =0 ; +writel (nvmeq->sq_tail, nvmeq->q_db); + +return 0 ; +} +static int nvme_submit_flush (struct nvme_queue *nvmeq,struct nvme_ns *ns,int cmdid) {
drivers/block/nvme.c static int nvme_submit_bio_queue(struct nvme_queue *nvmeq, struct nvme_ns *ns, · line 601
Quoted 5 lines
drivers/block/nvme.c +2
-0@@ -562,6 +597,8 @@ static int nvme_submit_bio_queue(struct nvme_queue *nvmeq, struct nvme_ns *ns,if (unlikely (cmdid <0 )) goto free_iod; +if (bio->bi_rw &REQ_DISCARD ) +return nvme_submit_discard (nvmeq, ns, bio, iod, cmdid);
[A] If this fails, we need to do this:
+ if (bio->bi_rw & REQ_DISCARD) {
+ result = nvme_submit_discard(nvmeq, ns, bio, iod, cmdid);
+ if (result)
+ goto free_iod;
+ }
Although ... now I'm looking at the failure path in nvme_submit_bio_queue(), isn't it the case that we'll leak cmdids if nvme_map_bio() fails? And in this case? So I think we also need:
+ free_cmdid:
+ free_cmdid(nvmeq, cmdid, NULL);
free_iod:
And then change the 'goto free_iod' to 'goto free_cmdid'.
Quoted 7 lines
drivers/block/nvme.c +0
-0if ((bio->bi_rw &REQ_FLUSH ) && !psegs)return nvme_submit_flush (nvmeq, ns, cmdid);@@ -1316,6 +1353,19 @@ static void nvme_put_ns_idx(int index)spin_unlock (&dev_list_lock); }
drivers/block/nvme.c static void nvme_put_ns_idx(int index) · line 1365
Quoted 10 lines
drivers/block/nvme.c +10
-0+static void nvme_config_discard (struct nvme_ns *ns) +{ +u32 logical_block_size =queue_logical_block_size (ns->queue); +sector_t nr_sectors =get_capacity (ns->disk); + + ns->queue->limits.discard_zeroes_data =0 ; + ns->queue->limits.discard_alignment = logical_block_size; + ns->queue->limits.discard_granularity = logical_block_size; + ns->queue->limits.max_discard_sectors = (u32 )min_t (u64, nr_sectors, +0xffffffff );
I don't think we need to set the limit to the maximum size of the block device ... I don't see any other driver doing this. Just:
+ ns->queue->limits.max_discard_sectors = 0xffffffff;
should be fine (and delete the get_capacity() line).
PATCH 1/1 NVMe: Add discard support for capable devices
Quoted 9 lines · Matthew Wilcox
Although ... now I'm looking at the failure path in
nvme_submit_bio_queue(), isn't it the case that we'll leak cmdids ifnvme_map_bio()fails? And in this case? So I think we also need:+
free_cmdid:(quoted diff) +1
-0+ free_cmdid(nvmeq, cmdid, NULL); free_iod:And then change the 'goto
free_iod' to 'gotofree_cmdid'.
Never mind; just got to the patch where you fixed this :-)
PATCH 1/1 NVMe: Add discard support for capable devices
Yes, though I should have made this patch dependent on the cmdid patch so I could use the right error handling here. I'll submit an update to reflect this and your other comments. Thanks for the review, I very much appreciate the feedback.
Quoted 19 lines
-----Original Message-----
From: Matthew Wilcox [mailto:willy at linux.intel.com]
Sent: Tuesday, November 06, 2012 9:15 AM
To: Busch, Keith
Cc: linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: Add discard support for capable devicesQuoted 9 lines · Matthew Wilcox
Although ... now I'm looking at the failure path in
nvme_submit_bio_queue(), isn't it the case that we'll leak cmdids ifnvme_map_bio()fails? And in this case? So I think we also need:+
free_cmdid:(quoted diff) +1
-0+ free_cmdid(nvmeq, cmdid, NULL); free_iod:And then change the 'goto
free_iod' to 'gotofree_cmdid'.Never mind; just got to the patch where you fixed this :-)