All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/11] scsi_debug: Some minor improvements
@ 2023-03-13  8:44 John Garry
  2023-03-13  8:44 ` [PATCH v2 01/11] scsi: scsi_debug: Don't hold driver host struct pointer in host->hostdata[] John Garry
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:44 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

This series contains a bunch of minor improvements to the driver. I have
another bunch waiting with more major changes.

Most of the changes are quite straightforward, and the only patches of note are
as follows:
- Fix the command abort feature, enabled with host option SDEBUG_OPT_CMD_ABORT
- Drop driver count of queued commands per device
- Add poll mode completions to statistics. We already have poll mode callback
  call count, so maybe it was intentional to omit poll mode from the
  statistics.
  
Difference to v1:
- rename function to get sdebug host from shost (Bart)

Based on scsi-staging 6.4 @ commit 0b31b77f281a ("Merge patch series "PCI/AER: ...")

John Garry (11):
  scsi: scsi_debug: Don't hold driver host struct pointer in
    host->hostdata[]
  scsi: scsi_debug: Stop setting devip->sdbg_host twice
  scsi: scsi_debug: Drop scsi_debug_abort() NULL pointer checks
  scsi: scsi_debug: Drop scsi_debug_device_reset() NULL pointer checks
  scsi: scsi_debug: Drop scsi_debug_target_reset() NULL pointer checks
  scsi: scsi_debug: Drop scsi_debug_bus_reset() NULL pointer checks
  scsi: scsi_debug: Drop scsi_debug_host_reset() device NULL pointer
    check
  scsi: scsi_debug: Drop check for num_in_q exceeding queue depth
  scsi: scsi_debug: Drop sdebug_dev_info.num_in_q
  scsi: scsi_debug: Get command abort feature working again
  scsi: scsi_debug: Add poll mode deferred completions to statistics

 drivers/scsi/scsi_debug.c | 209 ++++++++++++++------------------------
 1 file changed, 76 insertions(+), 133 deletions(-)

-- 
2.35.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 01/11] scsi: scsi_debug: Don't hold driver host struct pointer in host->hostdata[]
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
@ 2023-03-13  8:44 ` John Garry
  2023-03-13  8:44 ` [PATCH v2 02/11] scsi: scsi_debug: Stop setting devip->sdbg_host twice John Garry
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:44 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

This driver stores just a pointer to the driver host structure in
host->hostdata[]. Most other drivers actually have the driver host
structure allocated in host->hostdata[], but this driver is different as
we allocate that memory separately before allocating the shost memory.

However there is no need to allocate this memory only in host->hostdata[]
when we can already look up the driver host structure from shost->dma_dev,
so add a macro for this - shost_to_sdebug_host(). Rename to_sdebug_host()
-> dev_to_sdebug_host() to avoid ambiguity.

Also remove a check for !sdbg_host in find_build_dev_info(), as this cannot
be true. Other similar checks will be later removed.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 72149eeee6e6..554c03d7a648 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -324,9 +324,12 @@ struct sdeb_store_info {
 	void *map_storep;	/* provisioning map */
 };
 
-#define to_sdebug_host(d)	\
+#define dev_to_sdebug_host(d)	\
 	container_of(d, struct sdebug_host_info, dev)
 
+#define shost_to_sdebug_host(shost)	\
+	dev_to_sdebug_host(shost->dma_dev)
+
 enum sdeb_defer_type {SDEB_DEFER_NONE = 0, SDEB_DEFER_HRT = 1,
 		      SDEB_DEFER_WQ = 2, SDEB_DEFER_POLL = 3};
 
@@ -5166,11 +5169,7 @@ static struct sdebug_dev_info *find_build_dev_info(struct scsi_device *sdev)
 	struct sdebug_dev_info *open_devip = NULL;
 	struct sdebug_dev_info *devip;
 
-	sdbg_host = *(struct sdebug_host_info **)shost_priv(sdev->host);
-	if (!sdbg_host) {
-		pr_err("Host info NULL\n");
-		return NULL;
-	}
+	sdbg_host = shost_to_sdebug_host(sdev->host);
 
 	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
 		if ((devip->used) && (devip->channel == sdev->channel) &&
@@ -5407,7 +5406,7 @@ static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 	hp = sdp->host;
 	if (!hp)
 		goto lie;
-	sdbg_host = *(struct sdebug_host_info **)shost_priv(hp);
+	sdbg_host = shost_to_sdebug_host(hp);
 	if (sdbg_host) {
 		list_for_each_entry(devip,
 				    &sdbg_host->dev_info_list,
@@ -5440,7 +5439,7 @@ static int scsi_debug_bus_reset(struct scsi_cmnd *SCpnt)
 		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
 	hp = sdp->host;
 	if (hp) {
-		sdbg_host = *(struct sdebug_host_info **)shost_priv(hp);
+		sdbg_host = shost_to_sdebug_host(hp);
 		if (sdbg_host) {
 			list_for_each_entry(devip,
 					    &sdbg_host->dev_info_list,
@@ -7165,7 +7164,7 @@ static void sdebug_release_adapter(struct device *dev)
 {
 	struct sdebug_host_info *sdbg_host;
 
-	sdbg_host = to_sdebug_host(dev);
+	sdbg_host = dev_to_sdebug_host(dev);
 	kfree(sdbg_host);
 }
 
@@ -7812,14 +7811,14 @@ static int sdebug_driver_probe(struct device *dev)
 	struct Scsi_Host *hpnt;
 	int hprot;
 
-	sdbg_host = to_sdebug_host(dev);
+	sdbg_host = dev_to_sdebug_host(dev);
 
 	sdebug_driver_template.can_queue = sdebug_max_queue;
 	sdebug_driver_template.cmd_per_lun = sdebug_max_queue;
 	if (!sdebug_clustering)
 		sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
 
-	hpnt = scsi_host_alloc(&sdebug_driver_template, sizeof(sdbg_host));
+	hpnt = scsi_host_alloc(&sdebug_driver_template, 0);
 	if (NULL == hpnt) {
 		pr_err("scsi_host_alloc failed\n");
 		error = -ENODEV;
@@ -7862,7 +7861,6 @@ static int sdebug_driver_probe(struct device *dev)
 		hpnt->nr_maps = 3;
 
 	sdbg_host->shost = hpnt;
-	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
 	if ((hpnt->this_id >= 0) && (sdebug_num_tgts > hpnt->this_id))
 		hpnt->max_id = sdebug_num_tgts + 1;
 	else
@@ -7936,7 +7934,7 @@ static void sdebug_driver_remove(struct device *dev)
 	struct sdebug_host_info *sdbg_host;
 	struct sdebug_dev_info *sdbg_devinfo, *tmp;
 
-	sdbg_host = to_sdebug_host(dev);
+	sdbg_host = dev_to_sdebug_host(dev);
 
 	scsi_remove_host(sdbg_host->shost);
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 02/11] scsi: scsi_debug: Stop setting devip->sdbg_host twice
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
  2023-03-13  8:44 ` [PATCH v2 01/11] scsi: scsi_debug: Don't hold driver host struct pointer in host->hostdata[] John Garry
@ 2023-03-13  8:44 ` John Garry
  2023-03-13  8:44 ` [PATCH v2 03/11] scsi: scsi_debug: Drop scsi_debug_abort() NULL pointer checks John Garry
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:44 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

In sdebug_device_create(), the devip->sdbg_host pointer is needlessly set
twice, so stop doing that.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 554c03d7a648..4c60a055610a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5155,7 +5155,6 @@ static struct sdebug_dev_info *sdebug_device_create(
 		} else {
 			devip->zmodel = BLK_ZONED_NONE;
 		}
-		devip->sdbg_host = sdbg_host;
 		devip->create_ts = ktime_get_boottime();
 		atomic_set(&devip->stopped, (sdeb_tur_ms_to_ready > 0 ? 2 : 0));
 		list_add_tail(&devip->dev_list, &sdbg_host->dev_info_list);
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 03/11] scsi: scsi_debug: Drop scsi_debug_abort() NULL pointer checks
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
  2023-03-13  8:44 ` [PATCH v2 01/11] scsi: scsi_debug: Don't hold driver host struct pointer in host->hostdata[] John Garry
  2023-03-13  8:44 ` [PATCH v2 02/11] scsi: scsi_debug: Stop setting devip->sdbg_host twice John Garry
@ 2023-03-13  8:44 ` John Garry
  2023-03-13  8:44 ` [PATCH v2 04/11] scsi: scsi_debug: Drop scsi_debug_device_reset() " John Garry
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:44 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The SCSI cmnd pointer arg would never be NULL, so drop the check. In
addition, its SCSI device pointer would never be NULL.

The only caller is scsi_send_eh_cmnd() -> scsi_abort_eh_cmnd() ->
scsi_try_to_abort_cmd() -> scsi_try_to_abort_cmd(), and in the origin of
that chain those pointers cannot be NULL.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4c60a055610a..2c2a41b99641 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5360,13 +5360,13 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
 	bool ok;
 
 	++num_aborts;
-	if (SCpnt) {
-		ok = stop_queued_cmnd(SCpnt);
-		if (SCpnt->device && (SDEBUG_OPT_ALL_NOISE & sdebug_opts))
-			sdev_printk(KERN_INFO, SCpnt->device,
-				    "%s: command%s found\n", __func__,
-				    ok ? "" : " not");
-	}
+
+	ok = stop_queued_cmnd(SCpnt);
+	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
+		sdev_printk(KERN_INFO, SCpnt->device,
+			    "%s: command%s found\n", __func__,
+			    ok ? "" : " not");
+
 	return SUCCESS;
 }
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 04/11] scsi: scsi_debug: Drop scsi_debug_device_reset() NULL pointer checks
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (2 preceding siblings ...)
  2023-03-13  8:44 ` [PATCH v2 03/11] scsi: scsi_debug: Drop scsi_debug_abort() NULL pointer checks John Garry
@ 2023-03-13  8:44 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 05/11] scsi: scsi_debug: Drop scsi_debug_target_reset() " John Garry
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:44 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The SCSI cmnd pointer arg would never be NULL, so drop the check. In
addition, its SCSI device pointer would never be NULL (so drop that check
also).

The only caller is scsi_try_bus_device_reset(), and the command and its
device pointer could not be NULL when calling eh_device_reset_handler()
there.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 2c2a41b99641..5b51c24f7d09 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5372,17 +5372,16 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
 
 static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
 {
+	struct scsi_device *sdp = SCpnt->device;
+	struct sdebug_dev_info *devip = sdp->hostdata;
+
 	++num_dev_resets;
-	if (SCpnt && SCpnt->device) {
-		struct scsi_device *sdp = SCpnt->device;
-		struct sdebug_dev_info *devip =
-				(struct sdebug_dev_info *)sdp->hostdata;
 
-		if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
-			sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
-		if (devip)
-			set_bit(SDEBUG_UA_POR, devip->uas_bm);
-	}
+	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
+		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
+	if (devip)
+		set_bit(SDEBUG_UA_POR, devip->uas_bm);
+
 	return SUCCESS;
 }
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 05/11] scsi: scsi_debug: Drop scsi_debug_target_reset() NULL pointer checks
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (3 preceding siblings ...)
  2023-03-13  8:44 ` [PATCH v2 04/11] scsi: scsi_debug: Drop scsi_debug_device_reset() " John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 06/11] scsi: scsi_debug: Drop scsi_debug_bus_reset() " John Garry
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The checks for SCSI cmnd, SCSI device, and SCSI host are unnecessary, so
drop them. Likewise, drop the NULL check for sdbg_host.

