On Tue, 2013-03-05 at 19:17 +0200, Kai Makisara wrote: > On Tue, 5 Mar 2013, Joe Lawrence wrote: > > > Changes from v1: > > Corrected error paths as noted by Ewan Milne and Jan Vesely. > > > > These changes were applied to scsi.git, branch "misc". This patch > > fixes a reference count bug in the SCSI tape driver which can be > > reproduced with the following: > > > > * Boot with slub_debug=FZPU, tape drive attached > > * echo 1 > /sys/devices/... tape device pci path .../remove > > * Wait for device removal > > * echo 1 > /sys/kernel/slab/blkdev_queue/validate > > * Slub debug complains about corrupted poison pattern > > > > In commit 523e1d39 (block: make gendisk hold a reference to its queue) > > add_disk() and disk_release() were modified to get/put an additional > > reference on a disk queue to fix a reference counting discrepency > > between bdev release and SCSI device removal. The ST driver never > > calls add_disk(), so this commit introduced an extra kref put when the > > ST driver frees its struct gendisk. > > > > Attempts were made to fix this bug at the block level [1] but later > > abandoned due to floppy driver issues [2]. > > > > [1] https://lkml.org/lkml/2012/8/27/354 > > [2] https://lkml.org/lkml/2012/9/22/113 > > > > From a50a6ee28748b7c1620af6f76772164ec0fc4a1d Mon Sep 17 00:00:00 2001 > > From: Joe Lawrence <joe.lawrence@xxxxxxxxxxx> > > Date: Tue, 5 Mar 2013 09:30:14 -0500 > > Subject: [PATCH v2] st: Take additional queue ref in st_probe > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > The SCSI tape driver employs a struct gendisk, calling alloc_disk() to > > create an instance, but does not register it via add_disk(). When the > > gendisk is torn down, disk_release() is called and expects to return a > > disk queue reference that add_disk() normally would have taken out. (See > > commit 523e1d39.) Fix the kref accounting by adding a blk_get_queue() > > to st_probe(). > > > > Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxxx> > > Tested-by: Ewan D. Milne <emilne@xxxxxxxxxx> > > I don't have to like this fix but as it fixes a real problem: > > Acked-by: Kai Mäkisara <Kai.Makisara@xxxxxxxxxxx> > > (It should not be necessary to get additional references outside the block > layer to make the block layer behave.) Agree with that. However, from a practical point of view, alloc_disk() has no idea what disk queue it's allocating the minors for, so on the information we provide to block, there's no disconnect. On the other hand, the other consequence of this situation is that there's no current sysfs way of getting at the queue parameters because the gendisk isn't added. We actually need some type of add_disk_not_really_a_disk() Call which would tell block about the queue associated with the disk and ask to to add the relevant non-disk sysfs parameters. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
