Correct tag initialisation for commands

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:

drivers/scsi/scsi_lib.c +3 -2

@@ -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?

Quoted 58 lines · 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:

drivers/scsi/scsi_lib.c +0 -2

@@ -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;

drivers/scsi/scsi_lib.c +3 -0

@@ -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);
 

Quoted 4 lines

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?

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.