New SCSI command timeout handler
Subsystems (3)
cover New SCSI command timeout handler
Hi all,
this is the first step towards a new non-blocking error handler. This patch implements a new command timeout handler which will be sending command aborts inline without engaging SCSI EH.
In addition the commands will be returned directly if the command abort succeeded, cutting down recovery times dramatically.
With the original scsi error recovery I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 3.72732 s, 281 kB/s
real 2m14.475s
user 0m0.000s
sys 0m0.104s
with this patchset I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 31.5151 s, 33.3 kB/s
real 0m31.519s
user 0m0.000s
sys 0m0.088s
Test was to disable RSCN on the target port, disable the target port, and then start the 'dd' command as indicated.
As a proof-of-concept I've also enabled the new timeout handler for virtio, so that things can be tested out more easily.
Comments etc are welcome.
Hannes Reinecke (4):
scsi: move initialization of scmd->eh_entry
blk-timeout: add BLK_EH_SCHEDULED return code
scsi: improved eh timeout handler
virtio_scsi: use improved eh timeout handler
drivers/scsi/scsi_error.c | 82 ++++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_lib.c | 4 +- drivers/scsi/scsi_scan.c | 3 ++ drivers/scsi/scsi_transport_fc.c | 3 +- drivers/scsi/virtio_scsi.c | 8 ++++ include/linux/blkdev.h | 1 + include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_device.h | 2 + 8 files changed, 101 insertions(+), 3 deletions(-)
Signature · 1 line
1.7.12.4
PATCH 1/4 scsi: move initialization of scmd->eh_entry
The 'eh_entry' list might be used even before scsi_softirq_done() is called. Hence we should rather initialize it together with the other eh-related variables.
--- drivers/scsi/scsi_lib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/scsi_lib.c +2 -2
@@ -317,6 +317,8 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)memset (cmd->sense_buffer,0 ,SCSI_SENSE_BUFFERSIZE );if (cmd->cmd_len ==0 ) cmd->cmd_len =scsi_command_size (cmd->cmnd); +INIT_LIST_HEAD (&cmd->eh_entry); + cmd->eh_eflags =0 ; }void scsi_device_unbusy (struct scsi_device *sdev)
@@ -1482,8 +1484,6 @@ static void scsi_softirq_done(struct request *rq)unsigned long wait_for = (cmd->allowed +1 ) * rq->timeout;int disposition; -INIT_LIST_HEAD (&cmd->eh_entry); -atomic_inc (&cmd->device->iodone_cnt);if (cmd->result)atomic_inc (&cmd->device->ioerr_cnt);
Signature · 1 line
1.7.12.4
Re: [PATCH 1/4] scsi: move initialization of scmd->eh_entry
Quoted 6 lines · Hannes Reinecke
The '
eh_entry' list might be used even beforescsi_softirq_done()is called. Hence we should rather initialize it together with the other eh-related variables.Signed-off-by Hannes Reinecke <[email protected]>
Jörn
--
It is better to die of hunger having lived without grief and fear, than to live with a troubled spirit amid abundance.
-- Epictetus
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/4 blk-timeout: add BLK_EH_SCHEDULED return code
In reply to Hannes ReineckeAdd a 'BLK_EH_SCHEDULED' return code for blk-timeout to indicate that a delayed error recovery has been initiated.
--- drivers/scsi/scsi_error.c | 3 +++ include/linux/blkdev.h | 1 + 2 files changed, 4 insertions(+)
drivers/scsi/scsi_error.c +3 -0
@@ -146,6 +146,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req) rtn = host->hostt->eh_timed_out (scmd); scmd->result |=DID_TIME_OUT <<16 ; +/* Check for delayed EH scheduling */ +if (rtn ==BLK_EH_SCHEDULED ) +return BLK_EH_NOT_HANDLED ;if (unlikely (rtn ==BLK_EH_NOT_HANDLED && !scsi_eh_scmd_add (scmd,SCSI_EH_CANCEL_CMD )))
include/linux/blkdev.h +1 -0
@@ -238,6 +238,7 @@ enum blk_eh_timer_return {BLK_EH_NOT_HANDLED ,BLK_EH_HANDLED ,BLK_EH_RESET_TIMER , + BLK_EH_SCHEDULED, };typedef enum blk_eh_timer_return (rq_timed_out_fn )(struct request *);
Signature · 1 line
1.7.12.4
Re: [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code
Quoted 5 lines · Hannes Reinecke
Add a '
BLK_EH_SCHEDULED' return code for blk-timeout to indicate that a delayed error recovery has been initiated.Signed-off-by Hannes Reinecke <[email protected]>
Since the patch I tested diverged from your patches 2-4, this is only
Quoted 34 lines
--- drivers/scsi/scsi_error.c | 3 +++ include/linux/blkdev.h | 1 + 2 files changed, 4 insertions(+)drivers/scsi/scsi_error.c +3
-0@@ -146,6 +146,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req) rtn = host->hostt->eh_timed_out (scmd); scmd->result |=DID_TIME_OUT <<16 ; +/* Check for delayed EH scheduling */ +if (rtn ==BLK_EH_SCHEDULED ) +return BLK_EH_NOT_HANDLED ;if (unlikely (rtn ==BLK_EH_NOT_HANDLED && !scsi_eh_scmd_add (scmd,SCSI_EH_CANCEL_CMD )))include/linux/blkdev.h +1
-0@@ -238,6 +238,7 @@ enum blk_eh_timer_return {BLK_EH_NOT_HANDLED ,BLK_EH_HANDLED ,BLK_EH_RESET_TIMER , + BLK_EH_SCHEDULED, };typedef enum blk_eh_timer_return (rq_timed_out_fn )(struct request *);Signature · 1 line
1.7.12.4
Jörn
--
Geld macht nicht glücklich.
Glück macht nicht satt.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/4 scsi: improved eh timeout handler
In reply to Hannes ReineckeWhen a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the 'eh_abort_handler' LLDD callback.
Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.
This patch implements a new scsi_abort_command() function which invokes an asynchronous function scsi_eh_abort_handler() to abort the commands via 'eh_abort_handler'.
If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. For any other return code from 'eh_abort_handler' the command will be pushed onto the existing SCSI EH handler, or aborted with DID_TIME_OUT if that fails.
--- drivers/scsi/scsi_error.c | 79 ++++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_scan.c | 3 ++ drivers/scsi/scsi_transport_fc.c | 3 +- include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_device.h | 2 + 5 files changed, 87 insertions(+), 1 deletion(-)
drivers/scsi/scsi_error.c +79 -0
@@ -55,6 +55,8 @@ static void scsi_eh_done(struct scsi_cmnd *scmd);#define HOST_RESET_SETTLE_TIME (10)static int scsi_eh_try_stu (struct scsi_cmnd *scmd); +static int scsi_try_to_abort_cmd (struct scsi_host_template *hostt, +struct scsi_cmnd *scmd);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)
@@ -90,6 +92,83 @@ void scsi_schedule_eh(struct Scsi_Host *shost)EXPORT_SYMBOL_GPL (scsi_schedule_eh);/** +* scsi_eh_abort_handler - Handle command aborts +* @work: sdev on which commands should be aborted. +*/ +void +scsi_eh_abort_handler (struct work_struct *work) +{ +struct scsi_device *sdev = + container_of(work, struct scsi_device, abort_work); +struct Scsi_Host *shost = sdev->host; +struct scsi_cmnd *scmd, *tmp; +unsigned long flags; +int rtn; + +spin_lock_irqsave (&sdev->list_lock, flags); +list_for_each_entry_safe (scmd, tmp, &sdev->eh_abort_list, eh_entry) { +list_del_init (&scmd->eh_entry); +spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { +if (((scmd->request->cmd_flags &REQ_FAILFAST_DEV ) || + (scmd->request->cmd_type ==REQ_TYPE_BLOCK_PC )) && + (++scmd->retries <= scmd->allowed)) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"retry aborted command\n" )); + +scsi_queue_insert (scmd,SCSI_MLQUEUE_EH_RETRY ); + }else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags); +} + +/** +* scsi_abort_command - schedule a command abort +* @scmd: scmd to abort. +* +* We only need to abort commands after a command timeout +*/ +void +scsi_abort_command (struct scsi_cmnd *scmd) +{ +unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags); +if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ; +list_add (&scmd->eh_entry, &sdev->eh_abort_list); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd,"adding to eh_abort_list\n" )); +spin_unlock_irqrestore (&sdev->list_lock, flags); +if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command); + +/** * scsi_eh_scmd_add - add scsi cmd to error handling. * @scmd: scmd to run eh on. * @eh_flag: optional SCSI_EH flag.
drivers/scsi/scsi_scan.c +3 -0
@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);extern void scsi_evt_thread (struct work_struct *work);extern void scsi_requeue_run_queue (struct work_struct *work); +extern void scsi_eh_abort_handler (struct work_struct *work); sdev =kzalloc (sizeof (*sdev) + shost->transportt->device_size,GFP_ATOMIC );
@@ -251,9 +252,11 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,INIT_LIST_HEAD (&sdev->cmd_list);INIT_LIST_HEAD (&sdev->starved_entry);INIT_LIST_HEAD (&sdev->event_list); +INIT_LIST_HEAD (&sdev->eh_abort_list);spin_lock_init (&sdev->list_lock);INIT_WORK (&sdev->event_work, scsi_evt_thread);INIT_WORK (&sdev->requeue_work, scsi_requeue_run_queue); +INIT_WORK (&sdev->abort_work, scsi_eh_abort_handler); sdev->sdev_gendev.parent =get_device (&starget->dev); sdev->sdev_target = starget;
drivers/scsi/scsi_transport_fc.c +2 -1
@@ -2079,7 +2079,8 @@ fc_timed_out(struct scsi_cmnd *scmd)if (rport->port_state ==FC_PORTSTATE_BLOCKED )return BLK_EH_RESET_TIMER ; -return BLK_EH_NOT_HANDLED ; +scsi_abort_command (scmd); +return BLK_EH_SCHEDULED ; }/*
include/scsi/scsi_cmnd.h +1 -0
@@ -144,6 +144,7 @@ extern void scsi_put_command(struct scsi_cmnd *);extern void __scsi_put_command (struct Scsi_Host *,struct scsi_cmnd *,struct device *);extern void scsi_finish_command (struct scsi_cmnd *cmd); +extern void scsi_abort_command (struct scsi_cmnd *cmd);extern void *scsi_kmap_atomic_sg (struct scatterlist *sg,int sg_count,size_t *offset,size_t *len);
include/scsi/scsi_device.h +2 -0
@@ -80,6 +80,7 @@ struct scsi_device {spinlock_t list_lock;struct list_head cmd_list;/* queue of in use SCSI Command structures */ struct list_head starved_entry; +struct list_head eh_abort_list;struct scsi_cmnd *current_cmnd;/* currently active command */ unsigned short queue_depth;/* How deep of a queue we want */ unsigned short max_queue_depth;/* max queue depth */
@@ -180,6 +181,7 @@ struct scsi_device {struct execute_work ew;/* used to get process context on put */ struct work_struct requeue_work; +struct work_struct abort_work;struct scsi_dh_data *scsi_dh_data;enum scsi_device_state sdev_state;
Signature · 1 line
1.7.12.4
Re: [PATCH 3/4] scsi: improved eh timeout handler
Quoted 51 lines · Hannes Reinecke
When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the '
eh_abort_handler' LLDD callback.Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.
This patch implements a new
scsi_abort_command()function which invokes an asynchronous functionscsi_eh_abort_handler()to abort the commands via 'eh_abort_handler'.If the '
eh_abort_handler' returns SUCCESS orFAST_IO_FAILthe command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. For any other return code from 'eh_abort_handler' the command will be pushed onto the existing SCSI EH handler, or aborted withDID_TIME_OUTif that fails.Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_error.c | 79 ++++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_scan.c | 3 ++ drivers/scsi/scsi_transport_fc.c | 3 +- include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_device.h | 2 + 5 files changed, 87 insertions(+), 1 deletion(-)drivers/scsi/scsi_error.c +8
-0@@ -55,6 +55,8 @@ static void scsi_eh_done(struct scsi_cmnd *scmd);#define HOST_RESET_SETTLE_TIME (10)static int scsi_eh_try_stu (struct scsi_cmnd *scmd); +static int scsi_try_to_abort_cmd (struct scsi_host_template *hostt, +struct scsi_cmnd *scmd);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)@@ -90,6 +92,83 @@ void scsi_schedule_eh(struct Scsi_Host *shost)EXPORT_SYMBOL_GPL (scsi_schedule_eh);/** +* scsi_eh_abort_handler - Handle command aborts +* @work: sdev on which commands should be aborted. +*/ +void +scsi_eh_abort_handler (struct work_struct *work) +{
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 110
Quoted 10 lines
drivers/scsi/scsi_error.c +10
-0+struct scsi_device *sdev = + container_of(work, struct scsi_device, abort_work); +struct Scsi_Host *shost = sdev->host; +struct scsi_cmnd *scmd, *tmp; +unsigned long flags; +int rtn; + +spin_lock_irqsave (&sdev->list_lock, flags); +list_for_each_entry_safe (scmd, tmp, &sdev->eh_abort_list, eh_entry) { +list_del_init (&scmd->eh_entry);
The _init bit is not needed. I prefer list_del, as the poisoning sometimes helps catching bugs.
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 117
Quoted 7 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { + if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||
Am I being stupid again or should this be negated?
Quoted 37 lines
drivers/scsi/scsi_error.c +37
-0+ (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC)) && + (++scmd->retries <= scmd->allowed)) { + SCSI_LOG_ERROR_RECOVERY(3, + scmd_printk(KERN_WARNING, scmd, + "retry aborted command\n")); + + scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY); + } else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags); +} + +/** +* scsi_abort_command - schedule a command abort +* @scmd: scmd to abort. +* +* We only need to abort commands after a command timeout +*/ +void +scsi_abort_command (struct scsi_cmnd *scmd) +{
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 164
Quoted 10 lines
drivers/scsi/scsi_error.c +10
-0+unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags); +if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ; +list_add (&scmd->eh_entry, &sdev->eh_abort_list); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd,"adding to eh_abort_list\n" ));
The printk can be moved outside the spinlock. Who knows, maybe this will become a scalability bottleneck before the millenium is over.
Quoted 15 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command); + +/** * scsi_eh_scmd_add - add scsi cmd to error handling. * @scmd: scmd to run eh on. * @eh_flag: optional SCSI_EH flag. drivers/scsi/scsi_scan.c +0
-0
drivers/scsi/scsi_scan.c static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, · line 234
Quoted 4 lines
drivers/scsi/scsi_scan.c +1
-0@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);extern void scsi_evt_thread (struct work_struct *work);extern void scsi_requeue_run_queue (struct work_struct *work); +extern void scsi_eh_abort_handler (struct work_struct *work);
Function declarations in a .c file? Ick!
Quoted 64 lines
drivers/scsi/scsi_scan.c +2
-0sdev =kzalloc (sizeof (*sdev) + shost->transportt->device_size,GFP_ATOMIC );@@ -251,9 +252,11 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,INIT_LIST_HEAD (&sdev->cmd_list);INIT_LIST_HEAD (&sdev->starved_entry);INIT_LIST_HEAD (&sdev->event_list); +INIT_LIST_HEAD (&sdev->eh_abort_list);spin_lock_init (&sdev->list_lock);INIT_WORK (&sdev->event_work, scsi_evt_thread);INIT_WORK (&sdev->requeue_work, scsi_requeue_run_queue); +INIT_WORK (&sdev->abort_work, scsi_eh_abort_handler); sdev->sdev_gendev.parent =get_device (&starget->dev); sdev->sdev_target = starget;drivers/scsi/scsi_transport_fc.c +2
-1@@ -2079,7 +2079,8 @@ fc_timed_out(struct scsi_cmnd *scmd)if (rport->port_state ==FC_PORTSTATE_BLOCKED )return BLK_EH_RESET_TIMER ; -return BLK_EH_NOT_HANDLED ; +scsi_abort_command (scmd); +return BLK_EH_SCHEDULED ; }/* include/scsi/scsi_cmnd.h +1
-0@@ -144,6 +144,7 @@ extern void scsi_put_command(struct scsi_cmnd *);extern void __scsi_put_command (struct Scsi_Host *,struct scsi_cmnd *,struct device *);extern void scsi_finish_command (struct scsi_cmnd *cmd); +extern void scsi_abort_command (struct scsi_cmnd *cmd);extern void *scsi_kmap_atomic_sg (struct scatterlist *sg,int sg_count,size_t *offset,size_t *len);include/scsi/scsi_device.h +2
-0@@ -80,6 +80,7 @@ struct scsi_device {spinlock_t list_lock;struct list_head cmd_list;/* queue of in use SCSI Command structures */ struct list_head starved_entry; +struct list_head eh_abort_list;struct scsi_cmnd *current_cmnd;/* currently active command */ unsigned short queue_depth;/* How deep of a queue we want */ unsigned short max_queue_depth;/* max queue depth */ @@ -180,6 +181,7 @@ struct scsi_device {struct execute_work ew;/* used to get process context on put */ struct work_struct requeue_work; +struct work_struct abort_work;struct scsi_dh_data *scsi_dh_data;enum scsi_device_state sdev_state;Signature · 1 line
1.7.12.4
Jörn
--
You can't tell where a program is going to spend its time. Bottlenecks occur in surprising places, so don't try to second guess and put in a speed hack until you've proven that's where the bottleneck is. -- Rob Pike
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
Quoted 57 lines · Jörn Engel
Quoted 56 lines · Hannes Reinecke
When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the '
eh_abort_handler' LLDD callback.Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.
This patch implements a new
scsi_abort_command()function which invokes an asynchronous functionscsi_eh_abort_handler()to abort the commands via 'eh_abort_handler'.If the '
eh_abort_handler' returns SUCCESS orFAST_IO_FAILthe command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. For any other return code from 'eh_abort_handler' the command will be pushed onto the existing SCSI EH handler, or aborted withDID_TIME_OUTif that fails.Signed-off-by Hannes Reinecke <[email protected]>--- drivers/scsi/scsi_error.c | 79 ++++++++++++++++++++++++++++++++++++++++ drivers/scsi/scsi_scan.c | 3 ++ drivers/scsi/scsi_transport_fc.c | 3 +- include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_device.h | 2 + 5 files changed, 87 insertions(+), 1 deletion(-)drivers/scsi/scsi_error.c +13
-0@@ -55,6 +55,8 @@ static void scsi_eh_done(struct scsi_cmnd *scmd);#define HOST_RESET_SETTLE_TIME (10)static int scsi_eh_try_stu (struct scsi_cmnd *scmd); +static int scsi_try_to_abort_cmd (struct scsi_host_template *hostt, +struct scsi_cmnd *scmd);/* called with shost->host_lock held */ void scsi_eh_wakeup (struct Scsi_Host *shost)@@ -90,6 +92,83 @@ void scsi_schedule_eh(struct Scsi_Host *shost)EXPORT_SYMBOL_GPL (scsi_schedule_eh);/** +* scsi_eh_abort_handler - Handle command aborts +* @work: sdev on which commands should be aborted. +*/ +void +scsi_eh_abort_handler (struct work_struct *work) +{ +struct scsi_device *sdev = + container_of(work, struct scsi_device, abort_work); +struct Scsi_Host *shost = sdev->host; +struct scsi_cmnd *scmd, *tmp; +unsigned long flags;
Quoted 9 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 110
Quoted 5 lines
drivers/scsi/scsi_error.c +5
-0+int rtn; + +spin_lock_irqsave (&sdev->list_lock, flags); +list_for_each_entry_safe (scmd, tmp, &sdev->eh_abort_list, eh_entry) { +list_del_init (&scmd->eh_entry);The
_initbit is not needed. I preferlist_del, as the poisoning sometimes helps catching bugs.
Strictly speaking not.
But then I'm on the good side of programming who prefer to handle issues gracefully, not aborting with a kernel oops :-)
Quoted 10 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 117
Quoted 7 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { + if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||Am I being stupid again or should this be negated?
Knowing you I would think the former; where do you see the negation?
Quoted 42 lines
Quoted 42 lines
drivers/scsi/scsi_error.c +42
-0+ (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC)) && + (++scmd->retries <= scmd->allowed)) { + SCSI_LOG_ERROR_RECOVERY(3, + scmd_printk(KERN_WARNING, scmd, + "retry aborted command\n")); + + scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY); + } else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags); +} + +/** +* scsi_abort_command - schedule a command abort +* @scmd: scmd to abort. +* +* We only need to abort commands after a command timeout +*/ +void +scsi_abort_command (struct scsi_cmnd *scmd) +{ +unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags);
Quoted 9 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 164
Quoted 5 lines
drivers/scsi/scsi_error.c +5
-0+if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ; +list_add (&scmd->eh_entry, &sdev->eh_abort_list); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd,"adding to eh_abort_list\n" ));The printk can be moved outside the spinlock. Who knows, maybe this will become a scalability bottleneck before the millenium is over.
That's a debugging thing only. But yeah, you're right, it should move out of the spinlock.
Quoted 13 lines
Quoted 13 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command); + +/** * scsi_eh_scmd_add - add scsi cmd to error handling. * @scmd: scmd to run eh on. * @eh_flag: optional SCSI_EH flag. drivers/scsi/scsi_scan.c +0
-1--- a/drivers/scsi/scsi_scan.c
Quoted 9 lines
drivers/scsi/scsi_scan.c static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, · line 234
Quoted 6 lines
+++
b/drivers/scsi/scsi_scan.cdrivers/scsi/scsi_scan.c +1
-0@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);extern void scsi_evt_thread (struct work_struct *work);extern void scsi_requeue_run_queue (struct work_struct *work); +extern void scsi_eh_abort_handler (struct work_struct *work);Function declarations in a .c file? Ick!
Sing-along: We didn't start the fire, it always burning ...
I'm just following precedents here.
Thanks for the review.
Cheers,
Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
Quoted 11 lines · Hannes Reinecke
Quoted 11 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 117
Quoted 8 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { + if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||Am I being stupid again or should this be negated?
Quoted 1 line
Knowing you I would think the former; where do you see the negation?
If REQ_FAILFAST_DEV is set, this runs scsi_queue_insert(), which I would expect it should run scsi_finish_command().
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
drivers/scsi/scsi_error.c +26
-0+ (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC)) && + (++scmd->retries <= scmd->allowed)) { + SCSI_LOG_ERROR_RECOVERY(3, + scmd_printk(KERN_WARNING, scmd, + "retry aborted command\n")); + + scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY); + } else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags);
...
Quoted 11 lines
Quoted 8 lines
drivers/scsi/scsi_scan.c static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, · line 234
Quoted 5 lines
drivers/scsi/scsi_scan.c +1
-0@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);extern void scsi_evt_thread (struct work_struct *work);extern void scsi_requeue_run_queue (struct work_struct *work); +extern void scsi_eh_abort_handler (struct work_struct *work);Function declarations in a .c file? Ick!
Sing-along: We didn't start the fire, it always burning ...
I'm just following precedents here.
Fair enough.
Jörn
--
The cost of changing business rules is much more expensive for software than for a secretary.
-- unknown
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
Hi, Hannes:
Quoted 11 lines · Jörn Engel
Quoted 10 lines · Hannes Reinecke
Quoted 10 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost)
Quoted 7 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { + if (((scmd->request->cmd_flags& REQ_FAILFAST_DEV) ||Am I being stupid again or should this be negated?
Quoted 4 lines
Quoted 1 line
Knowing you I would think the former; where do you see the negation?
If
REQ_FAILFAST_DEVis set, this runsscsi_queue_insert(), which I would expect it should runscsi_finish_command().
I also think (scmd->request->cmd_flags & REQ_FAILFAST_DEV) and (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC) should be negated. I'm confused why not use !scsi_noretry_cmd(scmd) directly as your former patch here?
Quoted 43 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
drivers/scsi/scsi_error.c +26
-0+ (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC))&& + (++scmd->retries<= scmd->allowed)) { + SCSI_LOG_ERROR_RECOVERY(3, + scmd_printk(KERN_WARNING, scmd, + "retry aborted command\n")); + + scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY); + } else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags);...
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
drivers/scsi/scsi_error.c +16
-0+/** +* scsi_abort_command - schedule a command abort +* @scmd: scmd to abort. +* +* We only need to abort commands after a command timeout +*/ +void +scsi_abort_command (struct scsi_cmnd *scmd) +{ +unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags); +if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 169
Quoted 8 lines
drivers/scsi/scsi_error.c +8
-0+list_add (&scmd->eh_entry,&sdev->eh_abort_list); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd,"adding to eh_abort_list\n" )); +spin_unlock_irqrestore (&sdev->list_lock, flags); +if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command);
Should the name of function above be more ideographic/understandable? For example, scsi_abort_scmd_add? I was bewildered among functions named scsi_abort_eh_cmnd, scsi_eh_abort_cmds...
Thanks,
Ren
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
Quoted 14 lines · Ren Mingxin
Hi, Hannes:
Quoted 11 lines · Jörn Engel
Quoted 10 lines · Hannes Reinecke
Quoted 10 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost)
Quoted 7 lines
drivers/scsi/scsi_error.c +7
-0+spin_unlock_irqrestore (&sdev->list_lock, flags); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd, +"aborting command %p\n" , scmd)); + rtn =scsi_try_to_abort_cmd (shost->hostt, scmd); +if (rtn ==SUCCESS || rtn ==FAST_IO_FAIL ) { + if (((scmd->request->cmd_flags& REQ_FAILFAST_DEV) ||Am I being stupid again or should this be negated?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Knowing you I would think the former; where do you see the negation?
If
REQ_FAILFAST_DEVis set, this runsscsi_queue_insert(), which I would expect it should runscsi_finish_command().I also think (
scmd->request->cmd_flags&REQ_FAILFAST_DEV) and (scmd->request->cmd_type==REQ_TYPE_BLOCK_PC) should be negated.
Bummer. You are correct.
So far I've only encountered the 'BLOCK_PC' condition, which worked.
Quoted 3 lines
I'm confused why not use !scsi_noretry_cmd(scmd) directly as your former patch here?
Hehe. I've fallen into the trap myself. scsi_noretry_cmd() only works if you have a status for the command, and will only evaluate REQ_FAILFAST_DEV or BLOCK_PC if the command has a CHECK CONDITION status.
As this is the timeout handler we do _not_ have any status, so scsi_noretry_cmd() will always tell us that the command should be retried.
Quoted 49 lines
Quoted 49 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
+ (
scmd->request->cmd_type==
REQ_TYPE_BLOCK_PC))&&drivers/scsi/scsi_error.c +25
-0+ (++scmd->retries<= scmd->allowed)) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"retry aborted command\n" )); + +scsi_queue_insert (scmd,SCSI_MLQUEUE_EH_RETRY ); + } else { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"fast fail aborted command\n" )); + scmd->result |=DID_TRANSPORT_FAILFAST <<16 ; +scsi_finish_command (scmd); + } + }else { +if (!scsi_eh_scmd_add (scmd,0 )) { +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_WARNING , scmd, +"terminate aborted command\n" )); + scmd->result |=DID_TIME_OUT <<16 ; +scsi_finish_command (scmd); + } + } +spin_lock_irqsave (&sdev->list_lock, flags); + } +spin_unlock_irqrestore (&sdev->list_lock, flags);...
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
drivers/scsi/scsi_error.c +19
-0+/** +* scsi_abort_command - schedule a command abort +* @scmd: scmd to abort. +* +* We only need to abort commands after a command timeout +*/ +void +scsi_abort_command (struct scsi_cmnd *scmd) +{ +unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags); +if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ; +list_add (&scmd->eh_entry,&sdev->eh_abort_list); + SCSI_LOG_ERROR_RECOVERY(3, + scmd_printk(KERN_INFO, scmd, "adding toeh_abort_list\n"));
+spin_unlock_irqrestore(&sdev->list_lock, flags);
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 169
Quoted 4 lines
drivers/scsi/scsi_error.c +4
-0+if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command);Should the name of function above be more ideographic/understandable? For example,
scsi_abort_scmd_add? I was bewildered among functions namedscsi_abort_eh_cmnd,scsi_eh_abort_cmds...
scsi_abort_scmd_add()? That's even more confusing.
scsi_abort_command() does exactly this, it aborts a command. Yeah, the individual wrapper/callback function naming might be improved, but this is the one function which actually does what it advertises ...
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
In reply to Hannes Reineckedrivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 111
Quoted 5 lines · Hannes Reinecke
drivers/scsi/scsi_error.c +4
-0+spin_lock_irqsave (&sdev->list_lock, flags); +list_for_each_entry_safe (scmd, tmp, &sdev->eh_abort_list, eh_entry) { +list_del_init (&scmd->eh_entry); +spin_unlock_irqrestore (&sdev->list_lock, flags);
I never liked the list_for_each_entry_safe() loop but until this morning couldn't really say why. The problem is that it can finish with commands left on the list. And since your kick_worker checks for list_empty, there wouldn't be any more aborts.
Thread1 list state Thread2
head->1->2->head spin_lock_irqsave(); scmd = 1; tmp = 2; list_del_init(scmd); spin_unlock_irqrestore(); head->2->head spin_lock_irqsave(); kick_worker = 0; list_add(); spin_unlock_irqrestore(); head->3->2->head spin_lock_irqsave(); scmd = 2; tmp = head; list_del_init(scmd); spin_unlock_irqrestore(); head->3->head
And now that I think about nasty races, you could also do schedule_work() when the list is empty, but the worker thread is still running. I've had to debug similar cases and the approach is to waste a day or five, despair and eventually get lucky half a year later when you happen to notice the race. Rare random crashes in the kworker code a not my favorite sight.
Quoted 8 lines
drivers/scsi/scsi_error.c +8
-0+void +scsi_abort_command (struct scsi_cmnd *scmd) +{ +unsigned long flags; +int kick_worker =0 ; +struct scsi_device *sdev = scmd->device; + +spin_lock_irqsave (&sdev->list_lock, flags);
drivers/scsi/scsi_error.c void scsi_schedule_eh(struct Scsi_Host *shost) · line 169
Quoted 10 lines
drivers/scsi/scsi_error.c +10
-0+if (list_empty (&sdev->eh_abort_list)) + kick_worker =1 ; +list_add (&scmd->eh_entry, &sdev->eh_abort_list); +SCSI_LOG_ERROR_RECOVERY (3 , +scmd_printk (KERN_INFO , scmd,"adding to eh_abort_list\n" )); +spin_unlock_irqrestore (&sdev->list_lock, flags); +if (kick_worker) +schedule_work (&sdev->abort_work); +} +EXPORT_SYMBOL_GPL (scsi_abort_command);
Jörn
--
Time? What's that? Time is only worth what you do with it. -- Theo de Raadt
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] scsi: improved eh timeout handler
In reply to Hannes ReineckeQuoted 10 lines · Hannes Reinecke
When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the '
eh_abort_handler' LLDD callback.Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.
This patch implements a new
scsi_abort_command()function which invokes an asynchronous functionscsi_eh_abort_handler()to abort the commands via 'eh_abort_handler'.
Quoted 8 lines
If the '
eh_abort_handler' returns SUCCESS orFAST_IO_FAILthe command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. For any other return code from 'eh_abort_handler' the command will be pushed onto the existing SCSI EH handler, or aborted withDID_TIME_OUTif that fails.
Why would you do a retry at this low level? We already have a bad path, the IO may already be rerouted through another path and continuing with this IO here may interfere with the multipath handling.
Baruch
Re: [PATCH 3/4] scsi: improved eh timeout handler
Quoted 20 lines · Baruch Even
Quoted 10 lines · Hannes Reinecke
When a command runs into a timeout we need to send an 'ABORT TASK' TMF. This is typically done by the '
eh_abort_handler' LLDD callback.Conceptually, however, this function is a normal SCSI command, so there is no need to enter the error handler.
This patch implements a new
scsi_abort_command()function which invokes an asynchronous functionscsi_eh_abort_handler()to abort the commands via 'eh_abort_handler'.Quoted 8 lines
If the '
eh_abort_handler' returns SUCCESS orFAST_IO_FAILthe command will be retried if possible. If no retries are allowed the command will be returned immediately, as we have to assume the TMF succeeded and the command is completed with the LLDD. For any other return code from 'eh_abort_handler' the command will be pushed onto the existing SCSI EH handler, or aborted withDID_TIME_OUTif that fails.
Quoted 4 lines
Why would you do a retry at this low level? We already have a bad path, the IO may already be rerouted through another path and continuing with this IO here may interfere with the multipath handling.
For multipathing no retries would be set, so the command would be returned immediately.
I checked.
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/4 virtio_scsi: use improved eh timeout handler
In reply to Hannes ReineckeHook in the improved eh timeout handler for virtio.
--- drivers/scsi/virtio_scsi.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/scsi/virtio_scsi.c +8 -0
@@ -654,6 +654,12 @@ static int virtscsi_abort(struct scsi_cmnd *sc)return virtscsi_tmf (vscsi, cmd); } +static enum blk_eh_timer_return virtscsi_timedout (struct scsi_cmnd *scmd) +{ +scsi_abort_command (scmd); +return BLK_EH_SCHEDULED ; +} +static int virtscsi_target_alloc (struct scsi_target *starget) { struct virtio_scsi_target_state *tgt =
@@ -683,6 +689,7 @@ static struct scsi_host_template virtscsi_host_template_single = { .queuecommand = virtscsi_queuecommand_single, .eh_abort_handler = virtscsi_abort, .eh_device_reset_handler = virtscsi_device_reset, + .eh_timed_out = virtscsi_timedout, .can_queue = 1024, .dma_boundary = UINT_MAX,
@@ -699,6 +706,7 @@ static struct scsi_host_template virtscsi_host_template_multi = { .queuecommand = virtscsi_queuecommand_multi, .eh_abort_handler = virtscsi_abort, .eh_device_reset_handler = virtscsi_device_reset, + .eh_timed_out = virtscsi_timedout, .can_queue = 1024, .dma_boundary = UINT_MAX,
Signature · 1 line
1.7.12.4
Hi, Hannes:
Quoted 29 lines · Hannes Reinecke
this is the first step towards a new non-blocking error handler. This patch implements a new command timeout handler which will be sending command aborts inline without engaging SCSI EH.
In addition the commands will be returned directly if the command abort succeeded, cutting down recovery times dramatically.
With the original scsi error recovery I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 3.72732 s, 281 kB/sreal 2m14.475s
user 0m0.000s
sys 0m0.104swith this patchset I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 31.5151 s, 33.3 kB/sreal 0m31.519s
user 0m0.000s
sys 0m0.088s
Quoted 6 lines
Test was to disable RSCN on the target port, disable the target port, and then start the 'dd' command as indicated.
As a proof-of-concept I've also enabled the new timeout handler for virtio, so that things can be tested out more easily.
So this 31.5s is tested on virtio disks, right? Much faster than your former test via fc.
This approach may not work for some LLDDs as you said, but I wonder whether SAS is applicable(whether there will be later patches for SAS).
Thanks,
Ren
Quoted 35 lines · Ren Mingxin
Hi, Hannes:
Quoted 32 lines · Hannes Reinecke
this is the first step towards a new non-blocking error handler. This patch implements a new command timeout handler which will be sending command aborts inline without engaging SCSI EH.
In addition the commands will be returned directly if the command abort succeeded, cutting down recovery times dramatically.
With the original scsi error recovery I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 3.72732 s, 281 kB/sreal 2m14.475s
user 0m0.000s
sys 0m0.104swith this patchset I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync 2048+0 records in 2048+0 records out 1048576 bytes (1.0 MB) copied, 31.5151 s, 33.3 kB/sreal 0m31.519s
user 0m0.000s
sys 0m0.088sTest was to disable RSCN on the target port, disable the target port, and then start the 'dd' command as indicated.
Quoted 7 lines
Quoted 3 lines
As a proof-of-concept I've also enabled the new timeout handler for virtio, so that things can be tested out more easily.
So this 31.5s is tested on virtio disks, right? Much faster than your former test via fc.
No. This was done on HP Proliant /QLogic FC running against HP P2000.
There is a reason why I posted this new patchset :-)
(The old patchset had a bug where it wouldn't abort the TUR send by multipaths path-checker. Which were causing delays during switchover. This version now does terminate them properly) (Incidentally, it was the non-working scsi_noretry_cmd() which caused this bug :-)
Quoted 4 lines
This approach may not work for some LLDDs as you said, but I wonder whether SAS is applicable(whether there will be later patches for SAS).
SAS should be applicable for this, too. I'll check.
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Ren Mingxin
On 06/06/2013 05:43 PM, Hannes Reinecke wrote:
This approach may not work for some LLDDs as you said, but I wonder whether SAS is applicable(whether there will be later patches for SAS).
We have tested a derived patch on SAS and it did improve things.
Jörn
--
You ain't got no problem, Jules. I'm on the motherfucker. Go back in there, chill them niggers out and wait for the Wolf, who should be coming directly.
-- Marsellus Wallace
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html