Re: [PATCH 3/4] scsi: improved eh timeout handler

[Ren Mingxin <renmx@cn.fujitsu.com>]

Hi, Hannes:

On 06/07/2013 04:28 AM, Jörn Engel wrote:
> On Thu, 6 June 2013 22:39:14 +0200, Hannes Reinecke wrote:
>>>> +		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?
>
> If REQ_FAILFAST_DEV is set, this runs scsi_queue_insert(), which I
> would expect it should run scsi_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?

>>>> +			     (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);

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html