Questions about scsi_target_reap and starget/sdev lifecyle

Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

James:

Can you or anyone else please explain what scsi_target_reap() and
especially what scsi_target.reap_ref are for?  What's the point of this
home-brewed reference counting scheme when there's already a perfectly
useful embedded struct device inside scsi_target?  Why isn't
scsi_target_reap() simply the release routine for the embedded device?

In addition, can someone please explain how long a scsi_target structure 
and a scsi_device structure are supposed to remain linked into the host's 
__targets and __devices lists once they have been removed?  The 
scsi_device link isn't severed until the release routine runs, which can 
be quite a long time after the device has been removed.  The scsi_target 
link is severed in scsi_target_reap(), which should be thought of as a
kind of release routine as well.

Won't this cause trouble in a hotplug environment?  A target or a LUN can 
be removed, and while it's still on the host's list a new target or LUN 
could be discovered with the same ID.  Having multiple entries on a list 
with the same ID sounds like a bad idea.  Is there some reason why these 
things aren't taken off their list as soon as they are removed?

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[James Bottomley]

On Tue, 2005-06-14 at 17:27 -0400, Alan Stern wrote:
> Can you or anyone else please explain what scsi_target_reap() and
> especially what scsi_target.reap_ref are for?  What's the point of this
> home-brewed reference counting scheme when there's already a perfectly
> useful embedded struct device inside scsi_target?  Why isn't
> scsi_target_reap() simply the release routine for the embedded device?

Look at how scsi_target_reap works: it's the decider for visibility
removal, not actual release, that's why we can't use the device
reference, which is the decider for release.  I could have used a kref,
but, given the special locking conventions and the tying to the __target
list, the call would have had to be wrappered anyway, so there didn't
seem to to be much point.

A target is basically a self destroying entity which is why the
complexity.

> In addition, can someone please explain how long a scsi_target structure 
> and a scsi_device structure are supposed to remain linked into the host's 
> __targets and __devices lists once they have been removed?  The 
> scsi_device link isn't severed until the release routine runs, which can 
> be quite a long time after the device has been removed.  The scsi_target 
> link is severed in scsi_target_reap(), which should be thought of as a
> kind of release routine as well.

__target entries are removed when the target goes invisible (and for
this to happen, all the devices have to be already gone).  __devices
when the device is released.

> Won't this cause trouble in a hotplug environment?  A target or a LUN can 
> be removed, and while it's still on the host's list a new target or LUN 
> could be discovered with the same ID.  Having multiple entries on a list 
> with the same ID sounds like a bad idea.  Is there some reason why these 
> things aren't taken off their list as soon as they are removed?

It shouldn't, that's what rescan is about.  If the device isn't gone,
you get the old device back again.

James

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 14 Jun 2005, James Bottomley wrote:

> > Won't this cause trouble in a hotplug environment?  A target or a LUN can 
> > be removed, and while it's still on the host's list a new target or LUN 
> > could be discovered with the same ID.  Having multiple entries on a list 
> > with the same ID sounds like a bad idea.  Is there some reason why these 
> > things aren't taken off their list as soon as they are removed?
> 
> It shouldn't, that's what rescan is about.  If the device isn't gone,
> you get the old device back again.

Is this wise?  With hotplugging, it's possible that the new device is
physically different from the one that was there before.  You don't want
to re-use an old structure with stale data in it when that happens.

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 14 Jun 2005, James Bottomley wrote:

> On Tue, 2005-06-14 at 17:27 -0400, Alan Stern wrote:
> > Can you or anyone else please explain what scsi_target_reap() and
> > especially what scsi_target.reap_ref are for?  What's the point of this
> > home-brewed reference counting scheme when there's already a perfectly
> > useful embedded struct device inside scsi_target?  Why isn't
> > scsi_target_reap() simply the release routine for the embedded device?
> 
> Look at how scsi_target_reap works: it's the decider for visibility
> removal, not actual release, that's why we can't use the device
> reference, which is the decider for release.  I could have used a kref,
> but, given the special locking conventions and the tying to the __target
> list, the call would have had to be wrappered anyway, so there didn't
> seem to to be much point.
> 
> A target is basically a self destroying entity which is why the
> complexity.

> __target entries are removed when the target goes invisible (and for
> this to happen, all the devices have to be already gone).  __devices
> when the device is released.

This means that scsi_target_reap can be called and the __targets list 
changed essentially at any time (subject only to the host_lock).  Hence it 
is impossible for scsi_forget_host to iterate through the list of targets 
belonging to the host: While it is working to remove one target, the next 
target on the list (stored in the tmp variable) might be removed by 
another thread.

In fact there doesn't seem to be any safe way to remove all the targets
from a host.  And what's to prevent scsi_target_reap being called twice
for the same target?

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[James Bottomley]

On Wed, 2005-06-15 at 17:11 -0400, Alan Stern wrote:
> This means that scsi_target_reap can be called and the __targets list 
> changed essentially at any time (subject only to the host_lock).  Hence it 
> is impossible for scsi_forget_host to iterate through the list of targets 
> belonging to the host: While it is working to remove one target, the next 
> target on the list (stored in the tmp variable) might be removed by 
> another thread.

It's no better nor worse than we already have.  As has been said many
times before, we need a proper host state model.

> In fact there doesn't seem to be any safe way to remove all the targets
> from a host.  And what's to prevent scsi_target_reap being called twice
> for the same target?

The usage, if you look at the code ... it's alloc/reap or inc
reap_ref/reap

James

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Wed, 15 Jun 2005, James Bottomley wrote:

> On Wed, 2005-06-15 at 17:11 -0400, Alan Stern wrote:
> > This means that scsi_target_reap can be called and the __targets list 
> > changed essentially at any time (subject only to the host_lock).  Hence it 
> > is impossible for scsi_forget_host to iterate through the list of targets 
> > belonging to the host: While it is working to remove one target, the next 
> > target on the list (stored in the tmp variable) might be removed by 
> > another thread.
> 
> It's no better nor worse than we already have.  As has been said many
> times before, we need a proper host state model.
> 
> > In fact there doesn't seem to be any safe way to remove all the targets
> > from a host.  And what's to prevent scsi_target_reap being called twice
> > for the same target?
> 
> The usage, if you look at the code ... it's alloc/reap or inc
> reap_ref/reap

Okay, I understand a little better now.

Would it hurt anything to change scsi_remove_host so that SHOST_DEL gets
set at the start, before scsi_forget_host is called?  If that were done,
then we could make every pathway that adds a device acquire the scan_mutex
and fail if SHOST_DEL is already set.  Thus no devices would ever be added
after forget_host had removed the existing ones, which can happen as
things stand.

Furthermore, forget_host could be changed to loop over the __devices list
instead of the __targets list.  The net effect would be the same: All the
devices on the host would be removed and the targets would automatically
get reaped by scsi_device_dev_release.  There wouldn't be any need to
iterate over the targets at all.

As a final change, the new loop-over-devices in forget_host and the one in
__scsi_remove_target should be made to skip over devices with SDEV_DEL
already set, and each time they call scsi_remove_device the loops should
restart from the beginning.  This will eliminate the problem of the tmp
pointer being pulled out from under the code (even though it has 
quadratic behavior).

Do these changes sound okay?

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> On Wed, 15 Jun 2005, James Bottomley wrote:
> 
> > On Wed, 2005-06-15 at 17:11 -0400, Alan Stern wrote:
> > > This means that scsi_target_reap can be called and the __targets list 
> > > changed essentially at any time (subject only to the host_lock).  Hence it 
> > > is impossible for scsi_forget_host to iterate through the list of targets 
> > > belonging to the host: While it is working to remove one target, the next 
> > > target on the list (stored in the tmp variable) might be removed by 
> > > another thread.
> > 
> > It's no better nor worse than we already have.  As has been said many
> > times before, we need a proper host state model.
> > 
> > > In fact there doesn't seem to be any safe way to remove all the targets
> > > from a host.  And what's to prevent scsi_target_reap being called twice
> > > for the same target?
> > 
> > The usage, if you look at the code ... it's alloc/reap or inc
> > reap_ref/reap
> 
> Okay, I understand a little better now.
> 
> Would it hurt anything to change scsi_remove_host so that SHOST_DEL gets
> set at the start, before scsi_forget_host is called?  If that were done,
> then we could make every pathway that adds a device acquire the scan_mutex
> and fail if SHOST_DEL is already set.  Thus no devices would ever be added
> after forget_host had removed the existing ones, which can happen as
> things stand.
> 
> Furthermore, forget_host could be changed to loop over the __devices list
> instead of the __targets list.  The net effect would be the same: All the
> devices on the host would be removed and the targets would automatically
> get reaped by scsi_device_dev_release.  There wouldn't be any need to
> iterate over the targets at all.
> 
> As a final change, the new loop-over-devices in forget_host and the one in
> __scsi_remove_target should be made to skip over devices with SDEV_DEL
> already set, and each time they call scsi_remove_device the loops should
> restart from the beginning.  This will eliminate the problem of the tmp
> pointer being pulled out from under the code (even though it has 
> quadratic behavior).
> 
> Do these changes sound okay?
> 

I have something similar that I was testing since you mentioned the
problem the other day. Our build machine went down so I will need to wait
until tomorrow to get access to my patches for a post, if you have already
rolled a patch we can compare notes when I post.

What I did in the patch sequence was:
	1.) Recode the scsi_host state model to align with the device
	state model (i.e. added scsi_host_set_state function and
	associated changes).
	2.) Made shost cancel matched the scsi device state model and set
	this at the top of scsi_remove_host. Previous cancel code was not
	doing anything as the list was normally empty do to
	scsi_forget_host being called first. Also there where possible
	race conditions that you previously mentioned about canceling
	commands in this method.
	3.) Added choke point checks under the scan_mutex to determine if
	scanning is allowed (scsi_host_scan_allowed).
	4.) Added a target state model to match the scsi device state
	model.
	5.) I use the STGT_CANCEL and STGT_DEL states in scsi_forget_host
	to skip over entries in the list as I am no longer using the
	_safe version.
This looks like a good starting point with limited testing. I also need to
more states to the target state model as I only added a few that I needed
for the delete code.

Anyway sorry about the delay.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[James Bottomley]

On Thu, 2005-06-16 at 00:31 -0700, Mike Anderson wrote:

> I have something similar that I was testing since you mentioned the
> problem the other day. Our build machine went down so I will need to wait
> until tomorrow to get access to my patches for a post, if you have already
> rolled a patch we can compare notes when I post.
> 
> What I did in the patch sequence was:
> 	1.) Recode the scsi_host state model to align with the device
> 	state model (i.e. added scsi_host_set_state function and
> 	associated changes).
> 	2.) Made shost cancel matched the scsi device state model and set
> 	this at the top of scsi_remove_host. Previous cancel code was not
> 	doing anything as the list was normally empty do to
> 	scsi_forget_host being called first. Also there where possible
> 	race conditions that you previously mentioned about canceling
> 	commands in this method.
> 	3.) Added choke point checks under the scan_mutex to determine if
> 	scanning is allowed (scsi_host_scan_allowed).
> 	4.) Added a target state model to match the scsi device state
> 	model.
> 	5.) I use the STGT_CANCEL and STGT_DEL states in scsi_forget_host
> 	to skip over entries in the list as I am no longer using the
> 	_safe version.
> This looks like a good starting point with limited testing. I also need to
> more states to the target state model as I only added a few that I needed
> for the delete code.

That sounds something like what I was thinking.

There is some subtlety to this: the call to forget host with existing
targets must:

