Skip to contentSkip to conversation

p00001_scsi_tcq_queue_lock

PATCH In review Brian King 2 participants 5 messages Aug 3, 2004 – Sep 17, 2004

Subsystems (2)
SCSI Block layer
lore
Share & tools
lore

Brian King

author
#0 lore raw

PATCH 1/1 p00001_scsi_tcq_queue_lock

Signature · 4 lines
-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Add locking to scsi_activate_tcq and scsi_deactivate_tcq to fix a race condition that can occur when disabling tcqing with commands in flight.

Signed-off-by Brian King <[email protected]>

---

 linux-2.6.8-rc2-bjking1/include/scsi/scsi_tcq.h |    8 ++++++++
 1 files changed, 8 insertions(+)
include/scsi/scsi_tcq.h renamed from include/scsi/scsi_tcq.h~scsi_tcq_queue_lock +8 -0
diff -puN include/scsi/scsi_tcq.h~scsi_tcq_queue_lock include/scsi/scsi_tcq.h
@@ -25,9 +25,13 @@
  **/
 static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
 {
+	unsigned long flags;
+
         if (sdev->tagged_supported) {
+		spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 		if (!blk_queue_tagged(sdev->request_queue))
 			blk_queue_init_tags(sdev->request_queue, depth, NULL);
+		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, depth);
         }
 }

@@ -38,8 +42,12 @@ static inline void scsi_activate_tcq(str
  **/
 static inline void scsi_deactivate_tcq(struct scsi_device *sdev, int depth)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	if (blk_queue_tagged(sdev->request_queue))
 		blk_queue_free_tags(sdev->request_queue);
+	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 	scsi_adjust_queue_depth(sdev, 0, depth);
 }

_

#1 lore raw
Quoted 2 lines · Brian King

Add locking to scsi_activate_tcq and scsi_deactivate_tcq to fix a race condition that can occur when disabling tcqing with commands in flight.

It's possible to do it like this. However, we should really quiesce the SCSI device before disabling tcq. It is not legal in the scsi spec to have both tagged and untagged commands outstanding. Most drives do the right thing and return BUSY to an untagged request if they have tags in flight. Also, we have to wait until all tags return before freeing the block layer tag structures...and there's always the devices that do strange things in this situation...

James

author
#2 lore raw
Quoted 11 lines · James Bottomley
Quoted 2 lines · Brian King

Add locking to scsi_activate_tcq and scsi_deactivate_tcq to fix a race condition that can occur when disabling tcqing with commands in flight.

It's possible to do it like this. However, we should really quiesce the SCSI device before disabling tcq. It is not legal in the scsi spec to have both tagged and untagged commands outstanding. Most drives do the right thing and return BUSY to an untagged request if they have tags in flight.

Ok. I agree that we do not want both tagged and untagged requests going to the device. In the case of the ipr driver, the adapter microcode handles this for me. If there are tagged requests outstanding to a device and an untagged request is sent, the tagged requests must first finish, then the untagged request is sent to the device with no other requests outstanding.

Quoted 3 lines

Also, we have to wait until all tags return before freeing the block layer tag structures...and there's always the devices that do strange things in this situation...

I sent the following patch to Jens last week as well and he applied it. This fixes the free problem you mention.

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
Signed-off-by Brian King <[email protected]>

---

 linux-2.6.8-rc2-bjking1/drivers/block/ll_rw_blk.c |   47 +++++++++++++++++-----
 linux-2.6.8-rc2-bjking1/include/scsi/scsi_tcq.h   |    8 +++
 2 files changed, 46 insertions(+), 9 deletions(-)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~blk_queue_free_tags_bug2 +38 -9
diff -puN drivers/block/ll_rw_blk.c~blk_queue_free_tags_bug2 drivers/block/ll_rw_blk.c
@@ -482,15 +482,14 @@ struct request *blk_queue_find_tag(reque
 EXPORT_SYMBOL(blk_queue_find_tag);
 
 /**
- * blk_queue_free_tags - release tag maintenance info
+ * _blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
  *
  *  Notes:
  *    blk_cleanup_queue() will take care of calling this function, if tagging
- *    has been used. So there's usually no need to call this directly, unless
- *    tagging is just being disabled but the queue remains in function.
+ *    has been used.
  **/
-void blk_queue_free_tags(request_queue_t *q)
+static void _blk_queue_free_tags(request_queue_t *q)
 {
 	struct blk_queue_tag *bqt = q->queue_tags;
 

@@ -511,7 +510,21 @@ void blk_queue_free_tags(request_queue_t
 	}
 
 	q->queue_tags = NULL;
-	q->queue_flags &= ~(1 << QUEUE_FLAG_QUEUED);
+	clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+}
+
+/**
+ * blk_queue_free_tags - release tag maintenance info
+ * @q:  the request queue for the device
+ *
+ *  Notes:
+ *    blk_cleanup_queue() will take care of calling this function, if tagging
+ *    has been used. So there's usually no need to call this directly, unless
+ *    tagging is just being disabled but the queue remains in function.
+ **/
+void blk_queue_free_tags(request_queue_t *q)
+{
+	clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 }
 
 EXPORT_SYMBOL(blk_queue_free_tags);

@@ -564,13 +577,22 @@ fail:
 int blk_queue_init_tags(request_queue_t *q, int depth,
 			struct blk_queue_tag *tags)
 {
-	if (!tags) {
+	int rc;
+
+	BUG_ON(tags && q->queue_tags && tags != q->queue_tags);
+
+	if (!tags && !q->queue_tags) {
 		tags = kmalloc(sizeof(struct blk_queue_tag), GFP_ATOMIC);
 		if (!tags)
 			goto fail;
 
 		if (init_tag_map(q, tags, depth))
 			goto fail;
+	} else if (q->queue_tags) {
+		if ((rc = blk_queue_resize_tags(q, depth)))
+			return rc;
+		set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+		return 0;
 	} else
 		atomic_inc(&tags->refcnt);
 

@@ -578,7 +600,7 @@ int blk_queue_init_tags(request_queue_t 
 	 * assign it, all done
 	 */
 	q->queue_tags = tags;
-	q->queue_flags |= (1 << QUEUE_FLAG_QUEUED);
+	set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 	return 0;
 fail:
 	kfree(tags);

@@ -1333,8 +1355,8 @@ void blk_cleanup_queue(request_queue_t *
 	if (rl->rq_pool)
 		mempool_destroy(rl->rq_pool);
 
-	if (blk_queue_tagged(q))
-		blk_queue_free_tags(q);
+	if (q->queue_tags)
+		_blk_queue_free_tags(q);
 
 	kmem_cache_free(requestq_cachep, q);
 }

@@ -2034,6 +2056,13 @@ void __blk_put_request(request_queue_t *
 
 		elv_completed_request(q, req);
 
+		if (!list_empty(&req->queuelist)) {
+			if (req->flags & REQ_QUEUED)
+				printk(KERN_ERR "REQ_QUEUED still set in __blk_put_request\n");
+			printk(KERN_ERR "req: %p, q: %p, req_flags: %lx, q_flags: %lx\n",
+			       req, q, req->flags, q->queue_flags);
+		}
+
 		BUG_ON(!list_empty(&req->queuelist));
 
 		blk_free_request(q, req);
include/scsi/scsi_tcq.h renamed from include/scsi/scsi_tcq.h~blk_queue_free_tags_bug2 +8 -0
diff -puN include/scsi/scsi_tcq.h~blk_queue_free_tags_bug2 include/scsi/scsi_tcq.h
@@ -25,9 +25,13 @@
  **/
 static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
 {
+        unsigned long flags;
+
         if (sdev->tagged_supported) {
+		spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 		if (!blk_queue_tagged(sdev->request_queue))
 			blk_queue_init_tags(sdev->request_queue, depth, NULL);
+		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, depth);
         }
 }

@@ -38,8 +42,12 @@ static inline void scsi_activate_tcq(str
  **/
 static inline void scsi_deactivate_tcq(struct scsi_device *sdev, int depth)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	if (blk_queue_tagged(sdev->request_queue))
 		blk_queue_free_tags(sdev->request_queue);
+	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 	scsi_adjust_queue_depth(sdev, 0, depth);
 }

_

author
#3 lore raw

Oops. I guess this was the actual patch I sent.

Quoted 24 lines · Brian King
Quoted 7 lines · James Bottomley
Quoted 2 lines · Brian King

Add locking to scsi_activate_tcq and scsi_deactivate_tcq to fix a race condition that can occur when disabling tcqing with commands in flight.

Quoted 5 lines

It's possible to do it like this. However, we should really quiesce the SCSI device before disabling tcq. It is not legal in the scsi spec to have both tagged and untagged commands outstanding. Most drives do the right thing and return BUSY to an untagged request if they have tags in flight.

Ok. I agree that we do not want both tagged and untagged requests going to the device. In the case of the ipr driver, the adapter microcode handles this for me. If there are tagged requests outstanding to a device and an untagged request is sent, the tagged requests must first finish, then the untagged request is sent to the device with no other requests outstanding.

Quoted 3 lines

Also, we have to wait until all tags return before freeing the block layer tag structures...and there's always the devices that do strange things in this situation...

I sent the following patch to Jens last week as well and he applied it. This fixes the free problem you mention.

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center

Currently blk_queue_free_tags cannot be called with ops outstanding. The scsi_tcq API defined to LLD scsi drivers allows for scsi_deactivate_tcq to be called (which calls blk_queue_free_tags) with ops outstanding. Change blk_queue_free_tags to no longer free the tags, but rather just disable tagged queuing and also modify blk_queue_init_tags to handle re-enabling tagged queuing after it has been disabled.

Signed-off-by Brian King <[email protected]>

---

 linux-2.6.8-rc2-bjking1/drivers/block/ll_rw_blk.c |   35 +++++++++++++++++-----
 1 files changed, 28 insertions(+), 7 deletions(-)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~blk_queue_free_tags +28 -7
diff -puN drivers/block/ll_rw_blk.c~blk_queue_free_tags drivers/block/ll_rw_blk.c
@@ -482,15 +482,14 @@ struct request *blk_queue_find_tag(reque
 EXPORT_SYMBOL(blk_queue_find_tag);
 
 /**
- * blk_queue_free_tags - release tag maintenance info
+ * _blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
  *
  *  Notes:
  *    blk_cleanup_queue() will take care of calling this function, if tagging
- *    has been used. So there's usually no need to call this directly, unless
- *    tagging is just being disabled but the queue remains in function.
+ *    has been used. So there's no need to call this directly.
  **/
-void blk_queue_free_tags(request_queue_t *q)
+static void _blk_queue_free_tags(request_queue_t *q)
 {
 	struct blk_queue_tag *bqt = q->queue_tags;
 

@@ -514,6 +513,19 @@ void blk_queue_free_tags(request_queue_t
 	q->queue_flags &= ~(1 << QUEUE_FLAG_QUEUED);
 }
 
+/**
+ * blk_queue_free_tags - release tag maintenance info
+ * @q:  the request queue for the device
+ *
+ *  Notes:
+ *	This is used to disabled tagged queuing to a device, yet leave
+ *	queue in function.
+ **/
+void blk_queue_free_tags(request_queue_t *q)
+{
+	clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+}
+
 EXPORT_SYMBOL(blk_queue_free_tags);
 
 static int

@@ -564,13 +576,22 @@ fail:
 int blk_queue_init_tags(request_queue_t *q, int depth,
 			struct blk_queue_tag *tags)
 {
-	if (!tags) {
+	int rc;
+
+	BUG_ON(tags && q->queue_tags && tags != q->queue_tags);
+
+	if (!tags && !q->queue_tags) {
 		tags = kmalloc(sizeof(struct blk_queue_tag), GFP_ATOMIC);
 		if (!tags)
 			goto fail;
 
 		if (init_tag_map(q, tags, depth))
 			goto fail;
+	} else if (q->queue_tags) {
+		if ((rc = blk_queue_resize_tags(q, depth)))
+			return rc;
+		set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+		return 0;
 	} else
 		atomic_inc(&tags->refcnt);
 

@@ -1333,8 +1354,8 @@ void blk_cleanup_queue(request_queue_t *
 	if (rl->rq_pool)
 		mempool_destroy(rl->rq_pool);
 
-	if (blk_queue_tagged(q))
-		blk_queue_free_tags(q);
+	if (q->queue_tags)
+		_blk_queue_free_tags(q);
 
 	kmem_cache_free(requestq_cachep, q);
 }

_

author
#4 lore raw
In reply to Brian King

James,

Any chance of applying this patch? I think most of the issues you raised I've answered. This patch certainly does not preclude other LLD's from quiescing the device before changing the tcqing state if they need to.

Or would you prefer adding code to the scsi core to quiesce the device before changing its tcqing state? The scsi core could feasibly quiesce the device, enable or disable tagged queuing, then resume the queue. I haven't looked at generating a patch for this yet, so I'm not sure how simple it would be to do what I am suggesting...

Thanks

-Brian

Quoted 9 lines · Brian King
Quoted 7 lines · James Bottomley
Quoted 2 lines · Brian King

Add locking to scsi_activate_tcq and scsi_deactivate_tcq to fix a race condition that can occur when disabling tcqing with commands in flight.

Quoted 13 lines
Quoted 5 lines

It's possible to do it like this. However, we should really quiesce the SCSI device before disabling tcq. It is not legal in the scsi spec to have both tagged and untagged commands outstanding. Most drives do the right thing and return BUSY to an untagged request if they have tags in flight.

Ok. I agree that we do not want both tagged and untagged requests going to the device. In the case of the ipr driver, the adapter microcode handles this for me. If there are tagged requests outstanding to a device and an untagged request is sent, the tagged requests must first finish, then the untagged request is sent to the device with no other requests outstanding.

Quoted 3 lines

Also, we have to wait until all tags return before freeing the block layer tag structures...and there's always the devices that do strange things in this situation...

The block layer now handles this appropriately and does not free the tags until blk_cleanup_queue is called.

Quoted 31 lines

diff -puN include/scsi/scsi_tcq.h~blk_queue_free_tags_bug2 include/scsi/scsi_tcq.h

include/scsi/scsi_tcq.h renamed from include/scsi/scsi_tcq.h~blk_queue_free_tags_bug2 +8 -0
@@ -25,9 +25,13 @@
  **/
 static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
 {
+        unsigned long flags;
+
         if (sdev->tagged_supported) {
+		spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 		if (!blk_queue_tagged(sdev->request_queue))
 			blk_queue_init_tags(sdev->request_queue, depth, NULL);
+		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, depth);
         }
 }

@@ -38,8 +42,12 @@ static inline void scsi_activate_tcq(str
  **/
 static inline void scsi_deactivate_tcq(struct scsi_device *sdev, int depth)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	if (blk_queue_tagged(sdev->request_queue))
 		blk_queue_free_tags(sdev->request_queue);
+	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 	scsi_adjust_queue_depth(sdev, 0, depth);
 }

_

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center