Re: [PATCH] Correct tag initialisation for commands

[Mike Christie <michaelc@cs.wisc.edu>]

Mike Christie wrote:
> Hannes Reinecke wrote:
>> Hi all,
>>
>> currently the initialisation of scmd->tag seems to be wrong.
>> We have this lines from scsi_lib.c:scsi_get_cmd_from_req():
>>
>>     /* pull a tag out of the request if we have one */
>>     cmd->tag = req->tag;
>>
>> which are supposed to fill the tag value for a given command.
>> However, the function is called from the ->prep_fn callback,
>> invoked from elv_next_request() in scsi_request_fn().
>>
>> In scsi_request_fn() we have:
>>
>>     req = elv_next_request(q);
>>     if (!req || !scsi_dev_queue_ready(q, sdev))
>>         break;
>>
>>     if (unlikely(!scsi_device_online(sdev))) {
>>         sdev_printk(KERN_ERR, sdev,
>>                 "rejecting I/O to offline device\n");
>>         scsi_kill_request(req, q);
>>         continue;
>>     }
>>
>>     /*
>>      * Remove the request from the request list.
>>      */
>>     if (!(blk_queue_tagged(q) && !blk_queue_start_tag(q, req)))
>>         blkdev_dequeue_request(req);
>>
>> ie the value of rq->tag is filled _after it has been copied
>> to scmd->tag.
>>
>> A proposed patch would be:
>>
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1055,8 +1055,6 @@ static struct scsi_cmnd 
>> *scsi_get_cmd_from_req(struct scsi_device *sdev,
>>          cmd = req->special;
>>      }
>>  
>> -    /* pull a tag out of the request if we have one */
>> -    cmd->tag = req->tag;
>>      cmd->request = req;
>>  
>>      return cmd;
>> @@ -1445,6 +1443,9 @@ static void scsi_request_fn(struct request_queue 
>> *q)
>>              blk_dump_rq_flags(req, "foo");
>>              BUG();
>>          }
>> +        /* pull a tag out of the request if we have one */
>> +        cmd->tag = req->tag;
>> +
>>          spin_lock(shost->host_lock);
>>  
>>          if (!scsi_host_queue_ready(q, shost, sdev))
>>
>> Comments?
>>
> 
> I saw that while working on qla4xxx. I think the reason no one ever 
> complained before is because of how most drivers use the tags. Was there 
> some driver accesing the scsi cmd tag value?

Ignore that question. I forgot about your other patch.