a) put the host into a state where it won't accept any additions or
removal.
b) loop over targets and devices removing them from visibility and doing
final puts for them
c) finally remove the host from visibility, set the deleted state and do
the final put (which may not remove the actual structure until the last
referrer relinquishes its reference).

Also, since the target is a pure container, I don't think it needs to be
a party to the state model.

Anyway, post the code and we can refine it.

James

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Thu, 16 Jun 2005, James Bottomley wrote:

> On Thu, 2005-06-16 at 00:31 -0700, Mike Anderson wrote:
> 
> > I have something similar that I was testing since you mentioned the
> > problem the other day. Our build machine went down so I will need to wait
> > until tomorrow to get access to my patches for a post, if you have already
> > rolled a patch we can compare notes when I post.
> > 
> > What I did in the patch sequence was:
> > 	1.) Recode the scsi_host state model to align with the device
> > 	state model (i.e. added scsi_host_set_state function and
> > 	associated changes).
> > 	2.) Made shost cancel matched the scsi device state model and set
> > 	this at the top of scsi_remove_host. Previous cancel code was not
> > 	doing anything as the list was normally empty do to
> > 	scsi_forget_host being called first. Also there where possible
> > 	race conditions that you previously mentioned about canceling
> > 	commands in this method.
> > 	3.) Added choke point checks under the scan_mutex to determine if
> > 	scanning is allowed (scsi_host_scan_allowed).
> > 	4.) Added a target state model to match the scsi device state
> > 	model.
> > 	5.) I use the STGT_CANCEL and STGT_DEL states in scsi_forget_host
> > 	to skip over entries in the list as I am no longer using the
> > 	_safe version.
> > This looks like a good starting point with limited testing. I also need to
> > more states to the target state model as I only added a few that I needed
> > for the delete code.

That's more ambitious than what I was planning, although it seems to cover
much the same ground.  At the moment my patchwork is only partly finished.

> That sounds something like what I was thinking.
> 
> There is some subtlety to this: the call to forget host with existing
> targets must:
> 
> a) put the host into a state where it won't accept any additions or
> removal.

I don't see anything wrong with allowing removals, but certainly it 
shouldn't accept any additions.  Would it be best to add a new 
SHOST_REMOVED state bit and test for it every time the scan_mutex is 
acquired?

> b) loop over targets and devices removing them from visibility and doing
> final puts for them

There's no point in looping over targets.  As the devices get released
they will automatically take the targets along with them.  Visibility of 
targets won't matter because no new devices can be created.

> c) finally remove the host from visibility, set the deleted state and do
> the final put (which may not remove the actual structure until the last
> referrer relinquishes its reference).

Aside from the visibility part, this is already done by scsi_remove_host 
and the LLDs, right?

> Also, since the target is a pure container, I don't think it needs to be
> a party to the state model.

Agreed.

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

James and Mike:

Here's my patch (as529), very lightly tested.  Among the changes:

	Introduce an SHOST_REMOVE state bit, set it at the start
	of scsi_remove_host, and check it every time scan_mutex is
	acquired.

	In scsi_remove_host, call scsi_host_cancel _before_ calling
	scsi_forget_host so that outstanding I/O really does get
	cancelled.  I don't think this will cause any problems,
	but you know the code better than I do.

	Fix an unchecked call to scsi_device_get in
	scsi_probe_and_add_lun.  The call can fail (if the LLD is
	being unloaded, for example) in which case the patch calls
	scsi_remove_device.  This worked, but I'm not certain it's
	exactly the right way to handle the failure.

	Rename scsi_scan_target to __scsi_scan_target, and introduce
	a new exported function named scsi_scan_target, which merely
	acquires the scan_mutex around a call to the old routine.
	Change existing calls so they refer to __scsi_scan_target.

	Don't acquire the scan_mutex in scsi_remove_device.  It's
	not needed since there's nothing wrong with removing devices
	during scanning, and it will cause deadlock under certain
	error conditions.  (I'm not certain about this either.  Will
	removing a device as it's being scanned cause a problem?
	Perhaps there should be a separate version of the routine
	that does acquire the scan_mutex.)

	Make scsi_forget_host remove all devices, not all targets.

	Make the loops to remove devices in scsi_forget_host and
	__scsi_remove_target restart from the beginning every time
	a device is removed (rather than using list_for_each_entry_safe,
	which is very definitely _unsafe_), and skip over devices
	that are already in the SDEV_DEL state.

This fixes several oopses I encountered during testing.  Do you think this 
is ready to be applied?

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

--- a/include/scsi/scsi_host.h	Mon Jun 13 14:57:26 2005
+++ b/include/scsi/scsi_host.h	Fri Jun 17 22:19:09 2005
@@ -411,6 +411,7 @@
 	SHOST_DEL,
 	SHOST_CANCEL,
 	SHOST_RECOVERY,
+	SHOST_REMOVE,
 };
 
 struct Scsi_Host {
--- a/drivers/scsi/hosts.c	Mon Jun 13 14:57:14 2005
+++ b/drivers/scsi/hosts.c	Fri Jun 17 22:20:19 2005
@@ -74,8 +74,13 @@
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	scsi_forget_host(shost);
+	set_bit(SHOST_REMOVE, &shost->shost_state);
 	scsi_host_cancel(shost, 0);
+
+	down(&shost->scan_mutex);	/* Wait for scanning to stop */
+	up(&shost->scan_mutex);
+
+	scsi_forget_host(shost);
 	scsi_proc_host_rm(shost);
 
 	set_bit(SHOST_DEL, &shost->shost_state);
--- a/drivers/scsi/scsi_scan.c	Mon Jun 13 14:58:06 2005
+++ b/drivers/scsi/scsi_scan.c	Fri Jun 17 22:59:52 2005
@@ -843,8 +843,12 @@
  out_free_sdev:
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (sdevp) {
-			scsi_device_get(sdev);
-			*sdevp = sdev;
+			if (scsi_device_get(sdev) == 0) {
+				*sdevp = sdev;
+			} else {
+				scsi_remove_device(sdev);
+				res = SCSI_SCAN_NO_RESPONSE;
+			}
 		}
 	} else {
 		if (sdev->host->hostt->slave_destroy)
@@ -1186,22 +1190,28 @@
 				      uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
-	struct device *parent = &shost->shost_gendev;
 	int res;
-	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
+	struct scsi_target *starget;
 
-	if (!starget)
-		return ERR_PTR(-ENOMEM);
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state)) {
+		sdev = ERR_PTR(-ENODEV);
+		goto out;
+	}
+	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
+	if (!starget) {
+		sdev = ERR_PTR(-ENOMEM);
+		goto out;
+	}
 
 	get_device(&starget->dev);
-	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
-	up(&shost->scan_mutex);
 	scsi_target_reap(starget);
 	put_device(&starget->dev);
-
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(__scsi_add_device);
@@ -1222,29 +1232,9 @@
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-/**
- * scsi_scan_target - scan a target id, possibly including all LUNs on the
- *     target.
- * @sdevsca:	Scsi_Device handle for scanning
- * @shost:	host to scan
- * @channel:	channel to scan
- * @id:		target id to scan
- *
- * Description:
- *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
- *     0, and possibly all LUNs on the target id.
- *
- *     Use the pre-allocated @sdevscan as a handle for the scanning. This
- *     function sets sdevscan->host, sdevscan->id and sdevscan->lun; the
- *     scanning functions modify sdevscan->lun.
- *
- *     First try a REPORT LUN scan, if that does not scan the target, do a
- *     sequential scan of LUNs on the target id.
- **/
-void scsi_scan_target(struct device *parent, unsigned int channel,
-		      unsigned int id, unsigned int lun, int rescan)
+static void __scsi_scan_target(struct Scsi_Host *shost, unsigned int channel,
+		unsigned int id, unsigned int lun, int rescan)
 {
-	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
 	int res;
 	struct scsi_device *sdev = NULL;
@@ -1257,7 +1247,7 @@
 		return;
 
 
-	starget = scsi_alloc_target(parent, channel, id);
+	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
 
 	if (!starget)
 		return;
@@ -1304,6 +1294,32 @@
 
 	put_device(&starget->dev);
 }
+
+/**
+ * scsi_scan_target - scan a target id, possibly including all LUNs on the
+ *     target.
+ * @parent:	host to scan
+ * @channel:	channel to scan
+ * @id:		target id to scan
+ * @rescan:	passed to LUN scanning routines
+ *
+ * Description:
+ *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
+ *     0, and possibly all LUNs on the target id.
+ *
+ *     First try a REPORT LUN scan, if that does not scan the target, do a
+ *     sequential scan of LUNs on the target id.
+ **/
+void scsi_scan_target(struct device *parent, unsigned int channel,
+		      unsigned int id, unsigned int lun, int rescan)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+
+	down(&shost->scan_mutex);
+	if (!test_bit(SHOST_REMOVE, &shost->shost_state))
+		__scsi_scan_target(shost, channel, id, lun, rescan);
+	up(&shost->scan_mutex);
+}
 EXPORT_SYMBOL(scsi_scan_target);
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
@@ -1329,10 +1345,10 @@
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			scsi_scan_target(&shost->shost_gendev, channel, order_id, lun, rescan);
+			__scsi_scan_target(shost, channel, order_id, lun, rescan);
 		}
 	else
-		scsi_scan_target(&shost->shost_gendev, channel, id, lun, rescan);
+		__scsi_scan_target(shost, channel, id, lun, rescan);
 }
 
 int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
@@ -1347,11 +1363,14 @@
 		return -EINVAL;
 
 	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
 	if (channel == SCAN_WILD_CARD) 
 		for (channel = 0; channel <= shost->max_channel; channel++)
 			scsi_scan_channel(shost, channel, id, lun, rescan);
 	else
 		scsi_scan_channel(shost, channel, id, lun, rescan);
+out:
 	up(&shost->scan_mutex);
 
 	return 0;
@@ -1383,23 +1402,17 @@
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct scsi_target *starget, *tmp;
+	struct scsi_device *sdev;
 	unsigned long flags;
 
-	/*
-	 * Ok, this look a bit strange.  We always look for the first device
-	 * on the list as scsi_remove_device removes them from it - thus we
-	 * also have to release the lock.
-	 * We don't need to get another reference to the device before
-	 * releasing the lock as we already own the reference from
-	 * scsi_register_device that's release in scsi_remove_device.  And
-	 * after that we don't look at sdev anymore.
-	 */
+restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state == SDEV_DEL)
+			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
-		scsi_remove_target(&starget->dev);
-		spin_lock_irqsave(shost->host_lock, flags);
+		scsi_remove_device(sdev);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
@@ -1426,12 +1439,16 @@
  */
 struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct scsi_target *starget;
 
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
+
 	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
 	if (!starget)
-		return NULL;
+		goto out;
 
 	sdev = scsi_alloc_sdev(starget, 0, NULL);
 	if (sdev) {
@@ -1439,6 +1456,8 @@
 		sdev->borken = 0;
 	}
 	put_device(&starget->dev);
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(scsi_get_host_dev);
--- a/drivers/scsi/scsi_sysfs.c	Fri Jun 17 22:23:37 2005
+++ b/drivers/scsi/scsi_sysfs.c	Fri Jun 17 22:25:18 2005
@@ -631,11 +631,8 @@
  **/
 void scsi_remove_device(struct scsi_device *sdev)
 {
-	struct Scsi_Host *shost = sdev->host;
-
-	down(&shost->scan_mutex);
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		goto out;
+		return;
 
 	class_device_unregister(&sdev->sdev_classdev);
 	device_del(&sdev->sdev_gendev);
@@ -644,8 +641,6 @@
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_unregister_device(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
-out:
-	up(&shost->scan_mutex);
 }
 EXPORT_SYMBOL(scsi_remove_device);
 
@@ -653,17 +648,20 @@
 {
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
-	struct scsi_device *sdev, *tmp;
+	struct scsi_device *sdev;
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	starget->reap_ref++;
-	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+restart:
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
 		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id)
+		    sdev->id != starget->id ||
+		    sdev->sdev_state == SDEV_DEL)
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
 		spin_lock_irqsave(shost->host_lock, flags);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_target_reap(starget);

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Brian King]

