[BUG 1/3] bsg queue oops with iscsi logout

[BUG 1/3] bsg queue oops with iscsi logout

[Pete Wyckoff]

Here's an oops.  Mount a target with iscsi.  Start a process that
holds open the bsg device, in the debugger perhaps.  Then use
iscsiadm to logout.  Let the bsg process try to submit another
command.  Its queue is no longer usable, as blk_get_request() finds
a NULL q->elevator->ops.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000070
IP: [<ffffffff802e6adb>] elv_may_queue+0xb/0x20
PGD 3e8e1067 PUD 3e4ce067 PMD 0 
Oops: 0000 [1] SMP 
CPU 1 
Modules linked in: crc32c libcrc32c rdma_ucm rdma_cm iw_cm ib_addr ib_ipoib ib_ucm ib_cm ib_sa ib_umad ib_uverbs ib_mthca iscsi_tcp libiscsi scsi_transport_iscsi ext3 jbd ib_mad ib_core i2c_nforce2 i2c_core sg sd_mod sata_nv tg3 nfs lockd sunrpc
Pid: 3000, comm: sgio Not tainted 2.6.25-rc4-bidi-pw #29
RIP: 0010:[<ffffffff802e6adb>]  [<ffffffff802e6adb>] elv_may_queue+0xb/0x20
RSP: 0018:ffff81007d15fd28  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff81007d54d468 RCX: 0000000000000010
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff81007d54d468
RBP: ffff81007d15fd28 R08: 0000000000000001 R09: ffff81007d093e88
R10: 0000000000000000 R11: 0000000000000000 R12: ffff81007d54d468
R13: ffff81007d54d488 R14: 0000000000000000 R15: 0000000000000000
FS:  00002b05a4a17b00(0000) GS:ffff81007f600840(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000070 CR3: 000000003e497000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
Process sgio (pid: 3000, threadinfo ffff81007d15e000, task ffff81007f712830)
Stack:  ffff81007d15fd98 ffffffff802ea129 ffff810040005358 ffff810040005350
 00000010000612d0 0000000000000000 ffff81007cda6a68 ffff81007d9de020
 0000000000000086 0000000000000000 ffff81007d54d468 0000000000000000
Call Trace:
 [<ffffffff802ea129>] get_request+0x39/0x330
 [<ffffffff802ea44f>] get_request_wait+0x2f/0x1c0
 [<ffffffff80286b27>] ? cache_grow+0x1b7/0x260
 [<ffffffff802ea7d8>] blk_get_request+0x38/0x80
 [<ffffffff802f171c>] bsg_map_hdr+0x9c/0x340
 [<ffffffff802f1aac>] bsg_write+0xec/0x2e0
 [<ffffffff8028d4a7>] vfs_write+0xc7/0x150
 [<ffffffff8028db10>] sys_write+0x50/0x90
 [<ffffffff8020b58b>] system_call_after_swapgs+0x7b/0x80


Code: 55 48 8b 47 18 48 89 e5 48 8b 00 48 8b 40 68 48 85 c0 74 05 48 89 f7 ff d0 c9 c3 0f 1f 44 00 00 48 8b 47 18 55 48 89 e5 48 8b 00 <48> 8b 50 70 31 c0 48 85 d2 74 02 ff d2 c9 c3 66 0f 1f 44 00 00 
RIP  [<ffffffff802e6adb>] elv_may_queue+0xb/0x20
 RSP <ffff81007d15fd28>
CR2: 0000000000000070
---[ end trace a020f2a7368afcb4 ]---

I think this used not to happen; not sure.  But I changed two things
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?

		-- Pete

[BUG 2/3] bsg null sdev with iscsi logout

[Pete Wyckoff]

Here's a different oops that may happen when the target goes away
unexpectedly.  Mount a slow target with iscsi.  Start a process that
uses bsg to issue an oustanding command, kill off the target before it
can respond (or unplug the network), but do not ctrl-C the bsg process.

In another shell, use iscsiadm to logout.  This provokes a kref
complaint and a bug.  The bug looks like the bsg app has seen completion
in error of its hung command, issues andother command, and ends up with
a NULL sdev later in the SCSI processing.

WARNING: at lib/kref.c:43 kref_get+0x2d/0x30()
Modules linked in: crc32c libcrc32c rdma_ucm rdma_cm iw_cm ib_addr ib_ipoib ib_ucm ib_cm ib_sa ib_umad ib_uverbs ib_mthca iscsi_tcp libiscsi scsi_transport_iscsi ext3 jbd ib_mad sg ib_core sd_mod i2c_nforce2 i2c_core sata_nv tg3 nfs lockd sunrpc
Pid: 3045, comm: sgio Not tainted 2.6.25-rc4-bidi-pw #29

Call Trace:
 [<ffffffff8022f82f>] warn_on_slowpath+0x5f/0x80
 [<ffffffff802ea243>] ? get_request+0x153/0x330
 [<ffffffff80247d26>] ? hrtimer_start+0xd6/0x150
 [<ffffffff80239b96>] ? lock_timer_base+0x36/0x70
 [<ffffffff802fc07d>] kref_get+0x2d/0x30
 [<ffffffff802fafea>] kobject_get+0x1a/0x30
 [<ffffffff803559f7>] get_device+0x17/0x20
 [<ffffffff80367357>] scsi_request_fn+0x37/0x3b0
 [<ffffffff802e9d94>] __generic_unplug_device+0x24/0x30
 [<ffffffff802ece63>] blk_execute_rq_nowait+0x63/0x90
 [<ffffffff802f1b48>] bsg_write+0x188/0x2e0
 [<ffffffff8028d4a7>] vfs_write+0xc7/0x150
 [<ffffffff8028db10>] sys_write+0x50/0x90
 [<ffffffff8020b58b>] system_call_after_swapgs+0x7b/0x80

---[ end trace dbc99ed69e02749c ]---
BUG: unable to handle kernel NULL pointer dereference at 0000000000000420
IP: [<ffffffff8036513c>] scsi_prep_state_check+0xc/0xb0
PGD 3d111067 PUD 3e9b3067 PMD 0 
Oops: 0000 [1] SMP 
CPU 0 
Modules linked in: crc32c libcrc32c rdma_ucm rdma_cm iw_cm ib_addr ib_ipoib ib_ucm ib_cm ib_sa ib_umad ib_uverbs ib_mthca iscsi_tcp libiscsi scsi_transport_iscsi ext3 jbd ib_mad sg ib_core sd_mod i2c_nforce2 i2c_core sata_nv tg3 nfs lockd sunrpc
Pid: 3045, comm: sgio Not tainted 2.6.25-rc4-bidi-pw #29
RIP: 0010:[<ffffffff8036513c>]  [<ffffffff8036513c>] scsi_prep_state_check+0xc/0xb0
RSP: 0018:ffff81007f5dfd68  EFLAGS: 00010092
RAX: ffffffff80366150 RBX: 0000000000000000 RCX: 0000000000000001
RDX: 0000000000000001 RSI: ffff81003fdcd3e0 RDI: 0000000000000000
RBP: ffff81007f5dfd78 R08: 0000000000000000 R09: 00000000ffffffff
R10: 0000000000000000 R11: 0000000000000020 R12: 0000000000000000
R13: ffff81007e4ed800 R14: ffff81007c8b94e8 R15: 0000000000000001
FS:  00007f34d7ad96f0(0000) GS:ffffffff80515000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000420 CR3: 000000003d0e0000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process sgio (pid: 3045, threadinfo ffff81007f5de000, task ffff81007e4977b0)
Stack:  0000000100000000 ffff81003fdcd3e0 ffff81007f5dfda8 ffffffff80365fd8
 ffff81007f5dfdb8 ffff81003fdcd3e0 ffff81007c8b94e8 ffff81007e4ed800
 ffff81007f5dfdc8 ffffffff80366195 ffff81007c8b94e8 ffff81003fdcd3e0
Call Trace:
 [<ffffffff80365fd8>] scsi_setup_blk_pc_cmnd+0x18/0x190
 [<ffffffff80366195>] scsi_prep_fn+0x45/0x50
 [<ffffffff802e7809>] elv_next_request+0xc9/0x280
 [<ffffffff802fafea>] ? kobject_get+0x1a/0x30
 [<ffffffff80367529>] scsi_request_fn+0x209/0x3b0
 [<ffffffff802e9d94>] __generic_unplug_device+0x24/0x30
 [<ffffffff802ece63>] blk_execute_rq_nowait+0x63/0x90
 [<ffffffff802f1b48>] bsg_write+0x188/0x2e0
 [<ffffffff8028d4a7>] vfs_write+0xc7/0x150
 [<ffffffff8028db10>] sys_write+0x50/0x90
 [<ffffffff8020b58b>] system_call_after_swapgs+0x7b/0x80


Code: 0a 00 4c 89 f7 48 89 45 d0 e8 a1 a4 ff ff eb ab 66 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 89 fb 48 83 ec 08 <8b> 87 20 04 00 00 83 f8 02 75 09 31 c0 48 83 c4 08 5b c9 c3 83 
RIP  [<ffffffff8036513c>] scsi_prep_state_check+0xc/0xb0
 RSP <ffff81007f5dfd68>
CR2: 0000000000000420
---[ end trace dbc99ed69e02749c ]---

Same setup as the bug 1/3.

		-- Pete

[BUG 3/3] bsg mutex hang with iscsi logout

[Pete Wyckoff]

Here's an interesting hang.  Mount a target with iscsi, again one that
may be slow in responding.  Send it a command via bsg.  Kill the target
or unplug the network before the response returns.  At this point the
kernel notices and iscsid goes about trying to reconnect.

Hit ctrl-c to try to kill the bsg-using application.  Being the last
user of the device, it hangs during file close waiting for the
outstanding command to complete (even in error), sleeping with bsg_mutex
held.

sgio          D ffff81007c567bc8     0  2916   2846
 ffff81007c567b98 0000000000000046 ffff81007c567b78 ffff810040e16cc8
 ffff81007f721080 ffff81007f71f830 ffff81007f721298 ffffffff80263514
 ffff81007c567b98 0000000000000282 ffff81007c567c28 ffffffff80267238
Call Trace:
 [<ffffffff80263514>] ? __pagevec_free+0x24/0x30
 [<ffffffff80267238>] ? release_pages+0x198/0x1e0
 [<ffffffff80404ce8>] io_schedule+0x28/0x40
 [<ffffffff802f1520>] bsg_release+0x1c0/0x1e0
 [<ffffffff80244ad0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff8028ded3>] __fput+0xb3/0x1a0
 [<ffffffff8028e1d6>] fput+0x16/0x20
 [<ffffffff8028afab>] filp_close+0x4b/0x80
 [<ffffffff80232171>] put_files_struct+0xc1/0xd0
 [<ffffffff802321c5>] __exit_files+0x45/0x50
 [<ffffffff80233592>] do_exit+0x1a2/0x7a0
 [<ffffffff80233bc7>] do_group_exit+0x37/0xa0
 [<ffffffff8023ccf8>] get_signal_to_deliver+0x268/0x330
 [<ffffffff8020b614>] ? sysret_signal+0x1c/0x27
 [<ffffffff8020a814>] do_notify_resume+0xd4/0x950
 [<ffffffff80244ba0>] ? finish_wait+0x60/0x80
 [<ffffffff802f112a>] ? bsg_get_done_cmd+0x11a/0x140
 [<ffffffff802f1f8a>] ? bsg_read+0xda/0x180
 [<ffffffff8028d5f4>] ? vfs_read+0xc4/0x150
 [<ffffffff8020b614>] ? sysret_signal+0x1c/0x27
 [<ffffffff8020b8a7>] ptregscall_common+0x67/0xb0

Meanwhile, in another shell, use iscsiadm to logout from the target.  As
scsi removes the device, it tells bsg to unregister the queue that is
going away, and to do that, it needs the bsg_mutex.  You have to be
fairly quick about it to get into this situation.

iscsi_scan_4  D ffffffff804ff688     0  2873      2
 ffff81007c421cd0 0000000000000046 000001a800000002 fffffffffffffffc
 ffff81007c1690c0 ffff81007e4c8830 ffff81007c1692d8 0000000000015d6d
 0000015400000002 fffffffffffffffc 0000000000015e4c 0000016400000002
Call Trace:
 [<ffffffff804052ff>] __mutex_lock_slowpath+0x7f/0xd0
 [<ffffffff804050be>] mutex_lock+0xe/0x10
 [<ffffffff802f1d81>] bsg_unregister_queue+0x31/0xa0
 [<ffffffff8036b300>] __scsi_remove_device+0x40/0xa0
 [<ffffffff8036b38b>] scsi_remove_device+0x2b/0x40
 [<ffffffff8036b43c>] __scsi_remove_target+0x9c/0xe0
 [<ffffffff8036b4e0>] ? __remove_child+0x0/0x30
 [<ffffffff8036b4fe>] __remove_child+0x1e/0x30
 [<ffffffff80355f93>] device_for_each_child+0x33/0x60
 [<ffffffff802fafea>] ? kobject_get+0x1a/0x30
 [<ffffffff8036b4ce>] scsi_remove_target+0x4e/0x60
 [<ffffffff880f1d15>] :scsi_transport_iscsi:__iscsi_unbind_session+0x85/0xa0
 [<ffffffff880f1c90>] ? :scsi_transport_iscsi:__iscsi_unbind_session+0x0/0xa0
 [<ffffffff80240c64>] run_workqueue+0x84/0x110
 [<ffffffff80241713>] worker_thread+0x93/0xd0
 [<ffffffff80244ad0>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff80241680>] ? worker_thread+0x0/0xd0
 [<ffffffff8024466d>] kthread+0x4d/0x80
 [<ffffffff8020c3b8>] child_rip+0xa/0x12
 [<ffffffff80244620>] ? kthread+0x0/0x80
 [<ffffffff8020c3ae>] ? child_rip+0x0/0x12

Same setup as the bug 1/3.

		-- Pete

Re: [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

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.

Re: [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

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.

[-- Attachment #2: delay-bsg-unreg.patch --]
[-- Type: text/x-patch, Size: 907 bytes --]

Delay bsg unregistration, because we want to wait until all the request/cmds
have released their reference.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ed83cdb..b9b09a7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -294,6 +294,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	}
 
 	if (sdev->request_queue) {
+		bsg_unregister_queue(sdev->request_queue);
 		sdev->request_queue->queuedata = NULL;
 		/* user context needed to free queue */
 		scsi_free_queue(sdev->request_queue);
@@ -857,7 +858,6 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
 		return;
 
-	bsg_unregister_queue(sdev->request_queue);
 	class_device_unregister(&sdev->sdev_classdev);
 	transport_remove_device(dev);
 	device_del(dev);

Re: [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Tue, 11 Mar 2008 00:36:51 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> Delay bsg unregistration, because we want to wait until all the request/cmds
> have released their reference.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

Thanks!

Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ed83cdb..b9b09a7 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -294,6 +294,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>  	}
>  
>  	if (sdev->request_queue) {
> +		bsg_unregister_queue(sdev->request_queue);
>  		sdev->request_queue->queuedata = NULL;
>  		/* user context needed to free queue */
>  		scsi_free_queue(sdev->request_queue);
> @@ -857,7 +858,6 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>  		return;
>  
> -	bsg_unregister_queue(sdev->request_queue);
>  	class_device_unregister(&sdev->sdev_classdev);
>  	transport_remove_device(dev);
>  	device_del(dev);

Re: [BUG 1/3] bsg queue oops with iscsi logout

[Pete Wyckoff]

michaelc@cs.wisc.edu wrote on Tue, 11 Mar 2008 00:36 -0500:
> 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.

> Delay bsg unregistration, because we want to wait until all the request/cmds
> have released their reference.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ed83cdb..b9b09a7 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -294,6 +294,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>  	}
>  
>  	if (sdev->request_queue) {
> +		bsg_unregister_queue(sdev->request_queue);
>  		sdev->request_queue->queuedata = NULL;
>  		/* user context needed to free queue */
>  		scsi_free_queue(sdev->request_queue);
> @@ -857,7 +858,6 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>  		return;
>  
> -	bsg_unregister_queue(sdev->request_queue);
>  	class_device_unregister(&sdev->sdev_classdev);
>  	transport_remove_device(dev);
>  	device_del(dev);

This definitely avoids the hang in [BUG 3/3].  Instead I get the
useful console message:

    scsi 4:0:0:1: rejecting I/O to dead device

My tested-by for that.  But it leaves a bit of a mess in sysfs.
Apparently udev did not get the message that these devs went away.

ib30$ ll /sys/class/scsi_device/
total 0
drwxr-xr-x  3 root root 0 Mar 14 20:04 ./
drwxr-xr-x 28 root root 0 Mar 14 20:02 ../
drwxr-xr-x  2 root root 0 Mar 14 20:02 2:0:0:0/

ib30$ ll /dev/bsg/
total 0
drwxr-xr-x  2 root root    100 Mar 14 20:03 ./
drwxr-xr-x 12 root root   3020 Mar 14 20:04 ../
crw-rw-rw-  1 root root 254, 0 Mar 14 20:02 2:0:0:0
crw-rw-rw-  1 root root 254, 1 Mar 14 20:03 4:0:0:0
crw-rw-rw-  1 root root 254, 2 Mar 14 20:03 4:0:0:1

I do have a special udev rule to explain why the devs look like
this:

ACTION=="add", SUBSYSTEM=="bsg", NAME="bsg/%k", MODE="0666", OPTIONS="last_rule"

but that doesn't explain why they're still in /dev.

		-- Pete

Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[James Bottomley]

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
     3. Modify bsg_register_queue not to take a device reference on the
        grounds that the queue lifetime is *always* shorter than the
        generic device lifetime.  This sounds the most plausible, but
        it's unsafe ... if the device gets released first the queue is
        left holding a freed area of memory.  Jens, can we make this
        work?

James

---

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b9b09a7..9037b34 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -294,6 +294,20 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	}
 
 	if (sdev->request_queue) {
+		/*
+		 * FIXME: bsg_unregister_queue() is going to drop a
+		 * reference to the device.  This actually causes the
+		 * references to go negative (yuk).  We have to do
+		 * this because we're trying to do the queue
+		 * unregister in the device release routine, so we
+		 * already released the reference taken by bsg in
+		 * __scsi_remove_device().
+		 *
+		 * We're also calling bsg_unregister_queue in here on
+		 * things we never registered (scanned devices that
+		 * return DID_NO_CONNECT) if bsg ever starts squawking
+		 * about this, it needs fixing here.
+		 */
 		bsg_unregister_queue(sdev->request_queue);
 		sdev->request_queue->queuedata = NULL;
 		/* user context needed to free queue */
@@ -866,6 +880,13 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);
 	put_device(dev);
+	/*
+	 *  FIXME: This is an extra put because bsg_register_queue()
+	 * takes a reference to the device.  We do it here to allow
+	 * bsg_unregister_queue() to be called from the release method
+	 * of the device.
+	 */
+	put_device(dev);
 }
 
 /**

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Sat, 22 Mar 2008 11:06:00 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> 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.

This is extremely bad. Really sorry about it.


> 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.

bsg_unregister_queue checks that queues are registered. scsi_sysfs and
sas_transport ignore the failure of bsg_register_queue so bsg needs to
do that.


> 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
>      3. Modify bsg_register_queue not to take a device reference on the
>         grounds that the queue lifetime is *always* shorter than the
>         generic device lifetime.  This sounds the most plausible, but
>         it's unsafe ... if the device gets released first the queue is
>         left holding a freed area of memory.  Jens, can we make this
>         work?

If the third option doesn't work, bsg_unregister_queue needs to clean
up active commands properly instead of just freeing the stuff when
it's called from __scsi_remove_device. I think that sg driver does the
similar thing (if the third option works, we can remove the code in sg
too). I'll fix this for 2.6.26.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Sat, 22 Mar 2008 11:06:00 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> 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?

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[James Bottomley]

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@HansenPartnership.com> 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.

James

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Wed, 26 Mar 2008 07:36:26 -0700
James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/bsg.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 8917c51..28f0d1e 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -705,6 +705,7 @@ static struct bsg_device *bsg_alloc_device(void)
 static int bsg_put_device(struct bsg_device *bd)
 {
 	int ret = 0;
+	struct device *dev = bd->queue->bsg_dev.dev;
 
 	mutex_lock(&bsg_mutex);
 
@@ -730,6 +731,7 @@ static int bsg_put_device(struct bsg_device *bd)
 	kfree(bd);
 out:
 	mutex_unlock(&bsg_mutex);
+	put_device(dev);
 	return ret;
 }
 
@@ -789,21 +791,27 @@ static struct bsg_device *bsg_get_device(struct inode *inode, struct file *file)
 	struct bsg_device *bd;
 	struct bsg_class_device *bcd;
 
-	bd = __bsg_get_device(iminor(inode));
-	if (bd)
-		return bd;
-
 	/*
 	 * find the class device
 	 */
 	mutex_lock(&bsg_mutex);
 	bcd = idr_find(&bsg_minor_idr, iminor(inode));
+	if (bcd)
+		get_device(bcd->dev);
 	mutex_unlock(&bsg_mutex);
 
 	if (!bcd)
 		return ERR_PTR(-ENODEV);
 
-	return bsg_add_device(inode, bcd->queue, file);
+	bd = __bsg_get_device(iminor(inode));
+	if (bd)
+		return bd;
+
+	bd = bsg_add_device(inode, bcd->queue, file);
+	if (!bd)
+		put_device(bcd->dev);
+
+	return bd;
 }
 
 static int bsg_open(struct inode *inode, struct file *file)
@@ -942,7 +950,6 @@ void bsg_unregister_queue(struct request_queue *q)
 	class_device_unregister(bcd->class_dev);
 	put_device(bcd->dev);
 	bcd->class_dev = NULL;
-	bcd->dev = NULL;
 	mutex_unlock(&bsg_mutex);
 }
 EXPORT_SYMBOL_GPL(bsg_unregister_queue);
-- 
1.5.3.7

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

FUJITA Tomonori wrote:
> On Wed, 26 Mar 2008 07:36:26 -0700
> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> 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.
> 

It looks like it fixes the life time problem.

My patch was actually supposed to fix #3 and fixing #1 was a side 
affect. Will bsg_release still be called when the device is closed. If 
so then it may not fix #3 because the bsg_release function still needs 
to grab the mutex. Maybe bsg_complete_all_commands just needs to drop 
the mutex while it waits for IO to complete.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Wed, 26 Mar 2008 20:32:13 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 26 Mar 2008 07:36:26 -0700
> > James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> > 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.
> > 
> 
> It looks like it fixes the life time problem.

With the reverting and my patch, seems that all the problems (#1, #2,
and #3) has gone for me.


> My patch was actually supposed to fix #3 and fixing #1 was a side 
> affect. Will bsg_release still be called when the device is closed. If 
> so then it may not fix #3 because the bsg_release function still needs 
> to grab the mutex. Maybe bsg_complete_all_commands just needs to drop 
> the mutex while it waits for IO to complete.

I don't hit #3 problem. A process holds the mutex and waiting for I/O
completion. But fail_all_commands() makes all the commands fail, the
process releases the mutex and then bsg_unregister_queue is called.

But yeah, I think that we don't need to hold the mutex during waiting
for I/O completion here.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

FUJITA Tomonori wrote:
>> My patch was actually supposed to fix #3 and fixing #1 was a side 
>> affect. Will bsg_release still be called when the device is closed. If 
>> so then it may not fix #3 because the bsg_release function still needs 
>> to grab the mutex. Maybe bsg_complete_all_commands just needs to drop 
>> the mutex while it waits for IO to complete.
> 
> I don't hit #3 problem. A process holds the mutex and waiting for I/O
> completion. But fail_all_commands() makes all the commands fail, the
> process releases the mutex and then bsg_unregister_queue is called.
> 

I think what Pete saw was due to the transport class (really block 
layer) holding onto the command until recovery is completed. So until 
the iscsi recovery timer fires or the session comes back we will wait 
for the device to be unblocked and the command to complete. For FC we 
would have to wait for fail fast tmo or dev loss tmo depending on the 
IO. So #3 does not look like a bsg problem.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

FUJITA Tomonori wrote:
> On Wed, 26 Mar 2008 07:36:26 -0700
> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> 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?

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.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

Mike Christie wrote:
> FUJITA Tomonori wrote:
>> On Wed, 26 Mar 2008 07:36:26 -0700
>> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
>> 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?
> 
> 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.

Doh, I guess we sort of do this today. It looks like the blk_execute 
functions are bypassing the QUEUE_FLAG_DEAD checks, so 
scsi_device_dev_release_usercontext could have called scsi_free_queue, 
but if bsg calls blk_execute at the same time then it could stick a 
request into the queue and end up calling the scsi_request_fn (maybe 
this is what happens in #2 and when scsi_request_fn calls get_device we 
get that weird error since the refcount on the device is zero).

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Wed, 26 Mar 2008 21:18:02 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> Mike Christie wrote:
> > FUJITA Tomonori wrote:
> >> On Wed, 26 Mar 2008 07:36:26 -0700
> >> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> >> 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?
> > 
> > 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.
> 
> Doh, I guess we sort of do this today. It looks like the blk_execute 
> functions are bypassing the QUEUE_FLAG_DEAD checks, so 
> scsi_device_dev_release_usercontext could have called scsi_free_queue, 
> but if bsg calls blk_execute at the same time then it could stick a 
> request into the queue and end up calling the scsi_request_fn (maybe 
> this is what happens in #2 and when scsi_request_fn calls get_device we 
> get that weird error since the refcount on the device is zero).

With the patch, the device is still hold, so a command is rejected
properly by scsi_prep_state_check.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Wed, 26 Mar 2008 20:51:44 -0500
Mike Christie <michaelc@cs.wisc.edu> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 26 Mar 2008 07:36:26 -0700
> > James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> > 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@lab.ntt.co.jp>
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@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 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

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Thu, 27 Mar 2008 20:11:52 +0900
FUJITA Tomonori <tomof@acm.org> wrote:

> On Wed, 26 Mar 2008 20:51:44 -0500
> Mike Christie <michaelc@cs.wisc.edu> wrote:
> 
> > FUJITA Tomonori wrote:
> > > On Wed, 26 Mar 2008 07:36:26 -0700
> > > James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> > > 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@lab.ntt.co.jp>
> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Really sorry, please apply this one.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Both takes a ref to a queue. But blk_get_queue checks QUEUE_FLAG_DEAD
and is more appropriate interface here.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/bsg.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 28f0d1e..e2c65a1 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 ERR_PTR(-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);
@@ -808,7 +813,7 @@ static struct bsg_device *bsg_get_device(struct inode *inode, struct file *file)
 		return bd;
 
 	bd = bsg_add_device(inode, bcd->queue, file);
-	if (!bd)
+	if (IS_ERR(bd))
 		put_device(bcd->dev);
 
 	return bd;
-- 
1.5.3.7

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[James Bottomley]

On Thu, 2008-03-27 at 21:18 +0900, FUJITA Tomonori wrote:
> On Thu, 27 Mar 2008 20:11:52 +0900
> FUJITA Tomonori <tomof@acm.org> wrote:
> 
> > On Wed, 26 Mar 2008 20:51:44 -0500
> > Mike Christie <michaelc@cs.wisc.edu> wrote:
> > 
> > > FUJITA Tomonori wrote:
> > > > On Wed, 26 Mar 2008 07:36:26 -0700
> > > > James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> > > > 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@lab.ntt.co.jp>
> > Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> 
> Really sorry, please apply this one.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> 
> Both takes a ref to a queue. But blk_get_queue checks QUEUE_FLAG_DEAD
> and is more appropriate interface here.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>


This looks reasonable to me.  It's probably a rc-fixes patch, so could I
get Jen's ack and some evidence of testing (and that it actually fixes
the bug).

Thanks,

James


>  block/bsg.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 28f0d1e..e2c65a1 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 ERR_PTR(-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);
> @@ -808,7 +813,7 @@ static struct bsg_device *bsg_get_device(struct inode *inode, struct file *file)
>  		return bd;
>  
>  	bd = bsg_add_device(inode, bcd->queue, file);
> -	if (!bd)
> +	if (IS_ERR(bd))
>  		put_device(bcd->dev);
>  
>  	return bd;

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Sun, 30 Mar 2008 12:39:36 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> On Thu, 2008-03-27 at 21:18 +0900, FUJITA Tomonori wrote:
> > On Thu, 27 Mar 2008 20:11:52 +0900
> > FUJITA Tomonori <tomof@acm.org> wrote:
> > 
> > > On Wed, 26 Mar 2008 20:51:44 -0500
> > > Mike Christie <michaelc@cs.wisc.edu> wrote:
> > > 
> > > > FUJITA Tomonori wrote:
> > > > > On Wed, 26 Mar 2008 07:36:26 -0700
> > > > > James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> > > > > 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@lab.ntt.co.jp>
> > > Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> > 
> > Really sorry, please apply this one.
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> > 
> > Both takes a ref to a queue. But blk_get_queue checks QUEUE_FLAG_DEAD
> > and is more appropriate interface here.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> 
> This looks reasonable to me.  It's probably a rc-fixes patch, so could I
> get Jen's ack and some evidence of testing (and that it actually fixes
> the bug).

