bsg queue oops with iscsi logout

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?

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 4 lines

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.

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

Signed-off-by Mike Christie <[email protected]>

drivers/scsi/scsi_sysfs.c +1 -1

index ed83cdb..b9b09a7 100644
@@ -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);

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 <[email protected]>

drivers/scsi/scsi_sysfs.c +1 -0

index ed83cdb..b9b09a7 100644
@@ -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);

Quoted 25 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 2 lines

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.

Quoted 29 lines · Mike Christie

Quoted 27 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 26 lines · Mike Christie

Quoted 25 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 8 lines

Quoted 2 lines

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

Quoted 47 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 26 lines · Mike Christie

Quoted 25 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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?

Quoted 57 lines · FUJITA Tomonori

Quoted 53 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 26 lines · Mike Christie

Quoted 25 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 8 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 43 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 42 lines · FUJITA Tomonori

Quoted 39 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 15 lines · Mike Christie

Quoted 14 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 82 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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).

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 43 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 42 lines · FUJITA Tomonori

Quoted 39 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 15 lines · Mike Christie

Quoted 14 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 76 lines · FUJITA Tomonori

Quoted 44 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 43 lines · FUJITA Tomonori

Quoted 40 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 15 lines · Mike Christie

Quoted 14 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 4 lines

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.

Quoted 97 lines · Mike Christie

Quoted 88 lines · FUJITA Tomonori

Quoted 59 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 58 lines · FUJITA Tomonori

Quoted 55 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 28 lines · Mike Christie

Quoted 27 lines · Mike Christie

Quoted 2 lines · Pete Wyckoff

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.

Quoted 9 lines

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?

Quoted 8 lines

Quoted 3 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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).

Quoted 7 lines

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.

Quoted 7 lines

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

Quoted 85 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 4 lines

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.

Quoted 86 lines · On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <[email protected]> wrote:

Quoted 85 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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 <[email protected]>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Quoted 113 lines · On Thu, 27 Mar 2008 20:11:52 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 86 lines · On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <[email protected]> wrote:

Quoted 85 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

Anyway, here's a patch to replace kobject_get with blk_get_queue.

James, please apply this patch too.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Really sorry, please apply this one.

=
From: FUJITA Tomonori <[email protected]>
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 <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

 block/bsg.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

block/bsg.c +8 -3

index 28f0d1e..e2c65a1 100644
@@ -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;

Quoted 133 lines · FUJITA Tomonori

Quoted 113 lines · On Thu, 27 Mar 2008 20:11:52 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 86 lines · On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <[email protected]> wrote:

Quoted 85 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

Anyway, here's a patch to replace kobject_get with blk_get_queue.

James, please apply this patch too.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Really sorry, please apply this one.

=
From: FUJITA Tomonori <[email protected]>
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.

Quoted 3 lines

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

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

Quoted 129 lines · On Sun, 30 Mar 2008 12:39:36 -0500 James Bottomley…

Quoted 128 lines · FUJITA Tomonori

Quoted 109 lines · On Thu, 27 Mar 2008 20:11:52 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 82 lines · On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <[email protected]> wrote:

Quoted 81 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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).

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

Anyway, here's a patch to replace kobject_get with blk_get_queue.

James, please apply this patch too.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Really sorry, please apply this one.

=
From: FUJITA Tomonori <[email protected]>
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.

Quoted 7 lines

Quoted 3 lines

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

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.

Quoted 152 lines · FUJITA Tomonori

Quoted 129 lines · On Sun, 30 Mar 2008 12:39:36 -0500 James Bottomley…

Quoted 128 lines · FUJITA Tomonori

Quoted 109 lines · On Thu, 27 Mar 2008 20:11:52 +0900 FUJITA Tomonori <[email protected]> wrote:

Quoted 82 lines · On Wed, 26 Mar 2008 20:51:44 -0500 Mike Christie <[email protected]> wrote:

Quoted 81 lines · FUJITA Tomonori

Quoted 53 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 52 lines · FUJITA Tomonori

Quoted 49 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 24 lines · Mike Christie

Quoted 23 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 7 lines

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?

Quoted 7 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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).

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

Anyway, here's a patch to replace kobject_get with blk_get_queue.

James, please apply this patch too.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] bsg: replace kobject_get with blk_get_queue

Really sorry, please apply this one.

=
From: FUJITA Tomonori <[email protected]>
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.

Quoted 7 lines

Quoted 3 lines

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

Cc James Bottomley <[email protected]>

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

Quoted 2 lines

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

Quoted 43 lines · On Wed, 26 Mar 2008 07:36:26 -0700 James Bottomley…

Quoted 42 lines · FUJITA Tomonori

Quoted 39 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 15 lines · Mike Christie

Quoted 14 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

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 <[email protected]>
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.

Quoted 42 lines · FUJITA Tomonori

Quoted 39 lines · On Sat, 22 Mar 2008 11:06:00 -0500 James Bottomley…

Quoted 15 lines · Mike Christie

Quoted 14 lines · Mike Christie

Quoted 1 line · Pete Wyckoff

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.

Quoted 3 lines

lately. 2.6.25-rc1 to -rc4 and fedora 8 iscsi-initiator-utils (865) to fedora devel (868). Bidi and varlen patches always too.

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

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

Quoted 5 lines

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.