Alan Stern wrote:
> 	Don't acquire the scan_mutex in scsi_remove_device.  It's
> 	not needed since there's nothing wrong with removing devices
> 	during scanning, and it will cause deadlock under certain
> 	error conditions.  (I'm not certain about this either.  Will
> 	removing a device as it's being scanned cause a problem?
> 	Perhaps there should be a separate version of the routine
> 	that does acquire the scan_mutex.)

I actually submitted the patch to acquire the scan_mutex in scsi_remove_device
in order to fix an oops I ran into where a device was being deleted while it
was still being scanned. The actual oops was a NULL dentry in sysfs_remove_link.

I would much rather see a __scsi_remove_device API that scsi core and scsi_remove_device
can call that does not acquire the scan_mutex in the paths that make sense to
change. That way scsi_remove_device can maintain its existing behavior.

Brian


> 
> 	Make scsi_forget_host remove all devices, not all targets.
> 
> 	Make the loops to remove devices in scsi_forget_host and
> 	__scsi_remove_target restart from the beginning every time
> 	a device is removed (rather than using list_for_each_entry_safe,
> 	which is very definitely _unsafe_), and skip over devices
> 	that are already in the SDEV_DEL state.
> 
> This fixes several oopses I encountered during testing.  Do you think this 
> is ready to be applied?
> 
> Alan Stern
> 
> 
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> --- a/include/scsi/scsi_host.h	Mon Jun 13 14:57:26 2005
> +++ b/include/scsi/scsi_host.h	Fri Jun 17 22:19:09 2005
> @@ -411,6 +411,7 @@
>  	SHOST_DEL,
>  	SHOST_CANCEL,
>  	SHOST_RECOVERY,
> +	SHOST_REMOVE,
>  };
>  
>  struct Scsi_Host {
> --- a/drivers/scsi/hosts.c	Mon Jun 13 14:57:14 2005
> +++ b/drivers/scsi/hosts.c	Fri Jun 17 22:20:19 2005
> @@ -74,8 +74,13 @@
>   **/
>  void scsi_remove_host(struct Scsi_Host *shost)
>  {
> -	scsi_forget_host(shost);
> +	set_bit(SHOST_REMOVE, &shost->shost_state);
>  	scsi_host_cancel(shost, 0);
> +
> +	down(&shost->scan_mutex);	/* Wait for scanning to stop */
> +	up(&shost->scan_mutex);
> +
> +	scsi_forget_host(shost);
>  	scsi_proc_host_rm(shost);
>  
>  	set_bit(SHOST_DEL, &shost->shost_state);
> --- a/drivers/scsi/scsi_scan.c	Mon Jun 13 14:58:06 2005
> +++ b/drivers/scsi/scsi_scan.c	Fri Jun 17 22:59:52 2005
> @@ -843,8 +843,12 @@
>   out_free_sdev:
>  	if (res == SCSI_SCAN_LUN_PRESENT) {
>  		if (sdevp) {
> -			scsi_device_get(sdev);
> -			*sdevp = sdev;
> +			if (scsi_device_get(sdev) == 0) {
> +				*sdevp = sdev;
> +			} else {
> +				scsi_remove_device(sdev);
> +				res = SCSI_SCAN_NO_RESPONSE;
> +			}
>  		}
>  	} else {
>  		if (sdev->host->hostt->slave_destroy)
> @@ -1186,22 +1190,28 @@
>  				      uint id, uint lun, void *hostdata)
>  {
>  	struct scsi_device *sdev;
> -	struct device *parent = &shost->shost_gendev;
>  	int res;
> -	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
> +	struct scsi_target *starget;
>  
> -	if (!starget)
> -		return ERR_PTR(-ENOMEM);
> +	down(&shost->scan_mutex);
> +	if (test_bit(SHOST_REMOVE, &shost->shost_state)) {
> +		sdev = ERR_PTR(-ENODEV);
> +		goto out;
> +	}
> +	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
> +	if (!starget) {
> +		sdev = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}
>  
>  	get_device(&starget->dev);
> -	down(&shost->scan_mutex);
>  	res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
>  	if (res != SCSI_SCAN_LUN_PRESENT)
>  		sdev = ERR_PTR(-ENODEV);
> -	up(&shost->scan_mutex);
>  	scsi_target_reap(starget);
>  	put_device(&starget->dev);
> -
> +out:
> +	up(&shost->scan_mutex);
>  	return sdev;
>  }
>  EXPORT_SYMBOL(__scsi_add_device);
> @@ -1222,29 +1232,9 @@
>  }
>  EXPORT_SYMBOL(scsi_rescan_device);
>  
> -/**
> - * scsi_scan_target - scan a target id, possibly including all LUNs on the
> - *     target.
> - * @sdevsca:	Scsi_Device handle for scanning
> - * @shost:	host to scan
> - * @channel:	channel to scan
> - * @id:		target id to scan
> - *
> - * Description:
> - *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
> - *     0, and possibly all LUNs on the target id.
> - *
> - *     Use the pre-allocated @sdevscan as a handle for the scanning. This
> - *     function sets sdevscan->host, sdevscan->id and sdevscan->lun; the
> - *     scanning functions modify sdevscan->lun.
> - *
> - *     First try a REPORT LUN scan, if that does not scan the target, do a
> - *     sequential scan of LUNs on the target id.
> - **/
> -void scsi_scan_target(struct device *parent, unsigned int channel,
> -		      unsigned int id, unsigned int lun, int rescan)
> +static void __scsi_scan_target(struct Scsi_Host *shost, unsigned int channel,
> +		unsigned int id, unsigned int lun, int rescan)
>  {
> -	struct Scsi_Host *shost = dev_to_shost(parent);
>  	int bflags = 0;
>  	int res;
>  	struct scsi_device *sdev = NULL;
> @@ -1257,7 +1247,7 @@
>  		return;
>  
>  
> -	starget = scsi_alloc_target(parent, channel, id);
> +	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
>  
>  	if (!starget)
>  		return;
> @@ -1304,6 +1294,32 @@
>  
>  	put_device(&starget->dev);
>  }
> +
> +/**
> + * scsi_scan_target - scan a target id, possibly including all LUNs on the
> + *     target.
> + * @parent:	host to scan
> + * @channel:	channel to scan
> + * @id:		target id to scan
> + * @rescan:	passed to LUN scanning routines
> + *
> + * Description:
> + *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
> + *     0, and possibly all LUNs on the target id.
> + *
> + *     First try a REPORT LUN scan, if that does not scan the target, do a
> + *     sequential scan of LUNs on the target id.
> + **/
> +void scsi_scan_target(struct device *parent, unsigned int channel,
> +		      unsigned int id, unsigned int lun, int rescan)
> +{
> +	struct Scsi_Host *shost = dev_to_shost(parent);
> +
> +	down(&shost->scan_mutex);
> +	if (!test_bit(SHOST_REMOVE, &shost->shost_state))
> +		__scsi_scan_target(shost, channel, id, lun, rescan);
> +	up(&shost->scan_mutex);
> +}
>  EXPORT_SYMBOL(scsi_scan_target);
>  
>  static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
> @@ -1329,10 +1345,10 @@
>  				order_id = shost->max_id - id - 1;
>  			else
>  				order_id = id;
> -			scsi_scan_target(&shost->shost_gendev, channel, order_id, lun, rescan);
> +			__scsi_scan_target(shost, channel, order_id, lun, rescan);
>  		}
>  	else
> -		scsi_scan_target(&shost->shost_gendev, channel, id, lun, rescan);
> +		__scsi_scan_target(shost, channel, id, lun, rescan);
>  }
>  
>  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> @@ -1347,11 +1363,14 @@
>  		return -EINVAL;
>  
>  	down(&shost->scan_mutex);
> +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> +		goto out;
>  	if (channel == SCAN_WILD_CARD) 
>  		for (channel = 0; channel <= shost->max_channel; channel++)
>  			scsi_scan_channel(shost, channel, id, lun, rescan);
>  	else
>  		scsi_scan_channel(shost, channel, id, lun, rescan);
> +out:
>  	up(&shost->scan_mutex);
>  
>  	return 0;
> @@ -1383,23 +1402,17 @@
>  
>  void scsi_forget_host(struct Scsi_Host *shost)
>  {
> -	struct scsi_target *starget, *tmp;
> +	struct scsi_device *sdev;
>  	unsigned long flags;
>  
> -	/*
> -	 * Ok, this look a bit strange.  We always look for the first device
> -	 * on the list as scsi_remove_device removes them from it - thus we
> -	 * also have to release the lock.
> -	 * We don't need to get another reference to the device before
> -	 * releasing the lock as we already own the reference from
> -	 * scsi_register_device that's release in scsi_remove_device.  And
> -	 * after that we don't look at sdev anymore.
> -	 */
> +restart:
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
> +	list_for_each_entry(sdev, &shost->__devices, siblings) {
> +		if (sdev->sdev_state == SDEV_DEL)
> +			continue;
>  		spin_unlock_irqrestore(shost->host_lock, flags);
> -		scsi_remove_target(&starget->dev);
> -		spin_lock_irqsave(shost->host_lock, flags);
> +		scsi_remove_device(sdev);
> +		goto restart;
>  	}
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  }
> @@ -1426,12 +1439,16 @@
>   */
>  struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
>  {
> -	struct scsi_device *sdev;
> +	struct scsi_device *sdev = NULL;
>  	struct scsi_target *starget;
>  
> +	down(&shost->scan_mutex);
> +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> +		goto out;
> +
>  	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
>  	if (!starget)
> -		return NULL;
> +		goto out;
>  
>  	sdev = scsi_alloc_sdev(starget, 0, NULL);
>  	if (sdev) {
> @@ -1439,6 +1456,8 @@
>  		sdev->borken = 0;
>  	}
>  	put_device(&starget->dev);
> +out:
> +	up(&shost->scan_mutex);
>  	return sdev;
>  }
>  EXPORT_SYMBOL(scsi_get_host_dev);
> --- a/drivers/scsi/scsi_sysfs.c	Fri Jun 17 22:23:37 2005
> +++ b/drivers/scsi/scsi_sysfs.c	Fri Jun 17 22:25:18 2005
> @@ -631,11 +631,8 @@
>   **/
>  void scsi_remove_device(struct scsi_device *sdev)
>  {
> -	struct Scsi_Host *shost = sdev->host;
> -
> -	down(&shost->scan_mutex);
>  	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> -		goto out;
> +		return;
>  
>  	class_device_unregister(&sdev->sdev_classdev);
>  	device_del(&sdev->sdev_gendev);
> @@ -644,8 +641,6 @@
>  		sdev->host->hostt->slave_destroy(sdev);
>  	transport_unregister_device(&sdev->sdev_gendev);
>  	put_device(&sdev->sdev_gendev);
> -out:
> -	up(&shost->scan_mutex);
>  }
>  EXPORT_SYMBOL(scsi_remove_device);
>  
> @@ -653,17 +648,20 @@
>  {
>  	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>  	unsigned long flags;
> -	struct scsi_device *sdev, *tmp;
> +	struct scsi_device *sdev;
>  
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	starget->reap_ref++;
> -	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
> +restart:
> +	list_for_each_entry(sdev, &shost->__devices, siblings) {
>  		if (sdev->channel != starget->channel ||
> -		    sdev->id != starget->id)
> +		    sdev->id != starget->id ||
> +		    sdev->sdev_state == SDEV_DEL)
>  			continue;
>  		spin_unlock_irqrestore(shost->host_lock, flags);
>  		scsi_remove_device(sdev);
>  		spin_lock_irqsave(shost->host_lock, flags);
> +		goto restart;
>  	}
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  	scsi_target_reap(starget);
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Mon, 20 Jun 2005, Brian King wrote:

