[PATCH v3 0/4] SCSI core patches

[PATCH v3 0/4] SCSI core patches

[Bart Van Assche]

Hi Martin,

Please consider these SCSI core patches for the next merge window.

Thanks,

Bart.

Changes compared to v2:
- Dropped patch "scsi: core: Update a source code comment".
- In patch "scsi: core: Trace SCSI sense data", changed the format for the
  sense key and left out a superfluous parenthese.
- In patch "scsi: core: Only kick the requeue list if necessary", moved the
  blk_mq_kick_requeue_list() from scsi_run_host_queues() into scsi_run_queue().

Changes compared to v1:
- Improved the SCSI tracing patch as requested by Steven Rostedt and
  Niklas Cassel.
- Added patch "scsi: core: Delay running the queue if the host is blocked".

Bart Van Assche (4):
  scsi: core: Use min() instead of open-coding it
  scsi: core: Trace SCSI sense data
  scsi: core: Only kick the requeue list if necessary
  scsi: core: Delay running the queue if the host is blocked

 drivers/scsi/scsi_common.c  |  3 +--
 drivers/scsi/scsi_lib.c     | 15 +++++++++------
 include/trace/events/scsi.h | 21 +++++++++++++++++++--
 3 files changed, 29 insertions(+), 10 deletions(-)

[PATCH v3 1/4] scsi: core: Use min() instead of open-coding it

[Bart Van Assche]

Use min() instead of open-coding it in scsi_normalize_sense().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 6e50e81a8216..24dec80a6253 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -176,8 +176,7 @@ bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 		if (sb_len > 2)
 			sshdr->sense_key = (sense_buffer[2] & 0xf);
 		if (sb_len > 7) {
-			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
-					 sb_len : (sense_buffer[7] + 8);
+			sb_len = min(sb_len, sense_buffer[7] + 8);
 			if (sb_len > 12)
 				sshdr->asc = sense_buffer[12];
 			if (sb_len > 13)

[PATCH v3 2/4] scsi: core: Trace SCSI sense data

[Bart Van Assche]

If a command fails, SCSI sense data is essential to determine why it
failed. Hence make the sense key, ASC and ASCQ codes available in the
ftrace output.

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/trace/events/scsi.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
index a2c7befd451a..512792317577 100644
--- a/include/trace/events/scsi.h
+++ b/include/trace/events/scsi.h
@@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		__field( unsigned int,	prot_sglen )
 		__field( unsigned char,	prot_op )
 		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
+		__field( u8, sense_key )
+		__field( u8, asc )
+		__field( u8, ascq )
 	),
 
 	TP_fast_assign(
+		struct scsi_sense_hdr sshdr;
+
 		__entry->host_no	= cmd->device->host->host_no;
 		__entry->channel	= cmd->device->channel;
 		__entry->id		= cmd->device->id;
@@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
 		__entry->prot_op	= scsi_get_prot_op(cmd);
 		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
+		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
+		    scsi_command_normalize_sense(cmd, &sshdr)) {
+			__entry->sense_key = sshdr.sense_key;
+			__entry->asc = sshdr.asc;
+			__entry->ascq = sshdr.ascq;
+		} else {
+			__entry->sense_key = 0;
+			__entry->asc = 0;
+			__entry->ascq = 0;
+		}
 	),
 
 	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
 		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
-		  "result=(driver=%s host=%s message=%s status=%s)",
+		  "result=(driver=%s host=%s message=%s status=%s "
+		  "sense_key=%#x asc=%#x ascq=%#x)",
 		  __entry->host_no, __entry->channel, __entry->id,
 		  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
 		  show_prot_op_name(__entry->prot_op), __entry->driver_tag,
@@ -299,7 +315,8 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		  "DRIVER_OK",
 		  show_hostbyte_name(((__entry->result) >> 16) & 0xff),
 		  "COMMAND_COMPLETE",
-		  show_statusbyte_name(__entry->result & 0xff))
+		  show_statusbyte_name(__entry->result & 0xff),
+		  __entry->sense_key, __entry->asc, __entry->ascq)
 );
 
 DEFINE_EVENT(scsi_cmd_done_timeout_template, scsi_dispatch_cmd_done,

[PATCH v3 3/4] scsi: core: Only kick the requeue list if necessary

[Bart Van Assche]

Instead of running the request queue of each device associated with a
host every 3 ms (BLK_MQ_RESOURCE_DELAY) while host error handling is in
progress, run the request queue after error handling has finished.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e59eb0cbfc83..e4f34217b879 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -122,11 +122,9 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd, unsigned long msecs)
 		WARN_ON_ONCE(true);
 	}
 
-	if (msecs) {
-		blk_mq_requeue_request(rq, false);
+	blk_mq_requeue_request(rq, false);
+	if (!scsi_host_in_recovery(cmd->device->host))
 		blk_mq_delay_kick_requeue_list(rq->q, msecs);
-	} else
-		blk_mq_requeue_request(rq, true);
 }
 
 /**
@@ -165,7 +163,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
 	 */
 	cmd->result = 0;
 