Do you mean that the patch to take a ref to strutc device
(e.g. sdev_gendev for scsi devices) in fops->open is a reasonable fix?

http://marc.info/?l=linux-scsi&m=120654365424916&w=2

The patch with the commit reversion fixes all the problems for me that
Pete reported. Pete, can you test the patch?


It's a rc-fixes patch, but I'm fine with applying it to scsi-misc
(I'll send it to the stable tree later on).

The patch has one bug in an error handling path (I should have used
IS_ERR there). So I'll send an updated version shortly.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Sun, 30 Mar 2008 12:39:36 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
>> On Thu, 2008-03-27 at 21:18 +0900, FUJITA Tomonori wrote:
>>> On Thu, 27 Mar 2008 20:11:52 +0900
>>> FUJITA Tomonori <tomof@acm.org> wrote:
>>>
>>>> On Wed, 26 Mar 2008 20:51:44 -0500
>>>> Mike Christie <michaelc@cs.wisc.edu> wrote:
>>>>
>>>>> FUJITA Tomonori wrote:
>>>>>> On Wed, 26 Mar 2008 07:36:26 -0700
>>>>>> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
>>>>>> 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@lab.ntt.co.jp>
>>>> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
>>> Really sorry, please apply this one.
>>>
>>> =
>>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
>>>
>>> Both takes a ref to a queue. But blk_get_queue checks QUEUE_FLAG_DEAD
>>> and is more appropriate interface here.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Cc: Jens Axboe <jens.axboe@oracle.com>
>>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>>
>> This looks reasonable to me.  It's probably a rc-fixes patch, so could I
>> get Jen's ack and some evidence of testing (and that it actually fixes
>> the bug).
> 
> Do you mean that the patch to take a ref to strutc device
> (e.g. sdev_gendev for scsi devices) in fops->open is a reasonable fix?
> 
> http://marc.info/?l=linux-scsi&m=120654365424916&w=2
> 
> The patch with the commit reversion fixes all the problems for me that
> Pete reported. Pete, can you test the patch?
> 
> 
> It's a rc-fixes patch, but I'm fine with applying it to scsi-misc
> (I'll send it to the stable tree later on).
> 
> The patch has one bug in an error handling path (I should have used
> IS_ERR there). So I'll send an updated version shortly.

Hi Tomo.
Do you please have an accumulated latest patch for this problem.
(Or point me to the right one, I can't find it). I want to test
it here too. (Over rc-fixes)

Thanks
Boaz

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[FUJITA Tomonori]

On Wed, 02 Apr 2008 21:41:40 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > On Sun, 30 Mar 2008 12:39:36 -0500
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> >> On Thu, 2008-03-27 at 21:18 +0900, FUJITA Tomonori wrote:
> >>> On Thu, 27 Mar 2008 20:11:52 +0900
> >>> FUJITA Tomonori <tomof@acm.org> wrote:
> >>>
> >>>> On Wed, 26 Mar 2008 20:51:44 -0500
> >>>> Mike Christie <michaelc@cs.wisc.edu> wrote:
> >>>>
> >>>>> FUJITA Tomonori wrote:
> >>>>>> On Wed, 26 Mar 2008 07:36:26 -0700
> >>>>>> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> >>>>>> 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@lab.ntt.co.jp>
> >>>> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> >>> Really sorry, please apply this one.
> >>>
> >>> =
> >>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>> Subject: [PATCH] bsg: replace kobject_get with blk_get_queue
> >>>
> >>> Both takes a ref to a queue. But blk_get_queue checks QUEUE_FLAG_DEAD
> >>> and is more appropriate interface here.
> >>>
> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>> Cc: Jens Axboe <jens.axboe@oracle.com>
> >>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> >>
> >> This looks reasonable to me.  It's probably a rc-fixes patch, so could I
> >> get Jen's ack and some evidence of testing (and that it actually fixes
> >> the bug).
> > 
> > Do you mean that the patch to take a ref to strutc device
> > (e.g. sdev_gendev for scsi devices) in fops->open is a reasonable fix?
> > 
> > http://marc.info/?l=linux-scsi&m=120654365424916&w=2
> > 
> > The patch with the commit reversion fixes all the problems for me that
> > Pete reported. Pete, can you test the patch?
> > 
> > 
> > It's a rc-fixes patch, but I'm fine with applying it to scsi-misc
> > (I'll send it to the stable tree later on).
> > 
> > The patch has one bug in an error handling path (I should have used
> > IS_ERR there). So I'll send an updated version shortly.
> 
> Hi Tomo.
> Do you please have an accumulated latest patch for this problem.
> (Or point me to the right one, I can't find it). I want to test
> it here too. (Over rc-fixes)

No change since I submitted last time:

http://marc.info/?l=linux-scsi&m=120692552424155&w=2

They need to be applied to the latest Linus git (or scsi-fixes).

If you prefer a git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git bsg


James pointed out another race:

1. we hold the bsg device open and remove it.

2. we add a new device.

3. we try to open the new device

4. we get a ref to the removed device (but it's still hold open)
instead of the new one.


I overlooked this race (James, thanks a lot for pointing out
it). Fortunately, the fourth patch fixes this race. I've confirmed it.

So when submitting the patchset, I said that only the first patch is
crucial, however, the 4th patch is crucial too.

I'm fine with either via scsi-misc or scsi-fixes.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Boaz Harrosh]

On Thu, Apr 03 2008 at 0:00 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> No change since I submitted last time:
> 
> http://marc.info/?l=linux-scsi&m=120692552424155&w=2
> 
> They need to be applied to the latest Linus git (or scsi-fixes).
> 
> If you prefer a git tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git bsg
> 

Thanks for the git tree. The URLs inside the message above don't work for me.
They all point to the same email and without any patch inside. I don't
know if it is marc.info or my firefox that's bad.

Anyway the git tree is perfect, I will give them a spin.

> 
> James pointed out another race:
> 
> 1. we hold the bsg device open and remove it.
> 
> 2. we add a new device.
> 
> 3. we try to open the new device
> 
> 4. we get a ref to the removed device (but it's still hold open)
> instead of the new one.
> 
> 
> I overlooked this race (James, thanks a lot for pointing out
> it). Fortunately, the fourth patch fixes this race. I've confirmed it.
> 
> So when submitting the patchset, I said that only the first patch is
> crucial, however, the 4th patch is crucial too.
> 
> I'm fine with either via scsi-misc or scsi-fixes.
> --

Thanks
Boaz

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

FUJITA Tomonori wrote:
> On Wed, 26 Mar 2008 07:36:26 -0700
> James Bottomley <James.Bottomley@HansenPartnership.com> 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@HansenPartnership.com> 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@lab.ntt.co.jp>
> 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.

I think you need the ref count on the queue for scsi_transport_sas 
because it does not have the multi layer shutdown like how the 
scsi_devices do. If a request was just added to the queue and 
sas_smp_request was running, but then sas_bsg_remove is called the call 
to blk_cleanup_queue would free the queue from under the request fn.

Re: Serious regression caused by fix for [BUG 1/3] bsg queue oops with iscsi logout

[Mike Christie]

James Bottomley 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@HansenPartnership.com> 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.
> 

Sorry for the late response. I missed this mail.

Reverting is fine. This is not a iscsi specific problem. It will happen 
with any hotplug capable driver.