> Alan Stern wrote:
> > 	Don't acquire the scan_mutex in scsi_remove_device.  It's
> > 	not needed since there's nothing wrong with removing devices
> > 	during scanning, and it will cause deadlock under certain
> > 	error conditions.  (I'm not certain about this either.  Will
> > 	removing a device as it's being scanned cause a problem?
> > 	Perhaps there should be a separate version of the routine
> > 	that does acquire the scan_mutex.)
> 
> I actually submitted the patch to acquire the scan_mutex in scsi_remove_device
> in order to fix an oops I ran into where a device was being deleted while it
> was still being scanned. The actual oops was a NULL dentry in sysfs_remove_link.
> 
> I would much rather see a __scsi_remove_device API that scsi core and scsi_remove_device
> can call that does not acquire the scan_mutex in the paths that make sense to
> change. That way scsi_remove_device can maintain its existing behavior.

Okay, I can see you would run into problems if scsi_remove_device was 
called after the device structure had been initialized but before it was 
registered in sysfs.  (I'm not sure exactly how that could happen... but 
never mind.)

Here's a revised patch that does what Brian suggested.

Alan Stern


Index: usb-2.6/include/scsi/scsi_host.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_host.h
+++ usb-2.6/include/scsi/scsi_host.h
@@ -411,6 +411,7 @@ enum {
 	SHOST_DEL,
 	SHOST_CANCEL,
 	SHOST_RECOVERY,
+	SHOST_REMOVE,
 };
 
 struct Scsi_Host {
Index: usb-2.6/drivers/scsi/hosts.c
===================================================================
--- usb-2.6.orig/drivers/scsi/hosts.c
+++ usb-2.6/drivers/scsi/hosts.c
@@ -74,8 +74,13 @@ void scsi_host_cancel(struct Scsi_Host *
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	scsi_forget_host(shost);
+	set_bit(SHOST_REMOVE, &shost->shost_state);
 	scsi_host_cancel(shost, 0);
+
+	down(&shost->scan_mutex);	/* Wait for scanning to stop */
+	up(&shost->scan_mutex);
+
+	scsi_forget_host(shost);
 	scsi_proc_host_rm(shost);
 
 	set_bit(SHOST_DEL, &shost->shost_state);
Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -843,8 +843,12 @@ static int scsi_probe_and_add_lun(struct
  out_free_sdev:
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (sdevp) {
-			scsi_device_get(sdev);
-			*sdevp = sdev;
+			if (scsi_device_get(sdev) == 0) {
+				*sdevp = sdev;
+			} else {
+				__scsi_remove_device(sdev);
+				res = SCSI_SCAN_NO_RESPONSE;
+			}
 		}
 	} else {
 		if (sdev->host->hostt->slave_destroy)
@@ -1186,22 +1190,28 @@ struct scsi_device *__scsi_add_device(st
 				      uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
-	struct device *parent = &shost->shost_gendev;
 	int res;
-	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
+	struct scsi_target *starget;
 
-	if (!starget)
-		return ERR_PTR(-ENOMEM);
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state)) {
+		sdev = ERR_PTR(-ENODEV);
+		goto out;
+	}
+	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
+	if (!starget) {
+		sdev = ERR_PTR(-ENOMEM);
+		goto out;
+	}
 
 	get_device(&starget->dev);
-	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
-	up(&shost->scan_mutex);
 	scsi_target_reap(starget);
 	put_device(&starget->dev);
-
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(__scsi_add_device);
@@ -1222,29 +1232,9 @@ void scsi_rescan_device(struct device *d
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-/**
- * scsi_scan_target - scan a target id, possibly including all LUNs on the
- *     target.
- * @sdevsca:	Scsi_Device handle for scanning
- * @shost:	host to scan
- * @channel:	channel to scan
- * @id:		target id to scan
- *
- * Description:
- *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
- *     0, and possibly all LUNs on the target id.
- *
- *     Use the pre-allocated @sdevscan as a handle for the scanning. This
- *     function sets sdevscan->host, sdevscan->id and sdevscan->lun; the
- *     scanning functions modify sdevscan->lun.
- *
- *     First try a REPORT LUN scan, if that does not scan the target, do a
- *     sequential scan of LUNs on the target id.
- **/
-void scsi_scan_target(struct device *parent, unsigned int channel,
-		      unsigned int id, unsigned int lun, int rescan)
+static void __scsi_scan_target(struct Scsi_Host *shost, unsigned int channel,
+		unsigned int id, unsigned int lun, int rescan)
 {
-	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
 	int res;
 	struct scsi_device *sdev = NULL;
@@ -1257,7 +1247,7 @@ void scsi_scan_target(struct device *par
 		return;
 
 
-	starget = scsi_alloc_target(parent, channel, id);
+	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
 
 	if (!starget)
 		return;
@@ -1304,6 +1294,32 @@ void scsi_scan_target(struct device *par
 
 	put_device(&starget->dev);
 }
+
+/**
+ * scsi_scan_target - scan a target id, possibly including all LUNs on the
+ *     target.
+ * @parent:	host to scan
+ * @channel:	channel to scan
+ * @id:		target id to scan
+ * @rescan:	passed to LUN scanning routines
+ *
+ * Description:
+ *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
+ *     0, and possibly all LUNs on the target id.
+ *
+ *     First try a REPORT LUN scan, if that does not scan the target, do a
+ *     sequential scan of LUNs on the target id.
+ **/
+void scsi_scan_target(struct device *parent, unsigned int channel,
+		      unsigned int id, unsigned int lun, int rescan)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+
+	down(&shost->scan_mutex);
+	if (!test_bit(SHOST_REMOVE, &shost->shost_state))
+		__scsi_scan_target(shost, channel, id, lun, rescan);
+	up(&shost->scan_mutex);
+}
 EXPORT_SYMBOL(scsi_scan_target);
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
@@ -1329,10 +1345,10 @@ static void scsi_scan_channel(struct Scs
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			scsi_scan_target(&shost->shost_gendev, channel, order_id, lun, rescan);
+			__scsi_scan_target(shost, channel, order_id, lun, rescan);
 		}
 	else
-		scsi_scan_target(&shost->shost_gendev, channel, id, lun, rescan);
+		__scsi_scan_target(shost, channel, id, lun, rescan);
 }
 
 int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
@@ -1347,11 +1363,14 @@ int scsi_scan_host_selected(struct Scsi_
 		return -EINVAL;
 
 	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
 	if (channel == SCAN_WILD_CARD) 
 		for (channel = 0; channel <= shost->max_channel; channel++)
 			scsi_scan_channel(shost, channel, id, lun, rescan);
 	else
 		scsi_scan_channel(shost, channel, id, lun, rescan);
+out:
 	up(&shost->scan_mutex);
 
 	return 0;
@@ -1383,23 +1402,17 @@ EXPORT_SYMBOL(scsi_scan_single_target);
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct scsi_target *starget, *tmp;
+	struct scsi_device *sdev;
 	unsigned long flags;
 
-	/*
-	 * Ok, this look a bit strange.  We always look for the first device
-	 * on the list as scsi_remove_device removes them from it - thus we
-	 * also have to release the lock.
-	 * We don't need to get another reference to the device before
-	 * releasing the lock as we already own the reference from
-	 * scsi_register_device that's release in scsi_remove_device.  And
-	 * after that we don't look at sdev anymore.
-	 */
+restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state == SDEV_DEL)
+			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
-		scsi_remove_target(&starget->dev);
-		spin_lock_irqsave(shost->host_lock, flags);
+		__scsi_remove_device(sdev);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
@@ -1426,12 +1439,16 @@ void scsi_forget_host(struct Scsi_Host *
  */
 struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct scsi_target *starget;
 
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
+
 	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
 	if (!starget)
-		return NULL;
+		goto out;
 
 	sdev = scsi_alloc_sdev(starget, 0, NULL);
 	if (sdev) {
@@ -1439,6 +1456,8 @@ struct scsi_device *scsi_get_host_dev(st
 		sdev->borken = 0;
 	}
 	put_device(&starget->dev);
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(scsi_get_host_dev);
Index: usb-2.6/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
+++ usb-2.6/drivers/scsi/scsi_sysfs.c
@@ -591,7 +591,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 			error = attr_add(&sdev->sdev_gendev,
 					sdev->host->hostt->sdev_attrs[i]);
 			if (error) {
-				scsi_remove_device(sdev);
+				__scsi_remove_device(sdev);
 				goto out;
 			}
 		}
@@ -605,7 +605,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 							scsi_sysfs_sdev_attrs[i]);
 			error = device_create_file(&sdev->sdev_gendev, attr);
 			if (error) {
-				scsi_remove_device(sdev);
+				__scsi_remove_device(sdev);
 				goto out;
 			}
 		}
@@ -625,17 +625,10 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	return error;
 }
 
-/**
- * scsi_remove_device - unregister a device from the scsi bus
- * @sdev:	scsi_device to unregister
- **/
-void scsi_remove_device(struct scsi_device *sdev)
+void __scsi_remove_device(struct scsi_device *sdev)
 {
-	struct Scsi_Host *shost = sdev->host;
-
-	down(&shost->scan_mutex);
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		goto out;
+		return;
 
 	class_device_unregister(&sdev->sdev_classdev);
 	device_del(&sdev->sdev_gendev);
@@ -644,8 +637,17 @@ void scsi_remove_device(struct scsi_devi
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_unregister_device(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
-out:
-	up(&shost->scan_mutex);
+}
+
+/**
+ * scsi_remove_device - unregister a device from the scsi bus
+ * @sdev:	scsi_device to unregister
+ **/
+void scsi_remove_device(struct scsi_device *sdev)
+{
+	down(&sdev->host->scan_mutex);
+	__scsi_remove_device(sdev);
+	up(&sdev->host->scan_mutex);
 }
 EXPORT_SYMBOL(scsi_remove_device);
 
@@ -653,17 +655,20 @@ void __scsi_remove_target(struct scsi_ta
 {
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
-	struct scsi_device *sdev, *tmp;
+	struct scsi_device *sdev;
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	starget->reap_ref++;
-	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+restart:
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
 		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id)
+		    sdev->id != starget->id ||
+		    sdev->sdev_state == SDEV_DEL)
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
 		spin_lock_irqsave(shost->host_lock, flags);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_target_reap(starget);
Index: usb-2.6/drivers/scsi/scsi_priv.h
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_priv.h
+++ usb-2.6/drivers/scsi/scsi_priv.h
@@ -144,6 +144,7 @@ extern void scsi_sysfs_unregister(void);
 extern void scsi_sysfs_device_initialize(struct scsi_device *);
 extern int scsi_sysfs_target_initialize(struct scsi_device *);
 extern struct scsi_transport_template blank_transport_template;
+extern void __scsi_remove_device(struct scsi_device *);
 
 extern struct class sdev_class;
 extern struct bus_type scsi_bus_type;

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Patrick Mansfield]

Hi Alan -

On Mon, Jun 20, 2005 at 12:35:02PM -0400, Alan Stern wrote:

> -/**
> - * scsi_scan_target - scan a target id, possibly including all LUNs on the
> - *     target.
> - * @sdevsca:	Scsi_Device handle for scanning
> - * @shost:	host to scan
> - * @channel:	channel to scan
> - * @id:		target id to scan
> - *
> - * Description:
> - *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
> - *     0, and possibly all LUNs on the target id.
> - *
> - *     Use the pre-allocated @sdevscan as a handle for the scanning. This
> - *     function sets sdevscan->host, sdevscan->id and sdevscan->lun; the
> - *     scanning functions modify sdevscan->lun.
> - *
> - *     First try a REPORT LUN scan, if that does not scan the target, do a
> - *     sequential scan of LUNs on the target id.
> - **/
> -void scsi_scan_target(struct device *parent, unsigned int channel,
> -		      unsigned int id, unsigned int lun, int rescan)
> +static void __scsi_scan_target(struct Scsi_Host *shost, unsigned int channel,
> +		unsigned int id, unsigned int lun, int rescan)
>  {
> -	struct Scsi_Host *shost = dev_to_shost(parent);
>  	int bflags = 0;
>  	int res;
>  	struct scsi_device *sdev = NULL;
> @@ -1257,7 +1247,7 @@ void scsi_scan_target(struct device *par
>  		return;
>  
>  
> -	starget = scsi_alloc_target(parent, channel, id);
> +	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);

For FC (and iSCSI), parent != &shost->shost_gendev. See user scanning
on FC thread / patches. 

[kernel scsi]$ grep scsi_scan_target scsi_transport_fc.c
	scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,

And &rport->dev is not &shost->shost_gendev :-(

> +
> +/**
> + * scsi_scan_target - scan a target id, possibly including all LUNs on the
> + *     target.
> + * @parent:	host to scan
> + * @channel:	channel to scan
> + * @id:		target id to scan
> + * @rescan:	passed to LUN scanning routines
> + *
> + * Description:
> + *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
> + *     0, and possibly all LUNs on the target id.
> + *
> + *     First try a REPORT LUN scan, if that does not scan the target, do a
> + *     sequential scan of LUNs on the target id.
> + **/
> +void scsi_scan_target(struct device *parent, unsigned int channel,
> +		      unsigned int id, unsigned int lun, int rescan)
> +{
> +	struct Scsi_Host *shost = dev_to_shost(parent);
> +
> +	down(&shost->scan_mutex);
> +	if (!test_bit(SHOST_REMOVE, &shost->shost_state))
> +		__scsi_scan_target(shost, channel, id, lun, rescan);

And so parent still has to be passed down to __scsi_scan_target.

-- Patrick Mansfield

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Mon, 20 Jun 2005, Patrick Mansfield wrote:

> > -	starget = scsi_alloc_target(parent, channel, id);
> > +	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
> 
> For FC (and iSCSI), parent != &shost->shost_gendev. See user scanning
> on FC thread / patches. 
> 
> [kernel scsi]$ grep scsi_scan_target scsi_transport_fc.c
> 	scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
> 
> And &rport->dev is not &shost->shost_gendev :-(

> And so parent still has to be passed down to __scsi_scan_target.

Okay.  It's a good thing you guys are able to find the bugs I've
inadvertently introduced.  Here's the next revision of the patch.  
Patrick, you may be able to suggest a better kerneldoc explanation for the
"parent" parameter in scsi_scan_target.  (The kerneldoc there now is way
out of date!)

Alan Stern



Index: usb-2.6/include/scsi/scsi_host.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_host.h
+++ usb-2.6/include/scsi/scsi_host.h
@@ -411,6 +411,7 @@ enum {
 	SHOST_DEL,
 	SHOST_CANCEL,
 	SHOST_RECOVERY,
+	SHOST_REMOVE,
 };
 
 struct Scsi_Host {
Index: usb-2.6/drivers/scsi/hosts.c
===================================================================
--- usb-2.6.orig/drivers/scsi/hosts.c
+++ usb-2.6/drivers/scsi/hosts.c
@@ -74,8 +74,13 @@ void scsi_host_cancel(struct Scsi_Host *
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	scsi_forget_host(shost);
+	set_bit(SHOST_REMOVE, &shost->shost_state);
 	scsi_host_cancel(shost, 0);
+
+	down(&shost->scan_mutex);	/* Wait for scanning to stop */
+	up(&shost->scan_mutex);
+
+	scsi_forget_host(shost);
 	scsi_proc_host_rm(shost);
 
 	set_bit(SHOST_DEL, &shost->shost_state);
Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -843,8 +843,12 @@ static int scsi_probe_and_add_lun(struct
  out_free_sdev:
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (sdevp) {
-			scsi_device_get(sdev);
-			*sdevp = sdev;
+			if (scsi_device_get(sdev) == 0) {
+				*sdevp = sdev;
+			} else {
+				__scsi_remove_device(sdev);
+				res = SCSI_SCAN_NO_RESPONSE;
+			}
 		}
 	} else {
 		if (sdev->host->hostt->slave_destroy)
@@ -1186,22 +1190,28 @@ struct scsi_device *__scsi_add_device(st
 				      uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
-	struct device *parent = &shost->shost_gendev;
 	int res;
-	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
+	struct scsi_target *starget;
 
-	if (!starget)
-		return ERR_PTR(-ENOMEM);
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state)) {
+		sdev = ERR_PTR(-ENODEV);
+		goto out;
+	}
+	starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
+	if (!starget) {
+		sdev = ERR_PTR(-ENOMEM);
+		goto out;
+	}
 
 	get_device(&starget->dev);
-	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
-	up(&shost->scan_mutex);
 	scsi_target_reap(starget);
 	put_device(&starget->dev);
-
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(__scsi_add_device);
@@ -1222,27 +1232,8 @@ void scsi_rescan_device(struct device *d
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-/**
- * scsi_scan_target - scan a target id, possibly including all LUNs on the
- *     target.
- * @sdevsca:	Scsi_Device handle for scanning
- * @shost:	host to scan
- * @channel:	channel to scan
- * @id:		target id to scan
- *
- * Description:
- *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
- *     0, and possibly all LUNs on the target id.
- *
- *     Use the pre-allocated @sdevscan as a handle for the scanning. This
- *     function sets sdevscan->host, sdevscan->id and sdevscan->lun; the
- *     scanning functions modify sdevscan->lun.
- *
- *     First try a REPORT LUN scan, if that does not scan the target, do a
- *     sequential scan of LUNs on the target id.
- **/
-void scsi_scan_target(struct device *parent, unsigned int channel,
-		      unsigned int id, unsigned int lun, int rescan)
+static void __scsi_scan_target(struct device *parent, unsigned int channel,
+		unsigned int id, unsigned int lun, int rescan)
 {
 	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
@@ -1256,9 +1247,7 @@ void scsi_scan_target(struct device *par
 		 */
 		return;
 
-
 	starget = scsi_alloc_target(parent, channel, id);
-
 	if (!starget)
 		return;
 
@@ -1304,6 +1293,32 @@ void scsi_scan_target(struct device *par
 
 	put_device(&starget->dev);
 }
+
+/**
+ * scsi_scan_target - scan a target id, possibly including all LUNs on the
+ *     target.
+ * @parent:	host to scan
+ * @channel:	channel to scan
+ * @id:		target id to scan
+ * @rescan:	passed to LUN scanning routines
+ *
+ * Description:
+ *     Scan the target id on @shost, @channel, and @id. Scan at least LUN
+ *     0, and possibly all LUNs on the target id.
+ *
+ *     First try a REPORT LUN scan, if that does not scan the target, do a
+ *     sequential scan of LUNs on the target id.
+ **/
+void scsi_scan_target(struct device *parent, unsigned int channel,
+		      unsigned int id, unsigned int lun, int rescan)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+
+	down(&shost->scan_mutex);
+	if (!test_bit(SHOST_REMOVE, &shost->shost_state))
+		__scsi_scan_target(parent, channel, id, lun, rescan);
+	up(&shost->scan_mutex);
+}
 EXPORT_SYMBOL(scsi_scan_target);
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
@@ -1329,10 +1344,12 @@ static void scsi_scan_channel(struct Scs
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			scsi_scan_target(&shost->shost_gendev, channel, order_id, lun, rescan);
+			__scsi_scan_target(&shost->shost_gendev, channel,
+					order_id, lun, rescan);
 		}
 	else
-		scsi_scan_target(&shost->shost_gendev, channel, id, lun, rescan);
+		__scsi_scan_target(&shost->shost_gendev, channel,
+				id, lun, rescan);
 }
 
 int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
@@ -1347,11 +1364,14 @@ int scsi_scan_host_selected(struct Scsi_
 		return -EINVAL;
 
 	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
 	if (channel == SCAN_WILD_CARD) 
 		for (channel = 0; channel <= shost->max_channel; channel++)
 			scsi_scan_channel(shost, channel, id, lun, rescan);
 	else
 		scsi_scan_channel(shost, channel, id, lun, rescan);
+out:
 	up(&shost->scan_mutex);
 
 	return 0;
@@ -1383,23 +1403,17 @@ EXPORT_SYMBOL(scsi_scan_single_target);
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct scsi_target *starget, *tmp;
+	struct scsi_device *sdev;
 	unsigned long flags;
 
-	/*
-	 * Ok, this look a bit strange.  We always look for the first device
-	 * on the list as scsi_remove_device removes them from it - thus we
-	 * also have to release the lock.
-	 * We don't need to get another reference to the device before
-	 * releasing the lock as we already own the reference from
-	 * scsi_register_device that's release in scsi_remove_device.  And
-	 * after that we don't look at sdev anymore.
-	 */
+restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (sdev->sdev_state == SDEV_DEL)
+			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
-		scsi_remove_target(&starget->dev);
-		spin_lock_irqsave(shost->host_lock, flags);
+		__scsi_remove_device(sdev);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
@@ -1426,12 +1440,16 @@ void scsi_forget_host(struct Scsi_Host *
  */
 struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct scsi_target *starget;
 
+	down(&shost->scan_mutex);
+	if (test_bit(SHOST_REMOVE, &shost->shost_state))
+		goto out;
+
 	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
 	if (!starget)
-		return NULL;
+		goto out;
 
 	sdev = scsi_alloc_sdev(starget, 0, NULL);
 	if (sdev) {
@@ -1439,6 +1457,8 @@ struct scsi_device *scsi_get_host_dev(st
 		sdev->borken = 0;
 	}
 	put_device(&starget->dev);
+out:
+	up(&shost->scan_mutex);
 	return sdev;
 }
 EXPORT_SYMBOL(scsi_get_host_dev);
Index: usb-2.6/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
+++ usb-2.6/drivers/scsi/scsi_sysfs.c
@@ -591,7 +591,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 			error = attr_add(&sdev->sdev_gendev,
 					sdev->host->hostt->sdev_attrs[i]);
 			if (error) {
-				scsi_remove_device(sdev);
+				__scsi_remove_device(sdev);
 				goto out;
 			}
 		}
@@ -605,7 +605,7 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 							scsi_sysfs_sdev_attrs[i]);
 			error = device_create_file(&sdev->sdev_gendev, attr);
 			if (error) {
-				scsi_remove_device(sdev);
+				__scsi_remove_device(sdev);
 				goto out;
 			}
 		}
@@ -625,17 +625,10 @@ int scsi_sysfs_add_sdev(struct scsi_devi
 	return error;
 }
 
-/**
- * scsi_remove_device - unregister a device from the scsi bus
- * @sdev:	scsi_device to unregister
- **/
-void scsi_remove_device(struct scsi_device *sdev)
+void __scsi_remove_device(struct scsi_device *sdev)
 {
-	struct Scsi_Host *shost = sdev->host;
-
-	down(&shost->scan_mutex);
 	if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-		goto out;
+		return;
 
 	class_device_unregister(&sdev->sdev_classdev);
 	device_del(&sdev->sdev_gendev);
@@ -644,8 +637,17 @@ void scsi_remove_device(struct scsi_devi
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_unregister_device(&sdev->sdev_gendev);
 	put_device(&sdev->sdev_gendev);
-out:
-	up(&shost->scan_mutex);
+}
+
+/**
+ * scsi_remove_device - unregister a device from the scsi bus
+ * @sdev:	scsi_device to unregister
+ **/
+void scsi_remove_device(struct scsi_device *sdev)
+{
+	down(&sdev->host->scan_mutex);
+	__scsi_remove_device(sdev);
+	up(&sdev->host->scan_mutex);
 }
 EXPORT_SYMBOL(scsi_remove_device);
 
@@ -653,17 +655,20 @@ void __scsi_remove_target(struct scsi_ta
 {
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
-	struct scsi_device *sdev, *tmp;
+	struct scsi_device *sdev;
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	starget->reap_ref++;
-	list_for_each_entry_safe(sdev, tmp, &shost->__devices, siblings) {
+restart:
+	list_for_each_entry(sdev, &shost->__devices, siblings) {
 		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id)
+		    sdev->id != starget->id ||
+		    sdev->sdev_state == SDEV_DEL)
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_remove_device(sdev);
 		spin_lock_irqsave(shost->host_lock, flags);
+		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_target_reap(starget);
Index: usb-2.6/drivers/scsi/scsi_priv.h
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_priv.h
+++ usb-2.6/drivers/scsi/scsi_priv.h
@@ -144,6 +144,7 @@ extern void scsi_sysfs_unregister(void);
 extern void scsi_sysfs_device_initialize(struct scsi_device *);
 extern int scsi_sysfs_target_initialize(struct scsi_device *);
 extern struct scsi_transport_template blank_transport_template;
+extern void __scsi_remove_device(struct scsi_device *);
 
 extern struct class sdev_class;
 extern struct bus_type scsi_bus_type;

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> James and Mike:
> 
> Here's my patch (as529), very lightly tested.  Among the changes:
> 

I ran the updated version of your patch on a scsi-misc-2.6 kernel with an
Emulex and Qlogic adapter. It does not find any devices of these adapters
during scan. I need to look into this.

> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> --- a/include/scsi/scsi_host.h	Mon Jun 13 14:57:26 2005
> +++ b/include/scsi/scsi_host.h	Fri Jun 17 22:19:09 2005
> @@ -411,6 +411,7 @@
>  	SHOST_DEL,
>  	SHOST_CANCEL,
>  	SHOST_RECOVERY,
> +	SHOST_REMOVE,
>  };

Well it is a larger change we should consider migrating the shost state
model to one like the scsi device uses and also align on similar states
if possible.

>  
>  struct Scsi_Host {
> --- a/drivers/scsi/hosts.c	Mon Jun 13 14:57:14 2005
> +++ b/drivers/scsi/hosts.c	Fri Jun 17 22:20:19 2005
> @@ -74,8 +74,13 @@
>   **/
>  void scsi_remove_host(struct Scsi_Host *shost)
>  {
> -	scsi_forget_host(shost);
> +	set_bit(SHOST_REMOVE, &shost->shost_state);
>  	scsi_host_cancel(shost, 0);
> +
> +	down(&shost->scan_mutex);	/* Wait for scanning to stop */
> +	up(&shost->scan_mutex);
> +
> +	scsi_forget_host(shost);
>  	scsi_proc_host_rm(shost);
>  
>  	set_bit(SHOST_DEL, &shost->shost_state);

Moving scsi_forget_host to after scsi_host_cancel will cause the sd cache
flush routines to fail. Also as previously mentioned I would like to
understand if we still need the cancel functionality. I believe there are
end case races with cancel and LLDD really should be able to return all
commands prior to calling scsi_remove_host or post (prior to the last
shost put).

The bit is set to SHOST_REMOVE then scsi_host_cancel is called which will
set the bit SHOST_CANCEL. Later on scan is stopped only if state is
SHOST_REMOVE. Is that what you wanted?

>  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> @@ -1347,11 +1363,14 @@
>  		return -EINVAL;
>  
>  	down(&shost->scan_mutex);
> +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> +		goto out;
>  	if (channel == SCAN_WILD_CARD) 
>  		for (channel = 0; channel <= shost->max_channel; channel++)
>  			scsi_scan_channel(shost, channel, id, lun, rescan);
>  	else
>  		scsi_scan_channel(shost, channel, id, lun, rescan);
> +out:
>  	up(&shost->scan_mutex);
>  
>  	return 0;

It might be better to have a wrapper function so if we change the cases
where we would allow scanning we can change just one place. Also we might
cover more states if we reverse the logic on the check and look for the
case we allow scanning (see previous comment about cancel). This is what I
did in my previous patch.

> @@ -1383,23 +1402,17 @@
>  
>  void scsi_forget_host(struct Scsi_Host *shost)
>  {
> -	struct scsi_target *starget, *tmp;
> +	struct scsi_device *sdev;
>  	unsigned long flags;
>  
> -	/*
> -	 * Ok, this look a bit strange.  We always look for the first device
> -	 * on the list as scsi_remove_device removes them from it - thus we
> -	 * also have to release the lock.
> -	 * We don't need to get another reference to the device before
> -	 * releasing the lock as we already own the reference from
> -	 * scsi_register_device that's release in scsi_remove_device.  And
> -	 * after that we don't look at sdev anymore.
> -	 */
> +restart:
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
> +	list_for_each_entry(sdev, &shost->__devices, siblings) {
> +		if (sdev->sdev_state == SDEV_DEL)
> +			continue;
>  		spin_unlock_irqrestore(shost->host_lock, flags);
> -		scsi_remove_target(&starget->dev);
> -		spin_lock_irqsave(shost->host_lock, flags);
> +		scsi_remove_device(sdev);
> +		goto restart;
>  	}
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  }

Well it saves some some overhead we really should delete the parent
and let it handle cleanup of children. If we switch to using the driver
model lists the code would be easier to migrate.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Patrick Mansfield]

On Tue, Jun 21, 2005 at 10:12:09AM -0700, Mike Anderson wrote:
> Alan Stern [stern@rowland.harvard.edu] wrote:
> > James and Mike:
> > 
> > Here's my patch (as529), very lightly tested.  Among the changes:
> > 
> 
> I ran the updated version of your patch on a scsi-misc-2.6 kernel with an
> Emulex and Qlogic adapter. It does not find any devices of these adapters
> during scan. I need to look into this.

Mike -

You probably need Alan's most recent patch that passes through
scsi_scan_target's parent argument.

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson]

Patrick Mansfield [patmans@us.ibm.com] wrote:
> Mike -
> 
> You probably need Alan's most recent patch that passes through
> scsi_scan_target's parent argument.

Yes, thanks I thought that I was running the third update, but I was
running the second one. Running with third patch update fixes my scan
issue.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 21 Jun 2005, Mike Anderson wrote:

> > --- a/include/scsi/scsi_host.h	Mon Jun 13 14:57:26 2005
> > +++ b/include/scsi/scsi_host.h	Fri Jun 17 22:19:09 2005
> > @@ -411,6 +411,7 @@
> >  	SHOST_DEL,
> >  	SHOST_CANCEL,
> >  	SHOST_RECOVERY,
> > +	SHOST_REMOVE,
> >  };
> 
> Well it is a larger change we should consider migrating the shost state
> model to one like the scsi device uses and also align on similar states
> if possible.

For now I just wanted to make as simple a patch as possible.  I have no 
objection to making a larger change like the one you suggest.

> >  struct Scsi_Host {
> > --- a/drivers/scsi/hosts.c	Mon Jun 13 14:57:14 2005
> > +++ b/drivers/scsi/hosts.c	Fri Jun 17 22:20:19 2005
> > @@ -74,8 +74,13 @@
> >   **/
> >  void scsi_remove_host(struct Scsi_Host *shost)
> >  {
> > -	scsi_forget_host(shost);
> > +	set_bit(SHOST_REMOVE, &shost->shost_state);
> >  	scsi_host_cancel(shost, 0);
> > +
> > +	down(&shost->scan_mutex);	/* Wait for scanning to stop */
> > +	up(&shost->scan_mutex);
> > +
> > +	scsi_forget_host(shost);
> >  	scsi_proc_host_rm(shost);
> >  
> >  	set_bit(SHOST_DEL, &shost->shost_state);
> 
> Moving scsi_forget_host to after scsi_host_cancel will cause the sd cache
> flush routines to fail.

This objection runs up against an issue we discussed some time ago.  
Should the intended meaning of scsi_remove_host be simply that the kernel
needs to stop using the HBA reasonably soon?  In that case you are right.  
Or should the intended meaning be that the HBA is actually gone
(hot-unplugged) and all further attempts to use it will fail?  In that
case it doesn't matter.  The best ways to resolve this issue may be to
have a separate scsi_host_gone routine or to add an extra argument to
scsi_remove_host.

At any rate, we currently face the bigger problem that if scsi_forget_host
comes first then scsi_host_cancel doesn't work right.  Outstanding
commands do not get cancelled.  I didn't try to trace down the exact
reason for this; I just did the simplest thing to make it work.

>  Also as previously mentioned I would like to
> understand if we still need the cancel functionality. I believe there are
> end case races with cancel and LLDD really should be able to return all
> commands prior to calling scsi_remove_host or post (prior to the last
> shost put).

I rather agree in principle that the cancel functionality isn't really
needed.  Removing it will require some tricky changes to the LLDDs,
however.  And the changes will all have to be made at once.  If some LLDDs 
are changed and others aren't, then (depending on whether scsi_host_cancel 
has been removed) either the changed ones will oops as they try to cancel 
an already-cancelled command or the unchanged ones will oops as 
uncancelled commands time out.  I've seen both kinds of errors in working 
with usb-storage.

> The bit is set to SHOST_REMOVE then scsi_host_cancel is called which will
> set the bit SHOST_CANCEL. Later on scan is stopped only if state is
> SHOST_REMOVE. Is that what you wanted?

Remember, at the moment the state is a bit-vector.  It can have both
SHOST_CANCEL and SHOST_REMOVE set at the same time.  That is what I
wanted.  Changing to a host state model will of course require you to do
things differently.

> >  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> > @@ -1347,11 +1363,14 @@
> >  		return -EINVAL;
> >  
> >  	down(&shost->scan_mutex);
> > +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> > +		goto out;
> >  	if (channel == SCAN_WILD_CARD) 
> >  		for (channel = 0; channel <= shost->max_channel; channel++)
> >  			scsi_scan_channel(shost, channel, id, lun, rescan);
> >  	else
> >  		scsi_scan_channel(shost, channel, id, lun, rescan);
> > +out:
> >  	up(&shost->scan_mutex);
> >  
> >  	return 0;
> 
> It might be better to have a wrapper function so if we change the cases
> where we would allow scanning we can change just one place. Also we might
> cover more states if we reverse the logic on the check and look for the
> case we allow scanning (see previous comment about cancel). This is what I
> did in my previous patch.

That's okay with me.  So long as all the scanning pathways are covered and 
all scanning is stopped before scsi_forget_host runs, you can feel free to 
improve the implementation details.

> > @@ -1383,23 +1402,17 @@
> >  
> >  void scsi_forget_host(struct Scsi_Host *shost)
> >  {
> > -	struct scsi_target *starget, *tmp;
> > +	struct scsi_device *sdev;
> >  	unsigned long flags;
> >  
> > -	/*
> > -	 * Ok, this look a bit strange.  We always look for the first device
> > -	 * on the list as scsi_remove_device removes them from it - thus we
> > -	 * also have to release the lock.
> > -	 * We don't need to get another reference to the device before
> > -	 * releasing the lock as we already own the reference from
> > -	 * scsi_register_device that's release in scsi_remove_device.  And
> > -	 * after that we don't look at sdev anymore.
> > -	 */
> > +restart:
> >  	spin_lock_irqsave(shost->host_lock, flags);
> > -	list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
> > +	list_for_each_entry(sdev, &shost->__devices, siblings) {
> > +		if (sdev->sdev_state == SDEV_DEL)
> > +			continue;
> >  		spin_unlock_irqrestore(shost->host_lock, flags);
> > -		scsi_remove_target(&starget->dev);
> > -		spin_lock_irqsave(shost->host_lock, flags);
> > +		scsi_remove_device(sdev);
> > +		goto restart;
> >  	}
> >  	spin_unlock_irqrestore(shost->host_lock, flags);
> >  }
> 
> Well it saves some some overhead we really should delete the parent
> and let it handle cleanup of children. If we switch to using the driver
> model lists the code would be easier to migrate.

I didn't do it that way because it can't be made to work correctly with
the current code -- there's no way to know whether a target has already
been removed.  Adding a target state model would make your approach
feasible, but James has said that targets don't merit a state model.

Driver model klists also have their disadvantages.  If you delete a node 
from a klist asynchronously then you cannot re-use it; it must be allowed 
to deallocate itself when the refcount goes to 0.  And it's not possible 
to remove nodes from a klist synchronously while traversing the klist.

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Christoph Hellwig]

On Tue, Jun 21, 2005 at 04:04:06PM -0400, Alan Stern wrote:
> This objection runs up against an issue we discussed some time ago.  
> Should the intended meaning of scsi_remove_host be simply that the kernel
> needs to stop using the HBA reasonably soon?  In that case you are right.  
> Or should the intended meaning be that the HBA is actually gone
> (hot-unplugged) and all further attempts to use it will fail?  In that
> case it doesn't matter.  The best ways to resolve this issue may be to
> have a separate scsi_host_gone routine or to add an extra argument to
> scsi_remove_host.

It must mean both because we don't know whether a hot unplug happened or
not.  The ->remove callbacks don't tell us.

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 21 Jun 2005, Christoph Hellwig wrote:

> On Tue, Jun 21, 2005 at 04:04:06PM -0400, Alan Stern wrote:
> > This objection runs up against an issue we discussed some time ago.  
> > Should the intended meaning of scsi_remove_host be simply that the kernel
> > needs to stop using the HBA reasonably soon?  In that case you are right.  
> > Or should the intended meaning be that the HBA is actually gone
> > (hot-unplugged) and all further attempts to use it will fail?  In that
> > case it doesn't matter.  The best ways to resolve this issue may be to
> > have a separate scsi_host_gone routine or to add an extra argument to
> > scsi_remove_host.
> 
> It must mean both because we don't know whether a hot unplug happened or
> not.  The ->remove callbacks don't tell us.

I would describe it differently: Since you don't know whether a hot-unplug 
occurred, you might as well assume it did not.  There's no harm in this, 
because if the HBA really was unplugged then it doesn't matter what you 
do; everything will fail.

But those sd flush-cache commands are a problem.  Presumably you want to 
send them _after_ all the outstanding commands have finished or been 
cancelled.  What's the right way to allow those commands while rejecting 
all others?

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> On Tue, 21 Jun 2005, Christoph Hellwig wrote:
> 
> > On Tue, Jun 21, 2005 at 04:04:06PM -0400, Alan Stern wrote:
> > > This objection runs up against an issue we discussed some time ago.  
> > > Should the intended meaning of scsi_remove_host be simply that the kernel
> > > needs to stop using the HBA reasonably soon?  In that case you are right.  
> > > Or should the intended meaning be that the HBA is actually gone
> > > (hot-unplugged) and all further attempts to use it will fail?  In that
> > > case it doesn't matter.  The best ways to resolve this issue may be to
> > > have a separate scsi_host_gone routine or to add an extra argument to
> > > scsi_remove_host.
> > 
> > It must mean both because we don't know whether a hot unplug happened or
> > not.  The ->remove callbacks don't tell us.
> 
> I would describe it differently: Since you don't know whether a hot-unplug 
> occurred, you might as well assume it did not.  There's no harm in this, 
> because if the HBA really was unplugged then it doesn't matter what you 
> do; everything will fail.
> 

I have asked this multiple times, but I will again. If the hba knows to
fail everything by some internal state in the LLDD why does it not also
know to flush all commands back to the scsi mid layer in this unplugged
state so we do not have to deal with canceling them from the mid-layer. 

I think it has been stated many times if commands always go in through
queuecommand and always return through scsi_done it makes the chances of
errors a lot less.

> But those sd flush-cache commands are a problem.  Presumably you want to 
> send them _after_ all the outstanding commands have finished or been 
> cancelled.  What's the right way to allow those commands while rejecting 
> all others?

We have some notion of this already if you look in scsi_prep_fn (i.e.,
specials_only), but this is based on sdev state not shost. Checking for a
specials_only flag in scsi_dispatch may not be to clean. This may be why
we should look at a shost state model update and align sdev and shost to
clearly state what will happen to command.s

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson]

Alan Stern [stern@rowland.harvard.edu] wrote:
> > Moving scsi_forget_host to after scsi_host_cancel will cause the sd cache
> > flush routines to fail.
> 
> This objection runs up against an issue we discussed some time ago.  
> Should the intended meaning of scsi_remove_host be simply that the kernel
> needs to stop using the HBA reasonably soon?  In that case you are right.  
> Or should the intended meaning be that the HBA is actually gone
> (hot-unplugged) and all further attempts to use it will fail?  In that
> case it doesn't matter.  The best ways to resolve this issue may be to
> have a separate scsi_host_gone routine or to add an extra argument to
> scsi_remove_host.
> 

I believe this was discussed in the thread below or possible another (we
have had this conversation a number of times), and the action then was not
to create another interface.
http://marc.theaimsgroup.com/?t=108701426000002&r=1&w=2


> I rather agree in principle that the cancel functionality isn't really
> needed.  Removing it will require some tricky changes to the LLDDs,
> however.  And the changes will all have to be made at once.  If some LLDDs 
> are changed and others aren't, then (depending on whether scsi_host_cancel 
> has been removed) either the changed ones will oops as they try to cancel 
> an already-cancelled command or the unchanged ones will oops as 
> uncancelled commands time out.  I've seen both kinds of errors in working 
> with usb-storage.

We really should have scsi_times_out check the state of the host and
possible the device to stop calling into the host when removes are in
progress.

> 
> > The bit is set to SHOST_REMOVE then scsi_host_cancel is called which will
> > set the bit SHOST_CANCEL. Later on scan is stopped only if state is
> > SHOST_REMOVE. Is that what you wanted?
> 
> Remember, at the moment the state is a bit-vector.  It can have both
> SHOST_CANCEL and SHOST_REMOVE set at the same time.  That is what I
> wanted.  Changing to a host state model will of course require you to do
> things differently.

Yes, I guess I should know that :-(. I had my head in the new host state
model. Yes changing to the host state model did require some different
checks, but the concept is the same.

> 
> > >  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> > > @@ -1347,11 +1363,14 @@
> > >  		return -EINVAL;
> > >  
> > >  	down(&shost->scan_mutex);
> > > +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> > > +		goto out;
> > >  	if (channel == SCAN_WILD_CARD) 
> > >  		for (channel = 0; channel <= shost->max_channel; channel++)
> > >  			scsi_scan_channel(shost, channel, id, lun, rescan);
> > >  	else
> > >  		scsi_scan_channel(shost, channel, id, lun, rescan);
> > > +out:
> > >  	up(&shost->scan_mutex);
> > >  
> > >  	return 0;
> > 
> > It might be better to have a wrapper function so if we change the cases
> > where we would allow scanning we can change just one place. Also we might
> > cover more states if we reverse the logic on the check and look for the
> > case we allow scanning (see previous comment about cancel). This is what I
> > did in my previous patch.
> 
> That's okay with me.  So long as all the scanning pathways are covered and 
> all scanning is stopped before scsi_forget_host runs, you can feel free to 
> improve the implementation details.
> 

I already did this in the previous patch series I posted, but received not
comments so I guess there is no need to wrap it.

> 
> I didn't do it that way because it can't be made to work correctly with
> the current code -- there's no way to know whether a target has already
> been removed.  Adding a target state model would make your approach
> feasible, but James has said that targets don't merit a state model.
> 
> Driver model klists also have their disadvantages.  If you delete a node 
> from a klist asynchronously then you cannot re-use it; it must be allowed 
> to deallocate itself when the refcount goes to 0.  And it's not possible 
> to remove nodes from a klist synchronously while traversing the klist.

Is this still true for klists. I thought the locking updates and the
addition of a klist_iter was to fix this issue though I have not spent
much time looking through the code since the changes.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 21 Jun 2005, Mike Anderson wrote:

> I have asked this multiple times, but I will again. If the hba knows to
> fail everything by some internal state in the LLDD why does it not also
> know to flush all commands back to the scsi mid layer in this unplugged
> state so we do not have to deal with canceling them from the mid-layer. 

With usb-storage, part of it is my fault.  It never occurred to me that we
should fail all outstanding commands prior to calling scsi_remove_host,
since I expected the midlayer to take care of things.  I'll try changing
usb-storage and see if it works with scsi_host_cancel coming after
scsi_forget_host (or even removed entirely!).

> I think it has been stated many times if commands always go in through
> queuecommand and always return through scsi_done it makes the chances of
> errors a lot less.

I agree.  The current state must have arisen for historical reasons.  

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Tue, 21 Jun 2005, Mike Anderson wrote:

> > I rather agree in principle that the cancel functionality isn't really
> > needed.  Removing it will require some tricky changes to the LLDDs,
> > however.  And the changes will all have to be made at once.  If some LLDDs 
> > are changed and others aren't, then (depending on whether scsi_host_cancel 
> > has been removed) either the changed ones will oops as they try to cancel 
> > an already-cancelled command or the unchanged ones will oops as 
> > uncancelled commands time out.  I've seen both kinds of errors in working 
> > with usb-storage.
> 
> We really should have scsi_times_out check the state of the host and
> possible the device to stop calling into the host when removes are in
> progress.

Or when removes have been completed!

> > That's okay with me.  So long as all the scanning pathways are covered and 
> > all scanning is stopped before scsi_forget_host runs, you can feel free to 
> > improve the implementation details.
> > 
> 
> I already did this in the previous patch series I posted, but received not
> comments so I guess there is no need to wrap it.

I haven't had a chance to take a detailed look at your changes.  In fact I
wasn't even aware of them until yesterday (because I'm not subscribed to
linux-scsi).  I'll try them out when there's time.

> > Driver model klists also have their disadvantages.  If you delete a node 
> > from a klist asynchronously then you cannot re-use it; it must be allowed 
> > to deallocate itself when the refcount goes to 0.  And it's not possible 
> > to remove nodes from a klist synchronously while traversing the klist.
> 
> Is this still true for klists. I thought the locking updates and the
> addition of a klist_iter was to fix this issue though I have not spent
> much time looking through the code since the changes.

klist_iter fixes the problem of one thread deleting nodes while another
thread is traversing the klist.  However the deleted nodes aren't marked
in any way, so the traversing thread might not realize they had been
deleted.  Also, since the klist_iter itself holds a reference to the
deleted node you can't do a synchronous remove -- it will deadlock.

This ended up causing difficulties for the driver_detach routine in the 
driver model core.  I had to rewrite it, avoiding the use of a klist_iter.

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Luben Tuikov]

On 06/21/05 16:33, Alan Stern wrote:
> On Tue, 21 Jun 2005, Christoph Hellwig wrote:
> 
> 
>>On Tue, Jun 21, 2005 at 04:04:06PM -0400, Alan Stern wrote:
>>
>>>This objection runs up against an issue we discussed some time ago.  
>>>Should the intended meaning of scsi_remove_host be simply that the kernel
>>>needs to stop using the HBA reasonably soon?  In that case you are right.  
>>>Or should the intended meaning be that the HBA is actually gone
>>>(hot-unplugged) and all further attempts to use it will fail?  In that

For the success of the state machine, states must be discrete.
scsi_remove_host() cannot mean "reasonably soon".  If it _has_ to mean
that, then _add_ another state.

If one wants to consolidate the hot-unplugging with notified unplug,
one might as well aim for the supercase "hot-unplug".  Its implementation
would surely satisfy notified unplug.

>>>case it doesn't matter.  The best ways to resolve this issue may be to
>>>have a separate scsi_host_gone routine or to add an extra argument to
>>>scsi_remove_host.
>>
>>It must mean both because we don't know whether a hot unplug happened or
>>not.  The ->remove callbacks don't tell us.
> 
> 
> I would describe it differently: Since you don't know whether a hot-unplug 
> occurred, you might as well assume it did not.  There's no harm in this, 
> because if the HBA really was unplugged then it doesn't matter what you 
> do; everything will fail.

Conclusion is right, premise is not.  Assume that there was hot-unplug.
It is the supercase which absolves notified unplug.

(Hot unplug of targets is a bit different in that both the hw and the
LLDD knows of the event and some things need not be waited to time out...)

> But those sd flush-cache commands are a problem.  Presumably you want to 
> send them _after_ all the outstanding commands have finished or been 
> cancelled.  What's the right way to allow those commands while rejecting 
> all others?

I don't know.  But to be honest, I'd imagine sending flush-cache as soon
as possible without waiting for anything to cancel or timeout or return.
That is, you want to have the best chance.

The lower layers should make sure that if the device is gone, that is
they _know_ about the event, an error is returned.

So that the treatment of flush-cache is no different than already queued
commands.

So in effect, you (usb-storage) know about a condition (say device is gone)
you should act on it right in queuecommand() and not accept the command,
as opposed to hoping for the midlayer to turn around and preempt those commands.

	Luben
P.S. USB is perfect, since you're notified on unplug.

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Luben Tuikov]

On 06/21/05 16:58, Mike Anderson wrote:
> Alan Stern [stern@rowland.harvard.edu] wrote:
>>I would describe it differently: Since you don't know whether a hot-unplug 
>>occurred, you might as well assume it did not.  There's no harm in this, 
>>because if the HBA really was unplugged then it doesn't matter what you 
>>do; everything will fail.
>>
> 
> 
> I have asked this multiple times, but I will again. If the hba knows to
> fail everything by some internal state in the LLDD why does it not also
> know to flush all commands back to the scsi mid layer in this unplugged
> state so we do not have to deal with canceling them from the mid-layer. 

This is a good question.  The way I see it: follow the path of the event
and at each logical point act on it:
	device-->SDS-->HA-->LLDD-->SCSI midlayer-->etc.
That is, at each point of the unplug event's path, act on that
unplug event, meaning HA first, then LLDD, then SCSI midlayer.

The HA and LLDD would work together, but if they _know_ about "device gone"
they should stop accepting commands to that device and return them to
the midlayer, along with saying hey, this device is gone, along with
returning all previously queued but infinished commands and rejecting new.

The other way around, a notified unplug, the event travels the same path
but in opposite direction.  So the same actions should follow but
in opposite direction: SCSI midlayer first, LLDD, HA, finally device,
just as the event from userspace travels (remove device).

(We have to go to the device eventualy in case there any reservations on
the LU.)
 
> I think it has been stated many times if commands always go in through
> queuecommand and always return through scsi_done it makes the chances of
> errors a lot less.

Completely agree!

>>But those sd flush-cache commands are a problem.  Presumably you want to 
>>send them _after_ all the outstanding commands have finished or been 
>>cancelled.  What's the right way to allow those commands while rejecting 
>>all others?
> 
> 
> We have some notion of this already if you look in scsi_prep_fn (i.e.,
> specials_only), but this is based on sdev state not shost. Checking for a
> specials_only flag in scsi_dispatch may not be to clean. This may be why
> we should look at a shost state model update and align sdev and shost to
> clearly state what will happen to command.s

Check host state first, then target then lu...

	Luben

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Wed, 22 Jun 2005, Luben Tuikov wrote:

> For the success of the state machine, states must be discrete.
> scsi_remove_host() cannot mean "reasonably soon".  If it _has_ to mean
> that, then _add_ another state.
> 
> If one wants to consolidate the hot-unplugging with notified unplug,
> one might as well aim for the supercase "hot-unplug".  Its implementation
> would surely satisfy notified unplug.

Certainly.  In the UNPLUG_NOTIFY state, only commands like SYNCHRONIZE 
CACHE should be allowed.

> The lower layers should make sure that if the device is gone, that is
> they _know_ about the event, an error is returned.
> 
> So that the treatment of flush-cache is no different than already queued
> commands.

For the unplug case you are right.  For the notify case it's a little 
tricky because of the interaction with command cancellation (if commands 
need to be cancelled) and the need to allow the cache flush without 
allowing other commands.

> So in effect, you (usb-storage) know about a condition (say device is gone)
> you should act on it right in queuecommand() and not accept the command,
> as opposed to hoping for the midlayer to turn around and preempt those commands.
> 
> 	Luben
> P.S. USB is perfect, since you're notified on unplug.

Yes.  But in fact usb-storage has a small loophole.  Before calling
scsi_remove_host, we set a flag that causes the queuecommand routine to
reject all new commands and any currently-executing command to fail-fast.  
However there is a small window between the time a command is accepted and
the time it starts to execute (because execution takes place in a
separate kernel thread).  If a command has been accepted and has not
started to execute, then usb-storage doesn't cancel it.  We've been
relying on the midlayer to cancel it for us.

There's no reason usb-storage can't cancel the command before calling
scsi_remove_host.  I tried out a patch to do just that, and it worked
fine.  (Waiting until after scsi_remove_host to cancel the command doesn't
work, for the obvious reason that the midlayer may have already cancelled
it.)  I'll submit the patch; it should solve this particular problem for
usb-storage.  I don't know how any other hot-unpluggable LLDDs will handle
cancellation during unplug.

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Luben Tuikov]

On 06/22/05 11:12, Alan Stern wrote:
> 
> For the unplug case you are right.  For the notify case it's a little 
> tricky because of the interaction with command cancellation (if commands 
> need to be cancelled) and the need to allow the cache flush without 
> allowing other commands.

The notify event can "travel" right behind flush-cache and "close doors"
behind.  (since it travels from the application client to the device)

> Yes.  But in fact usb-storage has a small loophole.  Before calling
> scsi_remove_host, we set a flag that causes the queuecommand routine to
> reject all new commands and any currently-executing command to fail-fast.  
> However there is a small window between the time a command is accepted and
> the time it starts to execute (because execution takes place in a
> separate kernel thread).  If a command has been accepted and has not
> started to execute, then usb-storage doesn't cancel it.  We've been
> relying on the midlayer to cancel it for us.

Sounds like a race to me.  You should always be able to
identify/find a command at any point after it has been submitted
by queuecommand().

> There's no reason usb-storage can't cancel the command before calling
> scsi_remove_host.  I tried out a patch to do just that, and it worked
> fine.  (Waiting until after scsi_remove_host to cancel the command doesn't
> work, for the obvious reason that the midlayer may have already cancelled
> it.)  I'll submit the patch; it should solve this particular problem for

Yes indeed: the proper order is:
	* lock the front door,
	* empty all rooms,
	* call scsi_remove_host().

This is on _hot-unplug_ since the event came from the SDS.
In the event of a notified unplug, the event would be known to
SCSI Core _before_ it is known to the LLDD, so SCSI Core would
lock the "fence gate", send flush-cache (which would seem as any
other command to you), then wait for that to return (which would
mean no other commands are pending, since this was the last command
to be sent after locking the "fence gate") and then it would
call scsi_remove_host() or whatever appropriate.

> usb-storage.  I don't know how any other hot-unpluggable LLDDs will handle
> cancellation during unplug.

In case the LLDD is implemented properly, just doing nothing should suffice. ;-)

	Luben

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Alan Stern]

On Wed, 22 Jun 2005, Luben Tuikov wrote:

> The notify event can "travel" right behind flush-cache and "close doors"
> behind.  (since it travels from the application client to the device)

Are you sure about that?  One way to generate such a notify event is to
rmmod the LLDD.  The event is then generated by the LLDD and it travels up
to the midlayer, by way of scsi_remove_host.  You seem to be saying that
there should be a separate API for the LLDD to notify the midlayer that it
is about to be unloaded.

> Yes indeed: the proper order is:
> 	* lock the front door,
> 	* empty all rooms,
> 	* call scsi_remove_host().
> 
> This is on _hot-unplug_ since the event came from the SDS.
> In the event of a notified unplug, the event would be known to
> SCSI Core _before_ it is known to the LLDD, so SCSI Core would
> lock the "fence gate", send flush-cache (which would seem as any
> other command to you), then wait for that to return (which would
> mean no other commands are pending, since this was the last command
> to be sent after locking the "fence gate") and then it would
> call scsi_remove_host() or whatever appropriate.

Currently there is nothing like your "fence gate", although there should 
be.

> > usb-storage.  I don't know how any other hot-unpluggable LLDDs will handle
> > cancellation during unplug.
> 
> In case the LLDD is implemented properly, just doing nothing should suffice. ;-)

