[PATCH] Correct tag initialisation for commands

[PATCH] Correct tag initialisation for commands

[Hannes Reinecke]

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?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
-
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

Re: [PATCH] Correct tag initialisation for commands

[Mike Christie]

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?

qla4xxx and other drivers, if I remember drivers like ipr and qla2xxx, 
never access scsi_cmd->tag. Instead they use scsi_find_tag to go from a 
tag to a command or they use scsi_populate_tag_msg to get the MSG_* bits.

If every ones uses the scsi_tcq wrappers, maybe the scsi_cmnd->tag field 
would be something that could be deleted and help Mathew's goals to 
shrink the struct.

If not your patch looks ok, but maybe we could convert to using the 
wrappers since it does not seem that there is a good reason to duplicate 
the tag now that every scsi command has proper request backing.

Re: [PATCH] Correct tag initialisation for commands

[Mike Christie]

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.

Re: [PATCH] Correct tag initialisation for commands

[Matthew Wilcox]

On Fri, Oct 19, 2007 at 12:57:20PM -0500, Mike Christie wrote:
> If every ones uses the scsi_tcq wrappers, maybe the scsi_cmnd->tag field 
> would be something that could be deleted and help Mathew's goals to 
> shrink the struct.

It's something I'd thought about, but we don't have any other 'char'
sized pieces of data in the scsi_cmnd, and they're all nicely packed
together, so it won't save us anything today.  If we want to get rid of
it for other reasons, that's fine, of course ;-)

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."