All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nvme/pci: Print invalid SGL only once
@ 2017-09-14 19:06 Keith Busch
  2017-09-15  7:36 ` Johannes Thumshirn
  2017-09-15 14:55 ` Christoph Hellwig
  0 siblings, 2 replies; 6+ messages in thread
From: Keith Busch @ 2017-09-14 19:06 UTC (permalink / raw)


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(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index df96562..33d66da 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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 "
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] nvme/pci: Print invalid SGL only once
  2017-09-14 19:06 [PATCH] nvme/pci: Print invalid SGL only once Keith Busch
@ 2017-09-15  7:36 ` Johannes Thumshirn
  2017-09-15 14:55 ` Christoph Hellwig
  1 sibling, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2017-09-15  7:36 UTC (permalink / raw)


Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] nvme/pci: Print invalid SGL only once
  2017-09-14 19:06 [PATCH] nvme/pci: Print invalid SGL only once Keith Busch
  2017-09-15  7:36 ` Johannes Thumshirn
@ 2017-09-15 14:55 ` Christoph Hellwig
  2017-09-15 15:17   ` Keith Busch
  1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-09-15 14:55 UTC (permalink / raw)


On Thu, Sep 14, 2017@03:06:14PM -0400, Keith Busch wrote:
> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] nvme/pci: Print invalid SGL only once
  2017-09-15 14:55 ` Christoph Hellwig
@ 2017-09-15 15:17   ` Keith Busch
  2017-09-15 15:44     ` Keith Busch
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2017-09-15 15:17 UTC (permalink / raw)


On Fri, Sep 15, 2017@04:55:53PM +0200, Christoph Hellwig wrote:
> On Thu, Sep 14, 2017@03:06:14PM -0400, Keith Busch wrote:
> > 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] nvme/pci: Print invalid SGL only once
  2017-09-15 15:44     ` Keith Busch
@ 2017-09-15 15:42       ` Christoph Hellwig
  0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2017-09-15 15:42 UTC (permalink / raw)


On Fri, Sep 15, 2017@11:44:01AM -0400, Keith Busch wrote:
> I should have known Linux had a macro to do something "once". Is the
> following more acceptable?

This looks much nicer.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] nvme/pci: Print invalid SGL only once
  2017-09-15 15:17   ` Keith Busch
@ 2017-09-15 15:44     ` Keith Busch
  2017-09-15 15:42       ` Christoph Hellwig
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2017-09-15 15:44 UTC (permalink / raw)


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

---
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index df96562..8a7bbe2 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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,
--

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-15 15:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-14 19:06 [PATCH] nvme/pci: Print invalid SGL only once Keith Busch
2017-09-15  7:36 ` Johannes Thumshirn
2017-09-15 14:55 ` Christoph Hellwig
2017-09-15 15:17   ` Keith Busch
2017-09-15 15:44     ` Keith Busch
2017-09-15 15:42       ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.