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

Hillf Danton
2010-10-28T13:29:48Z

On Thu, Oct 28, 2010 at 2:18 AM, Mike Christie <michaelc@cs.wisc.edu> wrote:
> On 10/26/2010 06:09 PM, James Bottomley wrote:
>>
>> On Tue, 2010-10-26 at 18:07 -0500, Mike Christie wrote:
>>>
>>> On 10/25/2010 03:53 PM, James Bottomley wrote:
>>>>
>>>> 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);
>>>
>>>
>>> I think I am hitting multiple bugs. One might be with this patch, but
>>> strangely they both hit __list_add bugs in the scsi eh. I think this
>>> needs to be list_splice_init().
>>
>> I think that's exactly right ... I keep getting tripped up by the _init
>> requirements for reusing lists (and list_heads).
>>
>>>> +               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);
>>>
>>> Because if the target resets work and move all the commands to the
>>> done_q here.
>>>
>>>
>>>> +                       else
>>>> +                               /* push back on work queue for further
>>>> processing */
>>>> +                               list_move(&scmd->eh_entry, work_q);
>>>> +               }
>>>>
>>>>        return list_empty(work_q);
>>>>   }
>>>
>>>
>>> Then this work_q is showing garbage in it from the splice I think.
>>> Without the list_splice_init I get that trace I cut and pasted in the
>>> other mail when I just force the scsi eh to run the target reset code.
>>>
>>> In the test, the target reset succeeds. The scsi_eh_finish_cmd call
>>> above moves all the cmds to the done_q. But the work_q still has junk in
>>> it, and so we return that we need to do more work and eventually we end
>>> up running scsi_eh_offline_sdevs which calls scsi_eh_finish_cmd on a
>>> command we already moved to the done_q.
>>
>> So does changing to the _init version fix at least the junk done_q
>> problem?
>>
>
> Yeah.
>

Blame me if it is buggy, please.

---

--- a/drivers/scsi/scsi_error.c 2010-09-13 07:07:38.000000000 +0800
+++ b/drivers/scsi/scsi_error.c 2010-10-28 21:27:24.000000000 +0800
@@ -1156,51 +1156,42 @@ static int scsi_eh_target_reset(struct S
struct list_head *work_q,
struct list_head *done_q)
{
- struct scsi_cmnd *scmd, *tgtr_scmd, *next;
+ struct scsi_cmnd *scmd, *next;
unsigned int id = 0;
int rtn;
+ int success;
+ LIST_HEAD(head);

- 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;
+ while (! list_empty(work_q)) {
+ scmd = list_entry(work_q->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);
+ success = (rtn == SUCCESS || rtn == FAST_IO_FAIL);
+ if (! success)
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, work_q, eh_entry) {
+ if (scmd_id(scmd) != id)
+ continue;
+ if (! success) {
+ list_move(&scmd->eh_entry, &head);
+ continue;
+ }
+ if (!scsi_device_online(scmd->device) ||
+ rtn == FAST_IO_FAIL ||
+ !scsi_eh_tur(scmd))
+ scsi_eh_finish_cmd(scmd, done_q);
+ else
+ list_move(&scmd->eh_entry, &head);
+ }
+ }
+ list_splice(&head, work_q);

return list_empty(work_q);
}
--
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