The only caller is scsi_try_target_reset() -> eh_target_reset_handler(),
and there those pointers cannot be NULL.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 5b51c24f7d09..6364d6f08861 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5387,37 +5387,26 @@ static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
 
 static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 {
-	struct sdebug_host_info *sdbg_host;
+	struct scsi_device *sdp = SCpnt->device;
+	struct sdebug_host_info *sdbg_host = shost_to_sdebug_host(sdp->host);
 	struct sdebug_dev_info *devip;
-	struct scsi_device *sdp;
-	struct Scsi_Host *hp;
 	int k = 0;
 
 	++num_target_resets;
-	if (!SCpnt)
-		goto lie;
-	sdp = SCpnt->device;
-	if (!sdp)
-		goto lie;
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
-	hp = sdp->host;
-	if (!hp)
-		goto lie;
-	sdbg_host = shost_to_sdebug_host(hp);
-	if (sdbg_host) {
-		list_for_each_entry(devip,
-				    &sdbg_host->dev_info_list,
-				    dev_list)
-			if (devip->target == sdp->id) {
-				set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
-				++k;
-			}
+
+	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
+		if (devip->target == sdp->id) {
+			set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
+			++k;
+		}
 	}
+
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp,
 			    "%s: %d device(s) found in target\n", __func__, k);
-lie:
+
 	return SUCCESS;
 }
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 06/11] scsi: scsi_debug: Drop scsi_debug_bus_reset() NULL pointer checks
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (4 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 05/11] scsi: scsi_debug: Drop scsi_debug_target_reset() " John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 07/11] scsi: scsi_debug: Drop scsi_debug_host_reset() device NULL pointer check John Garry
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The checks for SCSI cmnd, SCSI device, and SCSI host are unnecessary, so
drop them. Likewise, drop the NULL check for sdbg_host.

The only caller is scsi_try_bus_reset() -> eh_bus_reset_handler(),
and there those pointers cannot be NULL.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6364d6f08861..749358b48335 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5412,34 +5412,24 @@ static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 
 static int scsi_debug_bus_reset(struct scsi_cmnd *SCpnt)
 {
-	struct sdebug_host_info *sdbg_host;
+	struct scsi_device *sdp = SCpnt->device;
+	struct sdebug_host_info *sdbg_host = shost_to_sdebug_host(sdp->host);
 	struct sdebug_dev_info *devip;
-	struct scsi_device *sdp;
-	struct Scsi_Host *hp;
 	int k = 0;
 
 	++num_bus_resets;
-	if (!(SCpnt && SCpnt->device))
-		goto lie;
-	sdp = SCpnt->device;
+
 	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp, "%s\n", __func__);
-	hp = sdp->host;
-	if (hp) {
-		sdbg_host = shost_to_sdebug_host(hp);
-		if (sdbg_host) {
-			list_for_each_entry(devip,
-					    &sdbg_host->dev_info_list,
-					    dev_list) {
-				set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
-				++k;
-			}
-		}
+
+	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
+		set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
+		++k;
 	}
+
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, sdp,
 			    "%s: %d device(s) found in host\n", __func__, k);
-lie:
 	return SUCCESS;
 }
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 07/11] scsi: scsi_debug: Drop scsi_debug_host_reset() device NULL pointer check
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (5 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 06/11] scsi: scsi_debug: Drop scsi_debug_bus_reset() " John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 08/11] scsi: scsi_debug: Drop check for num_in_q exceeding queue depth John Garry
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The check for device pointer for the SCSI command is unnecessary, so drop
it.

The only caller is scsi_try_host_reset() -> eh_host_reset_handler(),
and there that pointer cannot be NULL.

Indeed, there is already code later in the same function which does not
check the device pointer for the SCSI command.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 749358b48335..47820b9f6326 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5440,7 +5440,7 @@ static int scsi_debug_host_reset(struct scsi_cmnd *SCpnt)
 	int k = 0;
 
 	++num_host_resets;
