Re: [PATCH] fix id computation in scsi_eh_target_reset()

[Hillf Danton <dhillf@xxxxxxxxx>]

On Tue, Oct 26, 2010 at 4:53 AM, James Bottomley
<James.Bottomley@xxxxxxx> wrote:
> On Mon, 2010-10-18 at 00:33 -0500, Mike Christie wrote:
>> On 10/14/2010 09:04 AM, Hillf Danton wrote:
>> > There seems that the id of the tgtr_scmd processed by
>> > scsi_try_target_reset() does not match the id in case that
>> > SUCCESS is returned by scsi_try_target_reset().
>> >
>> > The mismatch is fixed, and the loop to find the next highest
>> > is also simplified.
>> >
>> > Signed-off-by: Hillf Danton<dhillf@xxxxxxxxx>
>> > ---
>> >
>> > --- a/drivers/scsi/scsi_error.c   2010-09-13 07:07:38.000000000 +0800
>> > +++ b/drivers/scsi/scsi_error.c   2010-10-14 21:45:56.000000000 +0800
>> > @@ -1173,14 +1173,19 @@ static int scsi_eh_target_reset(struct S
>> > Â Â Â Â Â Â Â Â Â Â list_for_each_entry(scmd, work_q, eh_entry) {
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (scmd_id(scmd)> Âid&&
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â (!tgtr_scmd ||
>> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âscmd_id(tgtr_scmd)> Âscmd_id(scmd)))
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âscmd_id(tgtr_scmd)> Âscmd_id(scmd))) {
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tgtr_scmd = scmd;
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (1 + id == scmd_id(scmd))
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> > Â Â Â Â Â Â Â Â Â Â }
>> > Â Â Â Â Â Â }
>> > Â Â Â Â Â Â if (!tgtr_scmd)
>> > Â Â Â Â Â Â Â Â Â Â /* no more commands, that's it */
>> > Â Â Â Â Â Â Â Â Â Â break;
>> >
>> > + Â Â Â Â Â id = scmd_id(tgtr_scmd);
>> > +
>> > Â Â Â Â Â Â SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending target reset "
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "to target %d\n",
>> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â current->comm, id));
>>
>>
>> Thanks. Fixes the extra target resets I have been seeing and commands
>> not getting cleaned up properly.
>>
>> Reviewed-by: Mike Christie <michaelc@xxxxxxxxxxx>
>
> The routine is still a bit fiendishly complicated, though. ÂWhat about
> unwinding all of the id processing? it's only required if we want to do
> target resets in numerical order (a number which has no real meaning on
> a hot plug bus anyway).
>
> What about just a simple list processor that chunks down the work_q,
> resets the target and pulls all equal targets out, like this?
>
> James

The work looks fine. Thanks.

Reviewed-by: Hillf Danton <dhillf@xxxxxxxxx>

>
> ---
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 1de30eb..484b128 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1156,51 +1156,40 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct list_head *work_q,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct list_head *done_q)
> Â{
> - Â Â Â struct scsi_cmnd *scmd, *tgtr_scmd, *next;
> - Â Â Â unsigned int id = 0;
> - Â Â Â int rtn;
> + Â Â Â LIST_HEAD(tmp_list);
>
> - Â Â Â do {
> - Â Â Â Â Â Â Â tgtr_scmd = NULL;
> - Â Â Â Â Â Â Â list_for_each_entry(scmd, work_q, eh_entry) {
> - Â Â Â Â Â Â Â Â Â Â Â if (id == scmd_id(scmd)) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tgtr_scmd = scmd;
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> - Â Â Â Â Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â if (!tgtr_scmd) {
> - Â Â Â Â Â Â Â Â Â Â Â /* not one exactly equal; find the next highest */
> - Â Â Â Â Â Â Â Â Â Â Â list_for_each_entry(scmd, work_q, eh_entry) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (scmd_id(scmd) > id &&
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (!tgtr_scmd ||
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âscmd_id(tgtr_scmd) > scmd_id(scmd)))
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â tgtr_scmd = scmd;
> - Â Â Â Â Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â if (!tgtr_scmd)
> - Â Â Â Â Â Â Â Â Â Â Â /* no more commands, that's it */
> - Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â list_splice(work_q, &tmp_list);
> +
> + Â Â Â while (!list_empty(&tmp_list)) {
> + Â Â Â Â Â Â Â struct scsi_cmnd *next, *scmd;
> + Â Â Â Â Â Â Â int rtn;
> + Â Â Â Â Â Â Â unsigned int id;
> +
> + Â Â Â Â Â Â Â scmd = list_entry(tmp_list.next, struct scsi_cmnd, eh_entry);
> + Â Â Â Â Â Â Â id = scmd_id(scmd);
>
> Â Â Â Â Â Â Â ÂSCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending target reset "
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"to target %d\n",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcurrent->comm, id));
> - Â Â Â Â Â Â Â rtn = scsi_try_target_reset(tgtr_scmd);
> - Â Â Â Â Â Â Â if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
> - Â Â Â Â Â Â Â Â Â Â Â list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (id == scmd_id(scmd))
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (!scsi_device_online(scmd->device) ||
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rtn == FAST_IO_FAIL ||
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â !scsi_eh_tur(tgtr_scmd))
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â scsi_eh_finish_cmd(scmd,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âdone_q);
> - Â Â Â Â Â Â Â Â Â Â Â }
> - Â Â Â Â Â Â Â } else
> + Â Â Â Â Â Â Â rtn = scsi_try_target_reset(scmd);
> + Â Â Â Â Â Â Â if (rtn != SUCCESS && rtn != FAST_IO_FAIL)
> Â Â Â Â Â Â Â Â Â Â Â ÂSCSI_LOG_ERROR_RECOVERY(3, printk("%s: Target reset"
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â" failed target: "
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â"%d\n",
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcurrent->comm, id));
> - Â Â Â Â Â Â Â id++;
> - Â Â Â } while(id != 0);
> + Â Â Â Â Â Â Â list_for_each_entry_safe(scmd, next, &tmp_list, eh_entry) {
> + Â Â Â Â Â Â Â Â Â Â Â if (scmd_id(scmd) != id)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
> +
> + Â Â Â Â Â Â Â Â Â Â Â if ((rtn == SUCCESS || rtn == FAST_IO_FAIL)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â && (!scsi_device_online(scmd->device) ||
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Ârtn == FAST_IO_FAIL || !scsi_eh_tur(scmd)))
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â scsi_eh_finish_cmd(scmd, done_q);
> + Â Â Â Â Â Â Â Â Â Â Â else
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* push back on work queue for further processing */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â list_move(&scmd->eh_entry, work_q);
> + Â Â Â Â Â Â Â }
> + Â Â Â }
>
> Â Â Â Âreturn list_empty(work_q);
> Â}
>
>
>
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±þÇ‹ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf