Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [PATCH] scsi: scsi_debug: default to a higher throughput config
@ 2026-09-02  9:28 John Garry
  2026-09-02  9:57 ` sashiko-bot
  2026-09-02 11:28 ` Christoph Hellwig
  0 siblings, 2 replies; 3+ messages in thread
From: John Garry @ 2026-09-02  9:28 UTC (permalink / raw)
  To: James.Bottomley, mkp
  Cc: linux-scsi, djwong, hch, fstests, bvanassche, John Garry

Currently the default config goes not give as high a throughput as some
would like.

Give a higher default throughput by modifying the following:
- set completion response delay as 0
- increase shost can_queue to 4096, which aligns better with modern
  high-speed HBAs
- turn on clustering

Signed-off-by: John Garry <john.garry@linux.dev>

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3346562275f8..f2afe757c438 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -79,7 +79,7 @@ static const char *sdebug_version_date = "20210520";
  */
 #define DEF_ATO 1
 #define DEF_CDB_LEN 10
-#define DEF_JDELAY   1		/* if > 0 unit is a jiffy */
+#define DEF_JDELAY   0		/* if > 0 unit is a jiffy */
 #define DEF_DEV_SIZE_PRE_INIT   0
 #define DEF_DEV_SIZE_MB   8
 #define DEF_ZBC_DEV_SIZE_MB   128
@@ -106,6 +106,7 @@ static const char *sdebug_version_date = "20210520";
 #define DEF_PTYPE   TYPE_DISK
 #define DEF_RANDOM false
 #define DEF_REMOVABLE false
+#define DEF_CLUSTERING true
 #define DEF_SCSI_LEVEL   7    /* INQUIRY, byte2 [6->SPC-4; 7->SPC-5] */
 #define DEF_SECTOR_SIZE 512
 #define DEF_UNMAP_ALIGNMENT 0