-	if ((SCpnt->device) && (SDEBUG_OPT_ALL_NOISE & sdebug_opts))
+	if (SDEBUG_OPT_ALL_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, SCpnt->device, "%s\n", __func__);
 	spin_lock(&sdebug_host_list_lock);
 	list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 08/11] scsi: scsi_debug: Drop check for num_in_q exceeding queue depth
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (6 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 07/11] scsi: scsi_debug: Drop scsi_debug_host_reset() device NULL pointer check John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 10/11] scsi: scsi_debug: Get command abort feature working again John Garry
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The per-device num_in_q value cannot exceed the device queue depth, so drop
the check.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 47820b9f6326..0d515bac93bf 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5593,15 +5593,8 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 	}
 	num_in_q = atomic_read(&devip->num_in_q);
 	qdepth = cmnd->device->queue_depth;
-	if (unlikely((qdepth > 0) && (num_in_q >= qdepth))) {
-		if (scsi_result) {
-			spin_unlock_irqrestore(&sqp->qc_lock, iflags);
-			goto respond_in_thread;
-		} else
-			scsi_result = device_qfull_result;
-	} else if (unlikely(sdebug_every_nth &&
-			    (SDEBUG_OPT_RARE_TSF & sdebug_opts) &&
-			    (scsi_result == 0))) {
+	if (unlikely(sdebug_every_nth && (SDEBUG_OPT_RARE_TSF & sdebug_opts) &&
+		     (scsi_result == 0))) {
 		if ((num_in_q == (qdepth - 1)) &&
 		    (atomic_inc_return(&sdebug_a_tsf) >=
 		     abs(sdebug_every_nth))) {
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 10/11] scsi: scsi_debug: Get command abort feature working again
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (7 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 08/11] scsi: scsi_debug: Drop check for num_in_q exceeding queue depth John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  8:45 ` [PATCH v2 11/11] scsi: scsi_debug: Add poll mode deferred completions to statistics John Garry
  2023-03-13  9:25 ` [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

The command abort feature allows us to test aborting a command which has
timed-out.

The idea is that for specific commands we just don't call scsi_done() and
allow the request to timeout, which ensures SCSI EH kicks-in we try to
abort the command.

Since commit 4a0c6f432d15 ("scsi: scsi_debug: Add new defer type for
mq_poll") this does not seem to work. The issue is that we clear the
sd_dp->aborted flag in schedule_resp() before the completion callback has
run. When the completion callback actually runs, it calls scsi_done() as
normal as sd_dp->aborted unset. This is all very racy.

Fix by not clearing sd_dp->aborted in schedule_resp(). Also move the call
to blk_abort_request() from schedule_resp() to sdebug_q_cmd_complete(),
which makes the code have a more logical sequence.

I also note that this feature only works for commands which are classed
as "SDEG_RES_IMMED_MASK", but only practically triggered with prior RW
commands. So for my experiment I need to run fio to trigger the error on
the "nth" command (see inject_on_this_cmd()), and then run something like
sg_sync to queue a command to actually trigger the abort.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 449b460e4c1b..1463e54179bf 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4983,7 +4983,8 @@ static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
 	spin_unlock_irqrestore(&sqp->qc_lock, iflags);
 	if (unlikely(aborted)) {
 		if (sdebug_verbose)
-			pr_info("bypassing scsi_done() due to aborted cmd\n");
+			pr_info("bypassing scsi_done() due to aborted cmd, kicking-off EH\n");
+		blk_abort_request(scsi_cmd_to_rq(scp));
 		return;
 	}
 	scsi_done(scp); /* callback to mid level */
@@ -5712,8 +5713,13 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 			sd_dp->issuing_cpu = raw_smp_processor_id();
 	} else {	/* jdelay < 0, use work queue */
 		if (unlikely((sdebug_opts & SDEBUG_OPT_CMD_ABORT) &&
-			     atomic_read(&sdeb_inject_pending)))
+			     atomic_read(&sdeb_inject_pending))) {
 			sd_dp->aborted = true;
+			atomic_set(&sdeb_inject_pending, 0);
+			sdev_printk(KERN_INFO, sdp, "abort request tag=%#x\n",
+				    blk_mq_unique_tag_to_tag(get_tag(cmnd)));
+		}
+
 		if (polled) {
 			sd_dp->cmpl_ts = ns_to_ktime(ns_from_boot);
 			spin_lock_irqsave(&sqp->qc_lock, iflags);
@@ -5738,13 +5744,6 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 		}
 		if (sdebug_statistics)
 			sd_dp->issuing_cpu = raw_smp_processor_id();
-		if (unlikely(sd_dp->aborted)) {
-			sdev_printk(KERN_INFO, sdp, "abort request tag %d\n",
-				    scsi_cmd_to_rq(cmnd)->tag);
-			blk_abort_request(scsi_cmd_to_rq(cmnd));
-			atomic_set(&sdeb_inject_pending, 0);
-			sd_dp->aborted = false;
-		}
 	}
 
 	return 0;
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 11/11] scsi: scsi_debug: Add poll mode deferred completions to statistics
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (8 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 10/11] scsi: scsi_debug: Get command abort feature working again John Garry
@ 2023-03-13  8:45 ` John Garry
  2023-03-13  9:25 ` [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  8:45 UTC (permalink / raw)
  To: jejb, martin.petersen
  Cc: linux-scsi, bvanassche, linux-kernel, dgilbert, John Garry

Currently commands completed via poll mode are not included in the
statistics gathering for deferred completions and missed CPUs.

Poll mode completions should be treated the same as other deferred
completion types, so add poll mode completions to the statistics.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/scsi_debug.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1463e54179bf..073fc02f9fed 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7531,6 +7531,13 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num)
 		}
 		WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE);
 		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