-	blk_mq_requeue_request(scsi_cmd_to_rq(cmd), true);
+	blk_mq_requeue_request(scsi_cmd_to_rq(cmd),
+			       !scsi_host_in_recovery(cmd->device->host));
 }
 
 /**
@@ -449,6 +448,7 @@ static void scsi_run_queue(struct request_queue *q)
 	if (!list_empty(&sdev->host->starved_list))
 		scsi_starved_list_run(sdev->host);
 
+	blk_mq_kick_requeue_list(q);
 	blk_mq_run_hw_queues(q, false);
 }
 
@@ -499,6 +499,9 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 
 static void scsi_run_queue_async(struct scsi_device *sdev)
 {
+	if (scsi_host_in_recovery(sdev->host))
+		return;
+
 	if (scsi_target(sdev)->single_lun ||
 	    !list_empty(&sdev->host->starved_list)) {
 		kblockd_schedule_work(&sdev->requeue_work);

[PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Bart Van Assche]

Tell the block layer to rerun the queue after a delay instead of
immediately if the SCSI LLD decided to block the host.

Note: neither scsi_run_host_queues() nor scsi_run_queue() are called
from the fast path.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e4f34217b879..b37718ebbbfc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1767,7 +1767,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		break;
 	case BLK_STS_RESOURCE:
 	case BLK_STS_ZONE_RESOURCE:
-		if (scsi_device_blocked(sdev))
+		if (scsi_device_blocked(sdev) || shost->host_self_blocked)
 			ret = BLK_STS_DEV_RESOURCE;
 		break;
 	case BLK_STS_AGAIN:

Re: [PATCH v3 2/4] scsi: core: Trace SCSI sense data

[Ming Lei]

On Wed, May 17, 2023 at 04:09:25PM -0700, Bart Van Assche wrote:
> If a command fails, SCSI sense data is essential to determine why it
> failed. Hence make the sense key, ASC and ASCQ codes available in the
> ftrace output.
> 
> Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/trace/events/scsi.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index a2c7befd451a..512792317577 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__field( unsigned int,	prot_sglen )
>  		__field( unsigned char,	prot_op )
>  		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
> +		__field( u8, sense_key )
> +		__field( u8, asc )
> +		__field( u8, ascq )
>  	),
>  
>  	TP_fast_assign(
> +		struct scsi_sense_hdr sshdr;
> +
>  		__entry->host_no	= cmd->device->host->host_no;
>  		__entry->channel	= cmd->device->channel;
>  		__entry->id		= cmd->device->id;
> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>  		__entry->prot_op	= scsi_get_prot_op(cmd);
>  		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
> +		    scsi_command_normalize_sense(cmd, &sshdr)) {
> +			__entry->sense_key = sshdr.sense_key;
> +			__entry->asc = sshdr.asc;
> +			__entry->ascq = sshdr.ascq;
> +		} else {
> +			__entry->sense_key = 0;
> +			__entry->asc = 0;
> +			__entry->ascq = 0;
> +		}
>  	),
>  
>  	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>  		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
> -		  "result=(driver=%s host=%s message=%s status=%s)",
> +		  "result=(driver=%s host=%s message=%s status=%s "
> +		  "sense_key=%#x asc=%#x ascq=%#x)",

This way probably breaks userspace script or utility, maybe you can
just append "sense(sense_key=%#x asc=%#x ascq=%#x)" only.


thanks,
Ming

Re: [PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Ming Lei]

On Wed, May 17, 2023 at 04:09:27PM -0700, Bart Van Assche wrote:
> Tell the block layer to rerun the queue after a delay instead of
> immediately if the SCSI LLD decided to block the host.
> 
> Note: neither scsi_run_host_queues() nor scsi_run_queue() are called
> from the fast path.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e4f34217b879..b37718ebbbfc 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1767,7 +1767,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		break;
>  	case BLK_STS_RESOURCE:
>  	case BLK_STS_ZONE_RESOURCE:
> -		if (scsi_device_blocked(sdev))
> +		if (scsi_device_blocked(sdev) || shost->host_self_blocked)
>  			ret = BLK_STS_DEV_RESOURCE;

What if scsi_unblock_requests() is just called after the above check and
before returning to block layer core? Then this request is invisible to
scsi_run_host_queues()<-scsi_unblock_requests(), and io hang happens.


Thanks,
Ming

Re: [PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Bart Van Assche]

On 5/17/23 18:16, Ming Lei wrote:
> On Wed, May 17, 2023 at 04:09:27PM -0700, Bart Van Assche wrote:
>> @@ -1767,7 +1767,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>>   		break;
>>   	case BLK_STS_RESOURCE:
>>   	case BLK_STS_ZONE_RESOURCE:
>> -		if (scsi_device_blocked(sdev))
>> +		if (scsi_device_blocked(sdev) || shost->host_self_blocked)
>>   			ret = BLK_STS_DEV_RESOURCE;
> 
> What if scsi_unblock_requests() is just called after the above check and
> before returning to block layer core? Then this request is invisible to
> scsi_run_host_queues()<-scsi_unblock_requests(), and io hang happens.

If returning BLK_STS_DEV_RESOURCE could cause an I/O hang, wouldn't that 
be a bug in the block layer core? Isn't the block layer core expected to 
rerun the queue after a delay if a block driver returns 
BLK_STS_DEV_RESOURCE? See also blk_mq_dispatch_rq_list().

Thanks,

Bart.

Re: [PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Ming Lei]

On Wed, May 17, 2023 at 07:34:38PM -0700, Bart Van Assche wrote:
> On 5/17/23 18:16, Ming Lei wrote:
> > On Wed, May 17, 2023 at 04:09:27PM -0700, Bart Van Assche wrote:
> > > @@ -1767,7 +1767,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
> > >   		break;
> > >   	case BLK_STS_RESOURCE:
> > >   	case BLK_STS_ZONE_RESOURCE:
> > > -		if (scsi_device_blocked(sdev))
> > > +		if (scsi_device_blocked(sdev) || shost->host_self_blocked)
> > >   			ret = BLK_STS_DEV_RESOURCE;
> > 
> > What if scsi_unblock_requests() is just called after the above check and
> > before returning to block layer core? Then this request is invisible to
> > scsi_run_host_queues()<-scsi_unblock_requests(), and io hang happens.
> 
> If returning BLK_STS_DEV_RESOURCE could cause an I/O hang, wouldn't that be
> a bug in the block layer core? Isn't the block layer core expected to rerun
> the queue after a delay if a block driver returns BLK_STS_DEV_RESOURCE? See
> also blk_mq_dispatch_rq_list().

Please see comment for BLK_STS_DEV_RESOURCE:

/*
 * BLK_STS_DEV_RESOURCE is returned from the driver to the block layer if
 * device related resources are unavailable, but the driver can guarantee
 * that the queue will be rerun in the future once resources become
 * available again. This is typically the case for device specific
 * resources that are consumed for IO. If the driver fails allocating these
 * resources, we know that inflight (or pending) IO will free these
 * resource upon completion.

Basically it requires driver to re-run queue.

In reality, it can be full of race, maybe we can just remove
BLK_STS_DEV_RESOURCE.

Thanks,
Ming

Re: [PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Hannes Reinecke]

On 5/18/23 04:34, Bart Van Assche wrote:
> On 5/17/23 18:16, Ming Lei wrote:
>> On Wed, May 17, 2023 at 04:09:27PM -0700, Bart Van Assche wrote:
>>> @@ -1767,7 +1767,7 @@ static blk_status_t scsi_queue_rq(struct 
>>> blk_mq_hw_ctx *hctx,
>>>           break;
>>>       case BLK_STS_RESOURCE:
>>>       case BLK_STS_ZONE_RESOURCE:
>>> -        if (scsi_device_blocked(sdev))
>>> +        if (scsi_device_blocked(sdev) || shost->host_self_blocked)
>>>               ret = BLK_STS_DEV_RESOURCE;
>>
>> What if scsi_unblock_requests() is just called after the above check and
>> before returning to block layer core? Then this request is invisible to
>> scsi_run_host_queues()<-scsi_unblock_requests(), and io hang happens.
> 
> If returning BLK_STS_DEV_RESOURCE could cause an I/O hang, wouldn't that 
> be a bug in the block layer core? Isn't the block layer core expected to 
> rerun the queue after a delay if a block driver returns 
> BLK_STS_DEV_RESOURCE? See also blk_mq_dispatch_rq_list().
> 
DEV_RESOURCE is a tricky thing; it actually implies that the _device_ is 
blocked, and no further I/O is possible.
And it's actually the responsibility of the device/driver to kick the 
queue again once the contention/block/whatever is done.
So no, the block layer should not do anything here, relying on the 
driver to do something.

Which is why returning DEV_RESOURCE is not recommended, seeing that it's 
easy to get it wrong...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v3 4/4] scsi: core: Delay running the queue if the host is blocked

[Bart Van Assche]

On 5/17/23 19:39, Ming Lei wrote:
> In reality, it can be full of race, maybe we can just remove
> BLK_STS_DEV_RESOURCE.

We need a better solution than changing BLK_STS_DEV_RESOURCE into 
BLK_STS_RESOURCE. Otherwise power usage will go up in battery-powered 
devices that use block drivers that return BLK_STS_DEV_RESOURCE today.

Thanks,

Bart.

Re: [PATCH v3 2/4] scsi: core: Trace SCSI sense data

[Bart Van Assche]

On 5/17/23 18:00, Ming Lei wrote:
> On Wed, May 17, 2023 at 04:09:25PM -0700, Bart Van Assche wrote:
>>   	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>>   		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
>> -		  "result=(driver=%s host=%s message=%s status=%s)",
>> +		  "result=(driver=%s host=%s message=%s status=%s "
>> +		  "sense_key=%#x asc=%#x ascq=%#x)",
> 
> This way probably breaks userspace script or utility, maybe you can
> just append "sense(sense_key=%#x asc=%#x ascq=%#x)" only.

I will make this change.

Thanks,

Bart.