* [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: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
* [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
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.