uas: kernel BUG at block/blk-tag.c:89 is back again :|

Hi Hans,

please test the path below:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Signed-off-by Christoph Hellwig <[email protected]>

block/blk-tag.c +7 -26

index 3f33d86..a185b86 100644
@@ -27,18 +27,15 @@ struct request *blk_queue_find_tag(struct request_queue *q, int tag)
 EXPORT_SYMBOL(blk_queue_find_tag);
 
 /**
- * __blk_free_tags - release a given set of tag maintenance info
+ * blk_free_tags - release a given set of tag maintenance info
  * @bqt:	the tag map to free
  *
- * Tries to free the specified @bqt.  Returns true if it was
- * actually freed and false if there are still references using it
+ * Drop the reference count on @bqt and frees it when the last reference
+ * is dropped.
  */
-static int __blk_free_tags(struct blk_queue_tag *bqt)
+void blk_free_tags(struct blk_queue_tag *bqt)
 {
-	int retval;
-
-	retval = atomic_dec_and_test(&bqt->refcnt);
-	if (retval) {
+	if (atomic_dec_and_test(&bqt->refcnt)) {
 		BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
 							bqt->max_depth);
 

@@ -50,9 +47,8 @@ static int __blk_free_tags(struct blk_queue_tag *bqt)
 
 		kfree(bqt);
 	}
-
-	return retval;
 }
+EXPORT_SYMBOL(blk_free_tags);
 
 /**
  * __blk_queue_free_tags - release tag maintenance info

@@ -69,28 +65,13 @@ void __blk_queue_free_tags(struct request_queue *q)
 	if (!bqt)
 		return;
 
-	__blk_free_tags(bqt);
+	blk_free_tags(bqt);
 
 	q->queue_tags = NULL;
 	queue_flag_clear_unlocked(QUEUE_FLAG_QUEUED, q);
 }
 
 /**
- * blk_free_tags - release a given set of tag maintenance info
- * @bqt:	the tag map to free
- *
- * For externally managed @bqt frees the map.  Callers of this
- * function must guarantee to have released all the queues that
- * might have been using this tag map.
- */
-void blk_free_tags(struct blk_queue_tag *bqt)
-{
-	if (unlikely(!__blk_free_tags(bqt)))
-		BUG();
-}
-EXPORT_SYMBOL(blk_free_tags);
-
-/**
  * blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
  *

Hi,

Quoted 3 lines · Christoph Hellwig

Hi Hans,

please test the path below:

Note, I did a similar patch a while back, but then it was decided to try and fix the tear-down ordering instead (which seems to have not completely fixed the issue).

Quoted 10 lines · Hans de Goede

Hi,

Quoted 3 lines · Christoph Hellwig

Hi Hans,

please test the path below:

Note, I did a similar patch a while back, but then it was decided to try and fix the tear-down ordering instead (which seems to have not completely fixed the issue).

I can't see any sensible way to "fix" the issue. Struct gendisk keeps a reference to the requeue_queue, and a struct gendisk reference is held as long as the block device is open. So whenever you surprise remove a device that is open (usually by having a filesystem mounted on it), the request_queue and it's reference to the tag structure will vastly outlive the Scsi_Host.

Quoted 6 lines

I can't see any sensible way to "fix" the issue. Struct gendisk keeps a reference to the requeue_queue, and a struct gendisk reference is held as long as the block device is open. So whenever you surprise remove a device that is open (usually by having a filesystem mounted on it), the request_queue and it's reference to the tag structure will vastly outlive the Scsi_Host.

Ok, so can we then please move forward with getting your patch merged ?

Quoted 7 lines · Hans de Goede

Quoted 6 lines

I can't see any sensible way to "fix" the issue. Struct gendisk keeps a reference to the requeue_queue, and a struct gendisk reference is held as long as the block device is open. So whenever you surprise remove a device that is open (usually by having a filesystem mounted on it), the request_queue and it's reference to the tag structure will vastly outlive the Scsi_Host.

Quoted 1 line

Ok, so can we then please move forward with getting your patch merged ?

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Quoted 12 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

Quoted 5 lines

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Thanks, applied. I don't think I was ever CC'ed on this one, though.

Hi,

Quoted 13 lines · Jens Axboe

Quoted 12 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

Quoted 7 lines

Quoted 5 lines

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Thanks, applied. I don't think I was ever CC'ed on this one, though.

Thanks, good to hear.

Can you please add a Cc: stable ? This patch should at least be backported to 3.15, where uas support has been declared stable.

With uas it is trivial to trigger the BUG_ON (and break any further device hotplugging) since uas allows for tcq, and thus using tags over usb (with all of its hotplug goodness and badness).

Hi,

Quoted 19 lines · Jens Axboe

Quoted 17 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Quoted 1 line

Thanks, applied. I don't think I was ever CC'ed on this one, though.

Thanks, good to hear.

Hi,

Quoted 22 lines · Hans de Goede

Hi,

Quoted 19 lines · Jens Axboe

Quoted 17 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Quoted 3 lines

Quoted 1 line

Thanks, applied. I don't think I was ever CC'ed on this one, though.

Thanks, good to hear.

It seems this one has not made it into 3.16-rc6 somehow ?

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Hans de Goede

Hi,

Quoted 19 lines · Jens Axboe

Quoted 17 lines · Christoph Hellwig

Quoted 4 lines · Jens Axboe

Quoted 2 lines

I've posted the patch in reply to another report and added Jens to the Cc list. As the block maintainer he need to review and merge it.

Which patch is this?

This one:

---
From: Christoph Hellwig <[email protected]>
Subject: block: don't assume last put of shared tags is for the host

There is no inherent reason why the last put of a tag structure must be the one for the Scsi_Host, as device model objects can be held for arbitrary periods. Merge blk_free_tags and __blk_free_tags into a single funtion that just release a references and get rid of the BUG() when the host reference wasn't the last.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Thanks, applied. I don't think I was ever CC'ed on this one, though.

Thanks, good to hear.

It seems this one has not made it into 3.16-rc6 somehow ?

It'll go out shortly, was on vacation last week. It is queued up in for-linus.