blk-tag.c: 89 BUG() triggering + initial analysis

Hi All,

I hope linux-scsi is the right list for this, if not let me know.

I've been working on getting the uas (Usb Attached Scsi) driver into working shape for the last 3 weeks, so that it can be enabled in 3.14 .

My latest tests where performed on top of 3.11 + a bunch of xhci and of course uas fixes.

I can reliable trigger the BUG() in blk-tag.c line 89:

void blk_free_tags(struct blk_queue_tag *bqt)

{
        if (unlikely(!__blk_free_tags(bqt)))
                BUG();
}

I believe this is not an uas driver bug, but rather a bug which any scsi host which uses scsi_init_shared_tag_map() can trigger, which is likely not seen before because almost no hosts actually use scsi_init_shared_tag_map().

The above test triggering the BUG() assumes that blk_free_tags() caller holds the last reference to the bqt. For scsi hosts using scsi_init_shared_tag_map() this assumes that the release of the block_queue through blk-sysfs.c: blk_release_queue() happens before the release of the host through scsi/hosts.c: scsi_host_dev_release()

I've added some strategic debug printk-s to debug this problem (and removed the BUG()) and in some cases this is not true.

Here is the output of my debug scripts on a normal unplug of the uas usb-device:

[ 7678.202540] blk-sysfs.c: blk_release_queue queue_tags ffff88022d4d59e0
[ 7678.202551] blk-tag.c: __blk_queue_free_tags bqt ffff88022d4d59e0
[ 7678.202553] blk-tag.c: __blk_free_tags refcnt before dec: 2
[ 7678.202626] scsi/hosts.c: scsi_host_dev_release bqt ffff88022d4d59e0
[ 7678.202654] blk-tag.c: blk_free_tags bqt ffff88022d4d59e0
[ 7678.202655] blk-tag.c: __blk_free_tags refcnt before dec: 1
[ 7678.202657] blk-tag.c: __blk_free_tags free-ed: ffff88022d4d59e0

Which does not trigger the BUG().

If however I do the following:
1) plug in uas usb-device
2) let udisks auto-mount it under:
/run/media/hans/4e82585c-3c40-48ac-81ad-11d2a7bad0fc
3) cd into that dir to keep it busy
4) unplug
5) cd out of the directory, at which points udisks will umount it

Then with an unpatched kernel I hit the BUG() at step 5, and with a kernel with the BUG() removed I get the following debug trace:

[ 9089.808021] scsi/hosts.c: scsi_host_dev_release bqt ffff88022c02ae40
[ 9089.808040] blk-tag.c: blk_free_tags bqt ffff88022c02ae40
[ 9089.808041] blk-tag.c: __blk_free_tags refcnt before dec: 2
[ 9089.808046] blk-sysfs.c: blk_release_queue queue_tags ffff88022c02ae40
[ 9089.808057] blk-tag.c: __blk_queue_free_tags bqt ffff88022c02ae40
[ 9089.808058] blk-tag.c: __blk_free_tags refcnt before dec: 1
[ 9089.808059] blk-tag.c: __blk_free_tags free-ed: ffff88022c02ae40

Notice how in this case scsi_host_dev_release() runs before blk_release_queue(), breaking the assumption the BUG() tests for.

I think this may be caused by userspace holding a reference to the kobj which has blk_release_queue as release callback when doing the umount. But I simply don't know the code in question well enough to do a more detailed analysis of the problem.

A naive fix, which seems to work, would be to simply remove the BUG() but I'm not sure if that is the right solution...

Regards,

Hans

Um, well, no, stex, qla4xxx and fnic do. Of those, I think fnic and qla4xxx have quite large install bases.

The devices all hold parent references to the host. The last action of a device release is a put on its parent (this is after the last put of the device queue). The host release function shouldn't be called until every device has released its reference ... unless something has gone wrong with the device model, of course.

So this looks like an induced failure caused by the filesystem holding a queue reference which you force by holding the directory, meaning that the blk_queue_put in the sdev release function isn't the last put. The fix for this is probably to release the tags once the queue is dead and can accept no I/O. That would be this, does that fix it?

This isn't the whole fix, we can likely now remove the queue free from last put as well ... I'm just not sure if there's any block devices that don't call blk_cleanup_queue, which keeping it in would catch.

James

---

index 8bdd012..03c0bf9 100644
@@ -506,6 +506,8 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
+	__blk_queue_free_tags(q);
+
 	spin_lock_irq(lock);
 	if (q->queue_lock != &q->__queue_lock)
 		q->queue_lock = &q->__queue_lock;