diff mbox

scsi_debug: IMMED related delay adjustments

Message ID 20180410170036.10188-1-dgilbert@interlog.com (mailing list archive)
State Accepted
Headers show

Commit Message

Douglas Gilbert April 10, 2018, 5 p.m. UTC 
A patch titled: "[PATCH v2] scsi_debug: implement IMMED bit"
introduced long delays to the Start stop unit (SSU) and
Synchronize cache (SC) commands when the IMMED bit is clear.
This patch makes those delays more realistic. It causes SSU
to only delay when the start stop state is changed; SC only
delays when there's been a write since the previous SC. It
also reduced the SC delay from 1 second to 50 milliseconds.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
This patch is in response to a report on the Linux scsi list
indicating a significant slowdown in benchmarks using the
original patch. The reporter seemed satisfield with an earlier
version of this patch (in which the only difference was that
the SC delay was 100 milliseconds).

 drivers/scsi/scsi_debug.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Ming Lei April 11, 2018, 3:06 a.m. UTC | #1 
On Tue, Apr 10, 2018 at 01:00:36PM -0400, Douglas Gilbert wrote:
> A patch titled: "[PATCH v2] scsi_debug: implement IMMED bit"
> introduced long delays to the Start stop unit (SSU) and
> Synchronize cache (SC) commands when the IMMED bit is clear.
> This patch makes those delays more realistic. It causes SSU
> to only delay when the start stop state is changed; SC only
> delays when there's been a write since the previous SC. It
> also reduced the SC delay from 1 second to 50 milliseconds.
> 
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
> ---
> This patch is in response to a report on the Linux scsi list
> indicating a significant slowdown in benchmarks using the
> original patch. The reporter seemed satisfield with an earlier
> version of this patch (in which the only difference was that
> the SC delay was 100 milliseconds).
> 
>  drivers/scsi/scsi_debug.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 26ce022dd6f4..9ab19285a3d3 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -234,11 +234,13 @@ static const char *sdebug_version_date = "20180128";
>  #define F_INV_OP		0x200
>  #define F_FAKE_RW		0x400
>  #define F_M_ACCESS		0x800	/* media access */
> -#define F_LONG_DELAY		0x1000
> +#define F_SSU_DELAY		0x1000
> +#define F_SYNC_DELAY		0x2000
>  
>  #define FF_RESPOND (F_RL_WLUN_OK | F_SKIP_UA | F_DELAY_OVERR)
>  #define FF_MEDIA_IO (F_M_ACCESS | F_FAKE_RW)
>  #define FF_SA (F_SA_HIGH | F_SA_LOW)
> +#define F_LONG_DELAY		(F_SSU_DELAY | F_SYNC_DELAY)
>  
>  #define SDEBUG_MAX_PARTS 4
>  
> @@ -510,7 +512,7 @@ static const struct opcode_info_t release_iarr[] = {
>  };
>  
>  static const struct opcode_info_t sync_cache_iarr[] = {
> -	{0, 0x91, 0, F_LONG_DELAY | F_M_ACCESS, resp_sync_cache, NULL,
> +	{0, 0x91, 0, F_SYNC_DELAY | F_M_ACCESS, resp_sync_cache, NULL,
>  	    {16,  0x6, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>  	     0xff, 0xff, 0xff, 0xff, 0x3f, 0xc7} },	/* SYNC_CACHE (16) */
>  };
> @@ -553,7 +555,7 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	    resp_write_dt0, write_iarr,			/* WRITE(16) */
>  		{16,  0xfa, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>  		 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7} },
> -	{0, 0x1b, 0, F_LONG_DELAY, resp_start_stop, NULL,/* START STOP UNIT */
> +	{0, 0x1b, 0, F_SSU_DELAY, resp_start_stop, NULL,/* START STOP UNIT */
>  	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>  	{ARRAY_SIZE(sa_in_16_iarr), 0x9e, 0x10, F_SA_LOW | F_D_IN,
>  	    resp_readcap16, sa_in_16_iarr, /* SA_IN(16), READ CAPACITY(16) */
> @@ -606,7 +608,7 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	    resp_write_same_10, write_same_iarr,	/* WRITE SAME(10) */
>  		{10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0,
>  		 0, 0, 0, 0, 0} },
> -	{ARRAY_SIZE(sync_cache_iarr), 0x35, 0, F_LONG_DELAY | F_M_ACCESS,
> +	{ARRAY_SIZE(sync_cache_iarr), 0x35, 0, F_SYNC_DELAY | F_M_ACCESS,
>  	    resp_sync_cache, sync_cache_iarr,
>  	    {10,  0x7, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
>  	     0, 0, 0, 0} },			/* SYNC_CACHE (10) */
> @@ -667,6 +669,7 @@ static bool sdebug_strict = DEF_STRICT;
>  static bool sdebug_any_injecting_opt;
>  static bool sdebug_verbose;
>  static bool have_dif_prot;
> +static bool write_since_sync;
>  static bool sdebug_statistics = DEF_STATISTICS;
>  
>  static unsigned int sdebug_store_sectors;
> @@ -1607,6 +1610,7 @@ static int resp_start_stop(struct scsi_cmnd *scp,
>  {
>  	unsigned char *cmd = scp->cmnd;
>  	int power_cond, stop;
> +	bool changing;
>  
>  	power_cond = (cmd[4] & 0xf0) >> 4;
>  	if (power_cond) {
> @@ -1614,8 +1618,12 @@ static int resp_start_stop(struct scsi_cmnd *scp,
>  		return check_condition_result;
>  	}
>  	stop = !(cmd[4] & 1);
> +	changing = atomic_read(&devip->stopped) == !stop;
>  	atomic_xchg(&devip->stopped, stop);
> -	return (cmd[1] & 0x1) ? SDEG_RES_IMMED_MASK : 0; /* check IMMED bit */
> +	if (!changing || cmd[1] & 0x1)  /* state unchanged or IMMED set */
> +		return SDEG_RES_IMMED_MASK;
> +	else
> +		return 0;
>  }
>  
>  static sector_t get_sdebug_capacity(void)
> @@ -2473,6 +2481,7 @@ static int do_device_access(struct scsi_cmnd *scmd, u32 sg_skip, u64 lba,
>  	if (do_write) {
>  		sdb = scsi_out(scmd);
>  		dir = DMA_TO_DEVICE;
> +		write_since_sync = true;
>  	} else {
>  		sdb = scsi_in(scmd);
>  		dir = DMA_FROM_DEVICE;
> @@ -3583,6 +3592,7 @@ static int resp_get_lba_status(struct scsi_cmnd *scp,
>  static int resp_sync_cache(struct scsi_cmnd *scp,
>  			   struct sdebug_dev_info *devip)
>  {
> +	int res = 0;
>  	u64 lba;
>  	u32 num_blocks;
>  	u8 *cmd = scp->cmnd;
> @@ -3598,7 +3608,11 @@ static int resp_sync_cache(struct scsi_cmnd *scp,
>  		mk_sense_buffer(scp, ILLEGAL_REQUEST, LBA_OUT_OF_RANGE, 0);
>  		return check_condition_result;
>  	}
> -	return (cmd[1] & 0x2) ? SDEG_RES_IMMED_MASK : 0; /* check IMMED bit */
> +	if (!write_since_sync || cmd[1] & 0x2)
> +		res = SDEG_RES_IMMED_MASK;
> +	else		/* delay if write_since_sync and IMMED clear */
> +		write_since_sync = false;
> +	return res;
>  }
>  
>  #define RL_BUCKET_ELEMS 8
> @@ -5777,13 +5791,14 @@ static int scsi_debug_queuecommand(struct Scsi_Host *shost,
>  		return schedule_resp(scp, devip, errsts, pfp, 0, 0);
>  	else if ((sdebug_jdelay || sdebug_ndelay) && (flags & F_LONG_DELAY)) {
>  		/*
> -		 * If any delay is active, want F_LONG_DELAY to be at least 1
> +		 * If any delay is active, for F_SSU_DELAY want at least 1
>  		 * second and if sdebug_jdelay>0 want a long delay of that
> -		 * many seconds.
> +		 * many seconds; for F_SYNC_DELAY want 1/20 of that.
>  		 */
>  		int jdelay = (sdebug_jdelay < 2) ? 1 : sdebug_jdelay;
> +		int denom = (flags & F_SYNC_DELAY) ? 20 : 1;
>  
> -		jdelay = mult_frac(USER_HZ * jdelay, HZ, USER_HZ);
> +		jdelay = mult_frac(USER_HZ * jdelay, HZ, denom * USER_HZ);
>  		return schedule_resp(scp, devip, errsts, pfp, jdelay, 0);
>  	} else
>  		return schedule_resp(scp, devip, errsts, pfp, sdebug_jdelay,

With this patch, scsi_debug can be used in sync io test again, so

Tested-by: Ming Lei <ming.lei@redhat.com>
Reported-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming
Johannes Thumshirn April 11, 2018, 7:34 a.m. UTC | #2 
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Martin K. Petersen April 13, 2018, 2:37 a.m. UTC | #3 
Doug,

> A patch titled: "[PATCH v2] scsi_debug: implement IMMED bit"
> introduced long delays to the Start stop unit (SSU) and Synchronize
> cache (SC) commands when the IMMED bit is clear.  This patch makes
> those delays more realistic. It causes SSU to only delay when the
> start stop state is changed; SC only delays when there's been a write
> since the previous SC. It also reduced the SC delay from 1 second to
> 50 milliseconds.

Applied to 4.17/scsi-fixes. Thank you!
diff mbox

Patch

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 26ce022dd6f4..9ab19285a3d3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -234,11 +234,13 @@  static const char *sdebug_version_date = "20180128";
 #define F_INV_OP		0x200
 #define F_FAKE_RW		0x400
 #define F_M_ACCESS		0x800	/* media access */
-#define F_LONG_DELAY		0x1000
+#define F_SSU_DELAY		0x1000
+#define F_SYNC_DELAY		0x2000
 
 #define FF_RESPOND (F_RL_WLUN_OK | F_SKIP_UA | F_DELAY_OVERR)
 #define FF_MEDIA_IO (F_M_ACCESS | F_FAKE_RW)
 #define FF_SA (F_SA_HIGH | F_SA_LOW)
+#define F_LONG_DELAY		(F_SSU_DELAY | F_SYNC_DELAY)
 
 #define SDEBUG_MAX_PARTS 4
 
@@ -510,7 +512,7 @@  static const struct opcode_info_t release_iarr[] = {
 };
 
 static const struct opcode_info_t sync_cache_iarr[] = {
-	{0, 0x91, 0, F_LONG_DELAY | F_M_ACCESS, resp_sync_cache, NULL,
+	{0, 0x91, 0, F_SYNC_DELAY | F_M_ACCESS, resp_sync_cache, NULL,
 	    {16,  0x6, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
 	     0xff, 0xff, 0xff, 0xff, 0x3f, 0xc7} },	/* SYNC_CACHE (16) */
 };
@@ -553,7 +555,7 @@  static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
 	    resp_write_dt0, write_iarr,			/* WRITE(16) */
 		{16,  0xfa, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
 		 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7} },
-	{0, 0x1b, 0, F_LONG_DELAY, resp_start_stop, NULL,/* START STOP UNIT */
+	{0, 0x1b, 0, F_SSU_DELAY, resp_start_stop, NULL,/* START STOP UNIT */
 	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
 	{ARRAY_SIZE(sa_in_16_iarr), 0x9e, 0x10, F_SA_LOW | F_D_IN,
 	    resp_readcap16, sa_in_16_iarr, /* SA_IN(16), READ CAPACITY(16) */
@@ -606,7 +608,7 @@  static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
 	    resp_write_same_10, write_same_iarr,	/* WRITE SAME(10) */
 		{10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0,
 		 0, 0, 0, 0, 0} },
-	{ARRAY_SIZE(sync_cache_iarr), 0x35, 0, F_LONG_DELAY | F_M_ACCESS,
+	{ARRAY_SIZE(sync_cache_iarr), 0x35, 0, F_SYNC_DELAY | F_M_ACCESS,
 	    resp_sync_cache, sync_cache_iarr,
 	    {10,  0x7, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
 	     0, 0, 0, 0} },			/* SYNC_CACHE (10) */
@@ -667,6 +669,7 @@  static bool sdebug_strict = DEF_STRICT;
 static bool sdebug_any_injecting_opt;
 static bool sdebug_verbose;
 static bool have_dif_prot;
+static bool write_since_sync;
 static bool sdebug_statistics = DEF_STATISTICS;
 
 static unsigned int sdebug_store_sectors;
@@ -1607,6 +1610,7 @@  static int resp_start_stop(struct scsi_cmnd *scp,
 {
 	unsigned char *cmd = scp->cmnd;
 	int power_cond, stop;
+	bool changing;
 
 	power_cond = (cmd[4] & 0xf0) >> 4;
 	if (power_cond) {
@@ -1614,8 +1618,12 @@  static int resp_start_stop(struct scsi_cmnd *scp,
 		return check_condition_result;
 	}
 	stop = !(cmd[4] & 1);
+	changing = atomic_read(&devip->stopped) == !stop;
 	atomic_xchg(&devip->stopped, stop);
-	return (cmd[1] & 0x1) ? SDEG_RES_IMMED_MASK : 0; /* check IMMED bit */
+	if (!changing || cmd[1] & 0x1)  /* state unchanged or IMMED set */
+		return SDEG_RES_IMMED_MASK;
+	else
+		return 0;
 }
 
 static sector_t get_sdebug_capacity(void)
@@ -2473,6 +2481,7 @@  static int do_device_access(struct scsi_cmnd *scmd, u32 sg_skip, u64 lba,
 	if (do_write) {
 		sdb = scsi_out(scmd);
 		dir = DMA_TO_DEVICE;
+		write_since_sync = true;
 	} else {
 		sdb = scsi_in(scmd);
 		dir = DMA_FROM_DEVICE;
@@ -3583,6 +3592,7 @@  static int resp_get_lba_status(struct scsi_cmnd *scp,
 static int resp_sync_cache(struct scsi_cmnd *scp,
 			   struct sdebug_dev_info *devip)
 {
+	int res = 0;
 	u64 lba;
 	u32 num_blocks;
 	u8 *cmd = scp->cmnd;
@@ -3598,7 +3608,11 @@  static int resp_sync_cache(struct scsi_cmnd *scp,
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, LBA_OUT_OF_RANGE, 0);
 		return check_condition_result;
 	}
-	return (cmd[1] & 0x2) ? SDEG_RES_IMMED_MASK : 0; /* check IMMED bit */
+	if (!write_since_sync || cmd[1] & 0x2)
+		res = SDEG_RES_IMMED_MASK;
+	else		/* delay if write_since_sync and IMMED clear */
+		write_since_sync = false;
+	return res;
 }
 
 #define RL_BUCKET_ELEMS 8
@@ -5777,13 +5791,14 @@  static int scsi_debug_queuecommand(struct Scsi_Host *shost,
 		return schedule_resp(scp, devip, errsts, pfp, 0, 0);
 	else if ((sdebug_jdelay || sdebug_ndelay) && (flags & F_LONG_DELAY)) {
 		/*
-		 * If any delay is active, want F_LONG_DELAY to be at least 1
+		 * If any delay is active, for F_SSU_DELAY want at least 1
 		 * second and if sdebug_jdelay>0 want a long delay of that
-		 * many seconds.
+		 * many seconds; for F_SYNC_DELAY want 1/20 of that.
 		 */
 		int jdelay = (sdebug_jdelay < 2) ? 1 : sdebug_jdelay;
+		int denom = (flags & F_SYNC_DELAY) ? 20 : 1;
 
-		jdelay = mult_frac(USER_HZ * jdelay, HZ, USER_HZ);
+		jdelay = mult_frac(USER_HZ * jdelay, HZ, denom * USER_HZ);
 		return schedule_resp(scp, devip, errsts, pfp, jdelay, 0);
 	} else
 		return schedule_resp(scp, devip, errsts, pfp, sdebug_jdelay,