+
+		if (sdebug_statistics) {
+			atomic_inc(&sdebug_completions);
+			if (raw_smp_processor_id() != sd_dp->issuing_cpu)
+				atomic_inc(&sdebug_miss_cpus);
+		}
+
 		scsi_done(scp); /* callback to mid level */
 		num_entries++;
 		spin_lock_irqsave(&sqp->qc_lock, iflags);
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 00/11] scsi_debug: Some minor improvements
  2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
                   ` (9 preceding siblings ...)
  2023-03-13  8:45 ` [PATCH v2 11/11] scsi: scsi_debug: Add poll mode deferred completions to statistics John Garry
@ 2023-03-13  9:25 ` John Garry
  10 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2023-03-13  9:25 UTC (permalink / raw)
  To: jejb, martin.petersen; +Cc: linux-scsi, bvanassche, linux-kernel, dgilbert

I'm going to resend this as I seem to be missing patch #9 of 11.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-03-13  9:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-13  8:44 [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry
2023-03-13  8:44 ` [PATCH v2 01/11] scsi: scsi_debug: Don't hold driver host struct pointer in host->hostdata[] John Garry
2023-03-13  8:44 ` [PATCH v2 02/11] scsi: scsi_debug: Stop setting devip->sdbg_host twice John Garry
2023-03-13  8:44 ` [PATCH v2 03/11] scsi: scsi_debug: Drop scsi_debug_abort() NULL pointer checks John Garry
2023-03-13  8:44 ` [PATCH v2 04/11] scsi: scsi_debug: Drop scsi_debug_device_reset() " John Garry
2023-03-13  8:45 ` [PATCH v2 05/11] scsi: scsi_debug: Drop scsi_debug_target_reset() " John Garry
2023-03-13  8:45 ` [PATCH v2 06/11] scsi: scsi_debug: Drop scsi_debug_bus_reset() " John Garry
2023-03-13  8:45 ` [PATCH v2 07/11] scsi: scsi_debug: Drop scsi_debug_host_reset() device NULL pointer check John Garry
2023-03-13  8:45 ` [PATCH v2 08/11] scsi: scsi_debug: Drop check for num_in_q exceeding queue depth John Garry
2023-03-13  8:45 ` [PATCH v2 10/11] scsi: scsi_debug: Get command abort feature working again John Garry
2023-03-13  8:45 ` [PATCH v2 11/11] scsi: scsi_debug: Add poll mode deferred completions to statistics John Garry
2023-03-13  9:25 ` [PATCH v2 00/11] scsi_debug: Some minor improvements John Garry

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.