So should the midlayer no longer try to cancel outstanding commands when a 
host is removed?  Can it rely on the LLDDs to do so?

Alan Stern

Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Luben Tuikov]

On 06/22/05 12:16, Alan Stern wrote:
> On Wed, 22 Jun 2005, Luben Tuikov wrote:
> 
> 
>>The notify event can "travel" right behind flush-cache and "close doors"
>>behind.  (since it travels from the application client to the device)
> 
> 
> Are you sure about that?  One way to generate such a notify event is to
> rmmod the LLDD.  The event is then generated by the LLDD and it travels up
> to the midlayer, by way of scsi_remove_host.  You seem to be saying that
> there should be a separate API for the LLDD to notify the midlayer that it
> is about to be unloaded.

Alan, I'd consider this to be "hot-unplug" event (as you describe it).

(In which case the other order of rules apply (set flag for queuecommand(),
empty queues, call scsi_remove_host()).

> So should the midlayer no longer try to cancel outstanding commands when a 
> host is removed?  Can it rely on the LLDDs to do so?

Well, let's think about it.  If the LLDD has emptied its queues, and
the LLDD doesn't own any commands, then the midlayer would have
nothing to cancel (midlayer's pending queue is empty).

Nevertheless, the midlayer should inspect its pending queue to see
if it needs to preempt any outstanding commands, in case the LLDD
didn't return them.

	Luben