@@ -217,8 +218,7 @@ struct tape_block {
  * /sys/class/scsi_device/<h:c:t:l>/device/queue_depth
  * but cannot exceed SDEBUG_CANQUEUE .
  */
-#define SDEBUG_CANQUEUE_WORDS  3	/* a WORD is bits in a long */
-#define SDEBUG_CANQUEUE  (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
+#define SDEBUG_CANQUEUE  (4096)
 #define DEF_CMD_PER_LUN  SDEBUG_CANQUEUE
 
 /* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
@@ -850,7 +850,7 @@ static int sdebug_num_hosts;
 static int sdebug_add_host = DEF_NUM_HOST;  /* in sysfs this is relative */
 static int sdebug_ato = DEF_ATO;
 static int sdebug_cdb_len = DEF_CDB_LEN;
-static int sdebug_jdelay = DEF_JDELAY;	/* if > 0 then unit is jiffies */
+static int sdebug_jdelay = DEF_JDELAY;
 static int sdebug_dev_size_mb = DEF_DEV_SIZE_PRE_INIT;
 static int sdebug_dif = DEF_DIF;
 static int sdebug_dix = DEF_DIX;
@@ -899,7 +899,7 @@ static int sdebug_uuid_ctl = DEF_UUID_CTL;
 static bool sdebug_random = DEF_RANDOM;
 static bool sdebug_per_host_store = DEF_PER_HOST_STORE;
 static bool sdebug_removable = DEF_REMOVABLE;
-static bool sdebug_clustering;
+static bool sdebug_clustering = DEF_CLUSTERING;
 static bool sdebug_host_lock = DEF_HOST_LOCK;
 static bool sdebug_strict = DEF_STRICT;
 static bool sdebug_no_rwlock;
@@ -7373,8 +7373,8 @@ MODULE_VERSION(SDEBUG_VERSION);
 MODULE_PARM_DESC(add_host, "add n hosts, in sysfs if negative remove host(s) (def=1)");
 MODULE_PARM_DESC(ato, "application tag ownership: 0=disk 1=host (def=1)");
 MODULE_PARM_DESC(cdb_len, "suggest CDB lengths to drivers (def=10)");
-MODULE_PARM_DESC(clustering, "when set enables larger transfers (def=0)");
-MODULE_PARM_DESC(delay, "response delay (def=1 jiffy); 0:imm, -1,-2:tiny");
+MODULE_PARM_DESC(clustering, "when set enables larger transfers (def=1)");
+MODULE_PARM_DESC(delay, "response delay (def=0 jiffy); 0:imm, -1,-2:tiny");
 MODULE_PARM_DESC(dev_size_mb, "size in MiB of ram shared by devs(def=8)");
 MODULE_PARM_DESC(dif, "data integrity field type: 0-3 (def=0)");
 MODULE_PARM_DESC(dix, "data integrity extensions mask (def=0)");
-- 
2.43.0


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

* Re: [PATCH] scsi: scsi_debug: default to a higher throughput config
  2026-09-02  9:28 [PATCH] scsi: scsi_debug: default to a higher throughput config John Garry
@ 2026-09-02  9:57 ` sashiko-bot
  2026-09-02 11:28 ` Christoph Hellwig
  1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-02  9:57 UTC (permalink / raw)
  To: John Garry; +Cc: linux-scsi

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stale defer_t in sdebug_scsi_cmd causes premature double scsi_done() leading to UAF.
--

commit 92c33bac6eb173f6156f5a3b2daf5604d8994b5b
Author: John Garry <john.garry@linux.dev>

scsi: scsi_debug: default to a higher throughput config

Currently the default config goes not give as high a throughput as some
would like. Give a higher default throughput by modifying the delay,
can_queue, and clustering defaults.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 3346562275f80..f2afe757c4380 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -79,7 +79,7 @@ static const char *sdebug_version_date = "20210520";
>   */
>  #define DEF_ATO 1
>  #define DEF_CDB_LEN 10
> -#define DEF_JDELAY   1		/* if > 0 unit is a jiffy */
> +#define DEF_JDELAY   0		/* if > 0 unit is a jiffy */

[Severity: Critical]
This is a pre-existing issue, but changing the default delay to 0 exposes it
out-of-the-box to all users.

When delay=0, schedule_resp() takes the early completion path without updating
sd_dp->defer_t or sd_dp->cmpl_ts:

drivers/scsi/scsi_debug.c:schedule_resp() {
    ...
respond_in_thread:      /* call back to mid-layer using invocation thread */
        cmnd->result = pfp != NULL ? pfp(cmnd, devip) : 0;
        cmnd->result &= ~SDEG_RES_IMMED_MASK;
        if (cmnd->result == 0 && scsi_result != 0)
                cmnd->result = scsi_result;
        scsi_done(cmnd);
        return 0;
}

Because scsi_cmd_priv(cmd) is not zeroed per-request by the SCSI midlayer,
a reused tag retains its previous defer_t value.

If the previous command on this tag was polled, defer_t is left as
SDEB_DEFER_POLL. Concurrently, if a userspace thread invokes
sdebug_blk_mq_poll_iter(), it iterates over busy tags, sees the stale
SDEB_DEFER_POLL and expired cmpl_ts, and spuriously calls scsi_done(cmd) a
second time:

drivers/scsi/scsi_debug.c:sdebug_blk_mq_poll_iter() {
    ...
        spin_lock_irqsave(&sdsc->lock, flags);
        sd_dp = &sdsc->sd_dp;
        if (sd_dp->defer_t != SDEB_DEFER_POLL) {
                spin_unlock_irqrestore(&sdsc->lock, flags);
                return true;
        }

        if (time < sd_dp->cmpl_ts) {
                spin_unlock_irqrestore(&sdsc->lock, flags);
                return true;
        }
        spin_unlock_irqrestore(&sdsc->lock, flags);
    ...
        scsi_done(cmd); /* callback to mid level */
}

Could this lead to a double scsi_done() and a Use-After-Free in the block
layer when racing with the early completion path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902092844.1741533-1-john.garry@linux.dev?part=1

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

* Re: [PATCH] scsi: scsi_debug: default to a higher throughput config
  2026-09-02  9:28 [PATCH] scsi: scsi_debug: default to a higher throughput config John Garry
  2026-09-02  9:57 ` sashiko-bot
@ 2026-09-02 11:28 ` Christoph Hellwig
  1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2026-09-02 11:28 UTC (permalink / raw)
  To: John Garry
  Cc: James.Bottomley, mkp, linux-scsi, djwong, hch, fstests,
	bvanassche

On Wed, Sep 02, 2026 at 10:28:44AM +0100, John Garry wrote:
> Currently the default config goes not give as high a throughput as some
> would like.
> 
> Give a higher default throughput by modifying the following:
> - set completion response delay as 0
> - increase shost can_queue to 4096, which aligns better with modern
>   high-speed HBAs
> - turn on clustering

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>


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

end of thread, other threads:[~2026-09-02 11:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-02  9:28 [PATCH] scsi: scsi_debug: default to a higher throughput config John Garry
2026-09-02  9:57 ` sashiko-bot
2026-09-02 11:28 ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox