On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <michaelc@xxxxxxxxxxx> wrote: > FUJITA Tomonori wrote: > > On Wed, 26 Mar 2008 07:36:26 -0700 > > James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > >> On Wed, 2008-03-26 at 23:22 +0900, FUJITA Tomonori wrote: > >>> On Sat, 22 Mar 2008 11:06:00 -0500 > >>> James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > >>> > >>>> On Tue, 2008-03-11 at 00:36 -0500, Mike Christie wrote: > >>>>> Mike Christie wrote: > >>>>>> Pete Wyckoff wrote: > >>>>>>> I think this used not to happen; not sure. But I changed two things > >>>>>> This most likely did not happen before 2.6.25-rc* or it broke in > >>>>>> slightly different ways, because iscsi used to try and do > >>>>>> > >>>>>> echo 1 > /sys/block/sdX/device/delete > >>>>>> > >>>>>> from userspace instead of calling scsi_remove_target from the kernel. > >>>>>> > >>>>>> As you know around 2.6.21, the behavior of doing the echo to the delete > >>>>>> file changed due to a driver model and scsi change and that broke the > >>>>>> iscsi tools. The iscsi tools userspace removal was sort of hack in the > >>>>>> first place and was racey, so we switched to removing devices/target > >>>>>> like the FC class. > >>>>>> > >>>>>> > >>>>>>> lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to > >>>>>>> fedora devel (868). Bidi and varlen patches always too. > >>>>>>> > >>>>>>> I'll follow with some more variations on this theme. Looks like bsg > >>>>>>> needs to protect more carefully against the device going away. Any > >>>>>>> ideas how best to do this? What was the approach in sg? > >>>>>>> > >>>>>> I think sg is broken in similar ways. The iser guys have some tests > >>>>>> cases that have broken sg while IO is outstanding. I am ccing Erez. > >>>>> Actually one of the problems looks a little different than some of the > >>>>> problems hit with sg and are caused because we remove the bsg device too > >>>>> soon. I think we want to wait until all the references from the > >>>>> commands/requests are released. The attached patch (untested) moves the > >>>>> bsg unreg call to the scsi device release fn. > >>>> Well, this fix is now upstream. However, it's causing all our > >>>> scsi_devices never to get released, which is a serious regression. > >>>> We're also doing spurious bsg_unregister_queue() for things that never > >>>> actually registered one (all scan devices that return DID_NO_CONNECT), > >>>> but bsg doesn't seem to be complaining about this. > >>>> > >>>> The essence of the problem is that bsg_register_queue() takes a ref to > >>>> the sdev_gendev, so you can't move bsg_unregister_queue() into the > >>>> release function because nothing ever puts bsg's device ref and so > >>>> release is never called. > >>>> > >>>> Options for fixing this before 2.6.25 are > >>>> > >>>> 1. revert the patch > >>>> 2. Do an additional put for the bsg reference in > >>>> __scsi_remove_device (patch below). It's nasty but it preserves > >>>> the semantics and does what you want > >>> After some investigation, this patch doesn't fix the bug that Pete > >>> reported (I'll send a new patch shortly). > >>> > >>> Can you revert the commit 4b6f5b3a993cbe34b4280f252bccc76967c185c8 > >>> instead of merging this? > >> Sure ... I didn't like the hack either. As long as iSCSI is fine with > >> the reversion it's the quickest way to fix the problem. > > > > How about this? With the commit reversion, I confirmed that this patch > > fixes the first bug that Pete reported: > > > > http://marc.info/?l=linux-scsi&m=120508166505141&w=2 > > > > I suspect that this could fix the rest too. > > > > = > > From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> > > Subject: [PATCH] bsg: takes a ref to struct device in fops->open > > > > bsg_register_queue() takes a ref to struct device that a caller > > passes. For example, it takes a ref to the sdev_gendev with scsi > > devices. However, bsg doesn't takes a ref to it in fops->open. So > > while an application opens a bsg device, the scsi device that the bsg > > device holds can go away (bsg also takes a ref to a queue, but it > > doesn't prevent the device from going away). > > > > With this, bsg takes a ref to struct device in fops->open and frees it > > in fops->release. > > > > Note that bsg doesn't need to takes a ref to a queue for SCSI devices > > at least. I think that it would be better to remove the code but I let > > it alone for now. > > > > Why does bsg_add_device do kobject_get instead of blk_get_queue? I think that it's a bug. But both takes a ref to a queue (though kobject_get doesn't see QUEUE_FLAG_DEAD), so I think that it's not related with the current problems. > It seems like if we added a blk_qet_queue when we opened the device and > a blk_put_queue when bsg_release is called we could remove the > get/put_device calls. I am not sure if that is cleaner or not. I was > just thinking that bsg goes from bsg->request_queue->scsi_device so > maybe it should not worry about the device. kobject_get takes a ref to a queue. If we don't take a ref to a device, the scsi device has gone though the queue is still there because the queue release is done from the device release. If the scsi device has gone, we are dead, right? Anyway, here's a patch to replace kobject_get with blk_get_queue. James, please apply this patch too. = From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue Both takes a ref to a queue. But the former checks QUEUE_FLAG_DEAD and is more appropriate interface here. Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> Cc: Jens Axboe <jens.axboe@xxxxxxxxxx> Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> --- block/bsg.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/block/bsg.c b/block/bsg.c index a348a79..b07efd3 100644 --- a/block/bsg.c +++ b/block/bsg.c @@ -740,16 +740,21 @@ static struct bsg_device *bsg_add_device(struct inode *inode, struct file *file) { struct bsg_device *bd; + int ret; #ifdef BSG_DEBUG unsigned char buf[32]; #endif + ret = blk_get_queue(rq); + if (ret) + return -ENXIO; bd = bsg_alloc_device(); - if (!bd) + if (!bd) { + blk_put_queue(rq); return ERR_PTR(-ENOMEM); + } bd->queue = rq; - kobject_get(&rq->kobj); bsg_set_block(bd, file); atomic_set(&bd->ref_count, 1); -- 1.5.3.6 -- 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
