Skip to contentSkip to conversation

Drives with MDTS set to zero

PATCH Paul Grabinar 2 participants 5 messages Nov 17, 2015 – Nov 19, 2015 NVMe

lore
Share & tools
lore

Paul Grabinar

author
#0 lore raw

Hi,

I have a drive that sets MDTS to zero. According to the NVMe specification, that is valid and means that the drive does not have a limit on the transfer size.

In the current 4.4 kernel driver, we detect this and set dev->max_hw_sectors to UINT_MAX.

Later, when a namespace is allocated, we set the block level max_hw_sectors and max_segments based on dev->max_hw_sectors. This operations perform shifts on the value, but we are already at UINT_MAX, so end up with strange results.

I'm not entirely sure what is supposed to happen. The following patch sets max_hw_sectors to

BLK_DEF_MAX_SECTORS and max_segments to BLK_MAX_SEGMENTS, but this still does not seem right, as drives with a large MDTS will set max_segments higher than this.

drivers/nvme/host/pci.c +6 -7
index 8187df2..be35401 100644
@@ -2265,11 +2265,12 @@ static void nvme_alloc_ns(struct nvme_dev *dev, unsigned nsid)
 	list_add_tail(&ns->list, &dev->namespaces);
 
 	blk_queue_logical_block_size(ns->queue, 1 << ns->lba_shift);
-	if (dev->max_hw_sectors) {
-		blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors);
-		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev->page_size) + 1);
-	}
+	blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors ?
+		dev->max_hw_sectors : BLK_DEF_MAX_SECTORS);
+	blk_queue_max_segments(ns->queue,
+		dev->max_hw_sectors ?
+		((dev->max_hw_sectors << 9) / dev->page_size) + 1 :
+		BLK_MAX_SEGMENTS);
 	if (dev->stripe_size)
 		blk_queue_chunk_sectors(ns->queue, dev->stripe_size >> 9);
 	if (dev->vwc & NVME_CTRL_VWC_PRESENT)

@@ -2622,8 +2623,6 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	memcpy(dev->firmware_rev, ctrl->fr, sizeof(ctrl->fr));
 	if (ctrl->mdts)
 		dev->max_hw_sectors = 1 << (ctrl->mdts + shift - 9);
-	else
-		dev->max_hw_sectors = UINT_MAX;
 	if ((pdev->vendor == PCI_VENDOR_ID_INTEL) &&
 			(pdev->device == 0x0953) && ctrl->vs[3]) {
 		unsigned int max_hw_sectors;
#1 lore raw
Quoted 12 lines · Paul Grabinar

I have a drive that sets MDTS to zero. According to the NVMe specification, that is valid and means that the drive does not have a limit on the transfer size.

In the current 4.4 kernel driver, we detect this and set dev->max_hw_sectors to UINT_MAX.

Later, when a namespace is allocated, we set the block level max_hw_sectors and max_segments based on dev->max_hw_sectors. This operations perform shifts on the value, but we are already at UINT_MAX, so end up with strange results.

Quoted 4 lines

I'm not entirely sure what is supposed to happen. The following patch sets max_hw_sectors to

BLK_DEF_MAX_SECTORS and max_segments to BLK_MAX_SEGMENTS, but this still does not seem right, as drives with a large MDTS will set max_segments higher than this.

Nice find. The original code was written before the segment count, so probably was not tested together (my controllers define MDTS, so I've never tested it at all).

We can fix this by reordering the math instead of artificially reducing the transfer size.
---

drivers/nvme/host/pci.c +1 -1
index 5aca81c..f17e3d3 100644
@@ -2266,7 +2266,7 @@ static void nvme_alloc_ns(struct nvme_dev *dev, unsigned nsid)
 	if (dev->max_hw_sectors) {
 		blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors);
 		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev->page_size) + 1);
+			((dev->max_hw_sectors / (dev->page_size >> 9) + 1);
 	}
 	if (dev->stripe_size)
 		blk_queue_chunk_sectors(ns->queue, dev->stripe_size >> 9);

--

#2 lore raw
Quoted 2 lines · Keith Busch

We can fix this by reordering the math instead of artificially reducing the transfer size.

Resend with an actually compilable patch.

---

drivers/nvme/host/pci.c +1 -1
index 5aca81c..f17e3d3 100644
@@ -2266,7 +2266,7 @@ static void nvme_alloc_ns(struct nvme_dev *dev, unsigned nsid)
 	if (dev->max_hw_sectors) {
 		blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors);
 		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev->page_size) + 1);
+			(dev->max_hw_sectors / (dev->page_size >> 9) + 1);
 	}
 	if (dev->stripe_size)
 		blk_queue_chunk_sectors(ns->queue, dev->stripe_size >> 9);

--

#3 lore raw

Ugh, broken again, sorry, having a distracted day...

I'll resend as a proper patch that really works.

Quoted 21 lines · Keith Busch
Quoted 2 lines

We can fix this by reordering the math instead of artificially reducing the transfer size.

Resend with an actually compilable patch.

---

drivers/nvme/host/pci.c +1 -2
index 5aca81c..f17e3d3 100644
@@ -2266,7 +2266,7 @@ static void nvme_alloc_ns(struct nvme_dev *dev, unsigned nsid)
 	if (dev->max_hw_sectors) {
 		blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors);
 		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev->page_size) + 1);
+			(dev->max_hw_sectors / (dev->page_size >> 9) + 1);
 	}
 	if (dev->stripe_size)
 		blk_queue_chunk_sectors(ns->queue, dev->stripe_size >> 9);
--
author
#4 lore raw

Thanks.

That should take care of max_segments.

There is still the issue that blk_queue_max_hw_sectors calls blk_limits_max_hw_sectors which performs:

(max_hw_sectors << 9)

and so ends up with a strange looking value. I'm not if this is a problem with the driver calling it with UINT_MAX, or a problem with the way blk_limits_max_hw_sectors processes max_hw_sectors. This is a minor issue since the device will still operate.

Quoted 25 lines · Keith

Ugh, broken again, sorry, having a distracted day...

I'll resend as a proper patch that really works.

Quoted 20 lines · Keith Busch
Quoted 2 lines

We can fix this by reordering the math instead of artificially reducing the transfer size.

Resend with an actually compilable patch.

---

drivers/nvme/host/pci.c +1 -2
index 5aca81c..f17e3d3 100644
@@ -2266,7 +2266,7 @@ static void nvme_alloc_ns(struct nvme_dev *dev, unsigned nsid)
 	if (dev->max_hw_sectors) {
 		blk_queue_max_hw_sectors(ns->queue, dev->max_hw_sectors);
 		blk_queue_max_segments(ns->queue,
-			((dev->max_hw_sectors << 9) / dev->page_size) + 1);
+			(dev->max_hw_sectors / (dev->page_size >> 9) + 1);
 	}
 	if (dev->stripe_size)
 		blk_queue_chunk_sectors(ns->queue, dev->stripe_size >> 9);
--