Skip to contentSkip to conversation

nvme/pci: Print invalid SGL only once

PATCH New Keith Busch 3 participants 6 messages Sep 14, 2017 – Sep 15, 2017 NVMe

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

The WARN_ONCE macro returns true if the condition is true, not if the warn was raised, so we're printing the scatter list every time it's invalid. This is excessive and makes debugging harder, so this patch prints it just once.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/nvme/host/pci.c +4 -1
index df96562..33d66da 100644
@@ -554,6 +554,8 @@ static blk_status_t nvme_setup_prps(struct nvme_dev *dev, struct request *req)
 	dma_addr_t prp_dma;
 	int nprps, i;
 
+	static bool sgl_warned = false;
+
 	length -= (page_size - offset);
 	if (length <= 0)
 		return BLK_STS_OK;

@@ -619,8 +621,9 @@ static blk_status_t nvme_setup_prps(struct nvme_dev *dev, struct request *req)
 	return BLK_STS_OK;
 
  bad_sgl:
-	if (WARN_ONCE(1, "Invalid SGL for payload:%d nents:%d\n",
+	if (!sgl_warned && WARN_ONCE(1, "Invalid SGL for payload:%d nents:%d\n",
 				blk_rq_payload_bytes(req), iod->nents)) {
+		sgl_warned = true;
 		for_each_sg(iod->sg, sg, iod->nents, i) {
 			dma_addr_t phys = sg_phys(sg);
 			pr_warn("sg[%d] phys_addr:%pad offset:%d length:%d "
Signature · 1 line
2.5.5
#1 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
maintainer
#2 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

In reply to Keith Busch
Quoted 4 lines · Keith Busch

The WARN_ONCE macro returns true if the condition is true, not if the warn was raised, so we're printing the scatter list every time it's invalid. This is excessive and makes debugging harder, so this patch prints it just once.

Please use the printk_ratelimited() helper instead.

author
#3 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

Quoted 7 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch

The WARN_ONCE macro returns true if the condition is true, not if the warn was raised, so we're printing the scatter list every time it's invalid. This is excessive and makes debugging harder, so this patch prints it just once.

Please use the printk_ratelimited() helper instead.

Hmm, I don't want to rate limit the prints. That may log only partial SGLs, or even worse, interleave parts from multiple errors. We want to see an entire SGL for a single error, like what WARN_ONCE does. A single occurrence is usually sufficient to work backwards to see how the breakage occurred.

author
#4 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

I should have known Linux had a macro to do something "once". Is the following more acceptable?

---

drivers/nvme/host/pci.c +18 -12
index df96562..8a7bbe2 100644
@@ -24,6 +24,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/once.h>
 #include <linux/pci.h>
 #include <linux/poison.h>
 #include <linux/t10-pi.h>

@@ -539,6 +540,20 @@ static void nvme_dif_complete(u32 p, u32 v, struct t10_pi_tuple *pi)
 }
 #endif
 
+static void nvme_print_sgl(struct scatterlist *sgl, int nents)
+{
+	int i;
+	struct scatterlist *sg;
+
+	for_each_sg(sgl, sg, nents, i) {
+		dma_addr_t phys = sg_phys(sg);
+		pr_warn("sg[%d] phys_addr:%pad offset:%d length:%d "
+			"dma_address:%pad dma_length:%d\n",
+			i, &phys, sg->offset, sg->length, &sg_dma_address(sg),
+			sg_dma_len(sg));
+	}
+}
+
 static blk_status_t nvme_setup_prps(struct nvme_dev *dev, struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

@@ -619,19 +634,10 @@ static blk_status_t nvme_setup_prps(struct nvme_dev *dev, struct request *req)
 	return BLK_STS_OK;
 
  bad_sgl:
-	if (WARN_ONCE(1, "Invalid SGL for payload:%d nents:%d\n",
-				blk_rq_payload_bytes(req), iod->nents)) {
-		for_each_sg(iod->sg, sg, iod->nents, i) {
-			dma_addr_t phys = sg_phys(sg);
-			pr_warn("sg[%d] phys_addr:%pad offset:%d length:%d "
-			       "dma_address:%pad dma_length:%d\n", i, &phys,
-					sg->offset, sg->length,
-					&sg_dma_address(sg),
-					sg_dma_len(sg));
-		}
-	}
+	WARN(DO_ONCE(nvme_print_sgl, iod->sg, iod->nents),
+		"Invalid SGL for payload:%d nents:%d\n",
+			blk_rq_payload_bytes(req), iod->nents);
 	return BLK_STS_IOERR;
-
 }
 
 static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,

--

maintainer
#5 lore raw

PATCH 1/1 nvme/pci: Print invalid SGL only once

Quoted 2 lines · Keith Busch

I should have known Linux had a macro to do something "once". Is the following more acceptable?

This looks much nicer.