[PATCH] scsi_set_host_offline (resend)

[PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

This is a resend of a previous patch for the scsi_set_host_offline
interface.

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

=====
name:		00_scsi_set_host_offline-2.diff
version:	2003-03-25.02:01:33-0800
against:	2.5.66

 scsi.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 scsi.h |    1 +
 2 files changed, 51 insertions(+)

=====
===== drivers/scsi/scsi.c 1.99 vs edited =====
--- 1.99/drivers/scsi/scsi.c	Mon Mar  3 10:48:16 2003
+++ edited/drivers/scsi/scsi.c	Tue Mar 25 01:45:00 2003
@@ -1295,6 +1295,37 @@
 }
 
 /**
+ * scsi_host_for_each_device - call function for each host child device
+ * @shost:	struct Scsi_Host to interate over.
+ * @data:	void pointer argumnet passed to called function.
+ * @fn:		function to call for each device.
+ *
+ **/
+static int scsi_host_for_each_device(struct Scsi_Host *shost,
+			 void * data, int (*fn)(struct scsi_device *, void *))
+{
+	struct list_head *lh;
+	struct scsi_device *sdev;
+	unsigned long flags;
+	int error = 0;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	for (lh = shost->my_devices.next; 
+	      (!error) && (lh != &shost->my_devices);) {
+		sdev = list_entry(lh, struct scsi_device, siblings);
+		scsi_device_get(sdev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		error = fn(sdev, data);
+		spin_lock_irqsave(shost->host_lock, flags);
+		lh = lh->next;
+		scsi_device_put(sdev);
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	return error;
+}
+
+/**
  * scsi_set_device_offline - set scsi_device offline
  * @sdev:	pointer to struct scsi_device to offline. 
  *
@@ -1333,6 +1364,25 @@
 	} else {
 		/* FIXME: Send online state change hotplug event */
 	}
+}
+
+/**
+ * scsi_set_device_offline - wrapper.
+ **/
+static int __scsi_set_device_offline(struct scsi_device *sdev, void *data)
+{
+	scsi_set_device_offline(sdev);
+	return 0;
+}
+
+/**
+ * scsi_set_host_offline - set all scsi_devices on a host offline
+ * @shost:	pointer to struct Scsi_Host.
+ *
+ **/
+void scsi_set_host_offline(struct Scsi_Host *shost)
+{
+	scsi_host_for_each_device(shost, NULL, __scsi_set_device_offline);
 }
 
 /*
===== drivers/scsi/scsi.h 1.67 vs edited =====
--- 1.67/drivers/scsi/scsi.h	Fri Mar 14 16:35:26 2003
+++ edited/drivers/scsi/scsi.h	Mon Mar 24 11:24:59 2003
@@ -436,6 +436,7 @@
 extern void scsi_slave_detach(struct scsi_device *);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
+extern void scsi_set_host_offline(struct Scsi_Host *);
 extern void scsi_set_device_offline(struct scsi_device *);
 extern void scsi_done(Scsi_Cmnd * SCpnt);
 extern void scsi_finish_command(Scsi_Cmnd *);

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Tue, 2003-03-25 at 04:07, Mike Anderson wrote:

This isn't right:

> +	spin_lock_irqsave(shost->host_lock, flags);
> +	for (lh = shost->my_devices.next; 
> +	      (!error) && (lh != &shost->my_devices);) {
> +		sdev = list_entry(lh, struct scsi_device, siblings);
> +		scsi_device_get(sdev);
> +		spin_unlock_irqrestore(shost->host_lock, flags);

You can't use the host_lock to protect the list and then drop it in the
middle of list traversal.  Doing this is bound to have repercussions for
hotplug.  I know we're a total mess for hotplug now, but I'd rather not
add to it.

This problem can't be unique to SCSI, so I think what we need is
something like a device generic function, like bus_for_each_device,
except that it's device_for_each_child or something. where we get a
properly ref counted and protected list traversal that will work for
hotplugging.

Pat, can you think of anything nicely generic?  We have a non-standard
list to traverse and would probably have to hold the list lock prior to
entry?

James

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

James Bottomley [James.Bottomley@SteelEye.com] wrote:
> On Tue, 2003-03-25 at 04:07, Mike Anderson wrote:
> 
> This isn't right:
> 
> > +	spin_lock_irqsave(shost->host_lock, flags);
> > +	for (lh = shost->my_devices.next; 
> > +	      (!error) && (lh != &shost->my_devices);) {
> > +		sdev = list_entry(lh, struct scsi_device, siblings);
> > +		scsi_device_get(sdev);
> > +		spin_unlock_irqrestore(shost->host_lock, flags);
> 
> You can't use the host_lock to protect the list and then drop it in the
> middle of list traversal.  Doing this is bound to have repercussions for
> hotplug.  I know we're a total mess for hotplug now, but I'd rather not
> add to it.
> 

Probably already implied by your comment but...

I am using the lock to keep the list stable only during the traverse to
the next device which I will get a ref before dropping the lock. The
list could change during the call to the function (i.e an addition /
removal of nodes I do not have ref to).

Even if we went with a subsystem bus_for_each function I believe you are
asking for more state as previous discussed on the host offline subject
to not allow more additions. Correct?

> This problem can't be unique to SCSI, so I think what we need is
> something like a device generic function, like bus_for_each_device,
> except that it's device_for_each_child or something. where we get a
> properly ref counted and protected list traversal that will work for
> hotplugging.
> 

The bus_for_each solution currently use a rw sema which I thought would
not be a good choice for these type of operations as there is a
possibility that we may want to call functions (offline) in interrupt
context.

Are you indicating that sema operations would be ok?

A possible side issue that Mochel can correct me on is that the calling
function would be restricted some as it cannot call device_register /
device_unregister as it would block.

If we want to head this direction we may need to alter the relationship
of children nodes off hosts.

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

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Tue, 2003-03-25 at 12:45, Mike Anderson wrote:
> Even if we went with a subsystem bus_for_each function I believe you are
> asking for more state as previous discussed on the host offline subject
> to not allow more additions. Correct?

Really, yes.  Once we're notified the host is going away we need to
quiesce it and all its lists (disallow at least addition).

> > This problem can't be unique to SCSI, so I think what we need is
> > something like a device generic function, like bus_for_each_device,
> > except that it's device_for_each_child or something. where we get a
> > properly ref counted and protected list traversal that will work for
> > hotplugging.
> > 
> 
> The bus_for_each solution currently use a rw sema which I thought would
> not be a good choice for these type of operations as there is a
> possibility that we may want to call functions (offline) in interrupt
> context.

Well...I think it's not too much of a stretch to require that additions
to the device list be done only from user context (initial inquiry does
that, as does scsi add/remove-single-device), so a sema can be made OK
here.

> Are you indicating that sema operations would be ok?
> 
> A possible side issue that Mochel can correct me on is that the calling
> function would be restricted some as it cannot call device_register /
> device_unregister as it would block.

Well...I'm not a huge fan of the kernel doing all the work.  I think the
most sensible way forward is to have the kernel trigger an agreed set of
events and have the nicely scriptable user level do the rest.  However,
if you want to do it this way, the function could use the kernel event
thread to do the actual work although it's a bit more overhead.

> If we want to head this direction we may need to alter the relationship
> of children nodes off hosts.

All I think we need to be sure of is that where generic device provides
hotplug usable infrastructure (like refcounting, etc) SCSI makes use of
it.  (Obviously, the more of the generic infrastructure we use, the more
of somebody else's problem it becomes...)

James

Re: [PATCH] scsi_set_host_offline (resend)

[Patrick Mochel]

On 25 Mar 2003, James Bottomley wrote:

> On Tue, 2003-03-25 at 12:45, Mike Anderson wrote:
> > Even if we went with a subsystem bus_for_each function I believe you are
> > asking for more state as previous discussed on the host offline subject
> > to not allow more additions. Correct?
> 
> Really, yes.  Once we're notified the host is going away we need to
> quiesce it and all its lists (disallow at least addition).
> 
> > > This problem can't be unique to SCSI, so I think what we need is
> > > something like a device generic function, like bus_for_each_device,
> > > except that it's device_for_each_child or something. where we get a
> > > properly ref counted and protected list traversal that will work for
> > > hotplugging.

Sure, that's sane. It should be relatively easy to implement. If you don't 
see anything in the next couple of days, bug me. 

> > A possible side issue that Mochel can correct me on is that the calling
> > function would be restricted some as it cannot call device_register /
> > device_unregister as it would block.
> 
> Well...I'm not a huge fan of the kernel doing all the work.  I think the
> most sensible way forward is to have the kernel trigger an agreed set of
> events and have the nicely scriptable user level do the rest.  However,
> if you want to do it this way, the function could use the kernel event
> thread to do the actual work although it's a bit more overhead.

<shameless plug>
I'm not exactly sure what the context is, but maybe it's something that 
could be off-loaded to /sbin/hotplug ?

> > If we want to head this direction we may need to alter the relationship
> > of children nodes off hosts.
> 
> All I think we need to be sure of is that where generic device provides
> hotplug usable infrastructure (like refcounting, etc) SCSI makes use of
> it.  (Obviously, the more of the generic infrastructure we use, the more
> of somebody else's problem it becomes...)

Actually, the more generic infrastructure you use, the more of everybody 
else's problem it becomes, which is better in a sense. ;)


	-pat

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> > 
> > The bus_for_each solution currently use a rw sema which I thought would
> > not be a good choice for these type of operations as there is a
> > possibility that we may want to call functions (offline) in interrupt
> > context.
> 
> Well...I think it's not too much of a stretch to require that additions
> to the device list be done only from user context (initial inquiry does
> that, as does scsi add/remove-single-device), so a sema can be made OK
> here.

I thought we where looking at a solution for all traversals of the list?
There are currently ~52 refs to my_devices, while some traversals can be
evalutated and possibly removed there maybe valid ones that happen
without user context.


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

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Tue, 2003-03-25 at 17:29, Mike Anderson wrote:
> I thought we where looking at a solution for all traversals of the list?
> There are currently ~52 refs to my_devices, while some traversals can be
> evalutated and possibly removed there maybe valid ones that happen
> without user context.

Sorry, I was thinking both short and long term.

Short term, it's probably easiest for the firewire stuff to access the
offline and removal functions from user land hotplug scripts (so they
don't actually need the in-kernel hook).

Long term we do need to see if we can go to a nice refcounted model for
all 50 odd list traversals.  At least auditing them to see if we can use
a semaphore everywhere.

James

Re: [PATCH] scsi_set_host_offline (resend)

[Patrick Mansfield]

On Thu, Mar 27, 2003 at 09:42:39AM -0600, James Bottomley wrote:
> On Tue, 2003-03-25 at 17:29, Mike Anderson wrote:
> > I thought we where looking at a solution for all traversals of the list?
> > There are currently ~52 refs to my_devices, while some traversals can be
> > evalutated and possibly removed there maybe valid ones that happen
> > without user context.
> 
> Sorry, I was thinking both short and long term.
> 
> Short term, it's probably easiest for the firewire stuff to access the
> offline and removal functions from user land hotplug scripts (so they
> don't actually need the in-kernel hook).
> 
> Long term we do need to see if we can go to a nice refcounted model for
> all 50 odd list traversals.  At least auditing them to see if we can use
> a semaphore everywhere.
> 
> James

same_target_siblings has a similiar problem.

It is used primarily for the single_lun code, sometimes in softirq context
(should really not sleep there), and also to get any exisiting scsi_level
during scan.

-- Patrick Mansfield

Re: [PATCH] scsi_set_host_offline (resend)

[Matthew Dharm]

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

Well, Linus wants this to work for usb-storage more short-term than
long-term....

Suggestions?

Matt

On Thu, Mar 27, 2003 at 09:42:39AM -0600, James Bottomley wrote:
> On Tue, 2003-03-25 at 17:29, Mike Anderson wrote:
> > I thought we where looking at a solution for all traversals of the list?
> > There are currently ~52 refs to my_devices, while some traversals can be
> > evalutated and possibly removed there maybe valid ones that happen
> > without user context.
> 
> Sorry, I was thinking both short and long term.
> 
> Short term, it's probably easiest for the firewire stuff to access the
> offline and removal functions from user land hotplug scripts (so they
> don't actually need the in-kernel hook).
> 
> Long term we do need to see if we can go to a nice refcounted model for
> all 50 odd list traversals.  At least auditing them to see if we can use
> a semaphore everywhere.
> 
> James
> 
> 
> -
> 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

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It's not that hard.  No matter what the problem is, tell the customer 
to reinstall Windows.
					-- Nurse
User Friendly, 3/22/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

Matthew Dharm [mdharm-scsi@one-eyed-alien.net] wrote:
> Well, Linus wants this to work for usb-storage more short-term than
> long-term....
> 
> Suggestions?
> 

I am still working toward the goal of supporting this. Short-term /
long-term will be determined by the changes and Linus / James's
recommendation. I will wait to see the iterator that Mochel recommends.
In the mean time I am working on getting our scsi_host sysfs structure
corrected so that release can be called at the proper time.

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

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Fri, 2003-03-28 at 19:32, Matthew Dharm wrote:
> Well, Linus wants this to work for usb-storage more short-term than
> long-term....

Well, I think Mike's given us everything you need.  We can use the
set_device_offline to alter the remove-single-device path to offline the
device, wait for the returning I/Os to complete or error and then remove
the device.

You can then trigger this from your hotplug scripts.

James

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> On Fri, 2003-03-28 at 19:32, Matthew Dharm wrote:
> > Well, Linus wants this to work for usb-storage more short-term than
> > long-term....
> 
> Well, I think Mike's given us everything you need.  We can use the
> set_device_offline to alter the remove-single-device path to offline the
> device, wait for the returning I/Os to complete or error and then remove
> the device.
> 
> You can then trigger this from your hotplug scripts.

I believe we still need to fixup scsi_remove_host to not fail (i.e
return void) and call "hostt->release" when the ref count on the host
goes to zero. (I believe this was a point that started us on this thread
of work as the usb implementation was one device per host).

I have a work in progress patch that ensures that all scsi hosts have a
sysfs struct device even if the driver is not converted to the driver
model which allows scsi_devices to be true children of a scsi_host
struct device. This should allow the usage of kobject ref counting for
scsi_hosts. I am still working on plugging the holes in this support
hopefully I will have something out to the list soon.

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

Re: [PATCH] scsi_set_host_offline (resend)

[Oliver Neukum]

Am Samstag, 29. März 2003 20:04 schrieb Mike Anderson:
> James Bottomley [James.Bottomley@steeleye.com] wrote:
> > On Fri, 2003-03-28 at 19:32, Matthew Dharm wrote:
> > > Well, Linus wants this to work for usb-storage more short-term than
> > > long-term....
> >
> > Well, I think Mike's given us everything you need.  We can use the
> > set_device_offline to alter the remove-single-device path to offline the
> > device, wait for the returning I/Os to complete or error and then remove
> > the device.
> >
> > You can then trigger this from your hotplug scripts.
>
> I believe we still need to fixup scsi_remove_host to not fail (i.e
> return void) and call "hostt->release" when the ref count on the host

Yes, we cannot have a failure there.

> goes to zero. (I believe this was a point that started us on this thread
> of work as the usb implementation was one device per host).

Not necessarily. The USB driver cannot tell a simple device apart
from a SCSI<->USB bridge, which several SCSI devices can be
attached to. Hotplugging however is always on a host level with
USB storage.

	Regards
		Oliver

-
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

Re: [PATCH] scsi_set_host_offline (resend)

[Matthew Dharm]

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

My understanding was that I couldn't call set_device_offline with the host
lock held, which is a problem because I need the host lock to traverse the
device list.

Matt

On Sat, Mar 29, 2003 at 08:43:08AM -0600, James Bottomley wrote:
> On Fri, 2003-03-28 at 19:32, Matthew Dharm wrote:
> > Well, Linus wants this to work for usb-storage more short-term than
> > long-term....
> 
> Well, I think Mike's given us everything you need.  We can use the
> set_device_offline to alter the remove-single-device path to offline the
> device, wait for the returning I/Os to complete or error and then remove
> the device.
> 
> You can then trigger this from your hotplug scripts.
> 
> James
> 

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Sat, 2003-03-29 at 14:53, Matthew Dharm wrote:
> My understanding was that I couldn't call set_device_offline with the host
> lock held, which is a problem because I need the host lock to traverse the
> device list.

The solution we're talking about here is from the user level hotplug
scripts.  Once we get the remove-single-device fixed, you simply use the
device hot unplug script to traverse the attached SCSI devices, clean up
the user land (kill processes, unmount filesystems etc.) and then blow
the devices away.  After the host has no devices, you should simply be
able to trigger a host removal from within your driver.

James

Re: [PATCH] scsi_set_host_offline (resend)

[Matthew Dharm]

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

On Sat, Mar 29, 2003 at 03:54:59PM -0600, James Bottomley wrote:
> On Sat, 2003-03-29 at 14:53, Matthew Dharm wrote:
> > My understanding was that I couldn't call set_device_offline with the host
> > lock held, which is a problem because I need the host lock to traverse the
> > device list.
> 
> The solution we're talking about here is from the user level hotplug
> scripts.  Once we get the remove-single-device fixed, you simply use the
> device hot unplug script to traverse the attached SCSI devices, clean up
> the user land (kill processes, unmount filesystems etc.) and then blow
> the devices away.  After the host has no devices, you should simply be
> able to trigger a host removal from within your driver.

So, the sequence goes something like this:

(1) user removes device, causing disconnect() callback in USB system to be
called

(2) usb-storage driver markes device as gone so as to make all commands
fail immediately, but does nothing else

(3) hotplug script is notified of USB device removal, figures out that this
is a usb-storage device (possibly a USB/SCSI bridge), and invokes the
hot-unplug for SCSI for each SCSI device (after unmount, etc.)  NOTE: I
don't know how hotplug is supposed to be able to identify the bridge case.

(4) SCSI hot-unplugging offlines the device, then removes them.

(5) Someone notices that the dead virtual host has no devices left, and
signals the usb-storage driver that it's safe to unregister?!??!

(6) SCSI mid-layer does cleanup and notifies usb-storage when it is safe to
free resources

(7) usb-storage destroys all structures and completes cleanup

I can see how this all works, except for step #5 -- how does usb-storage
know that it's time to unregister?

This seems awfully complicated... usb -> usb-storage -> userspace -> scsi
-> unknown -> usb-storage -> scsi -> usb-storage is the sequence of events.

It also means that if the hotplug scripts don't do the job right, we can
wind up with lots of dead virtual HBAs lying around.  Not fatal, I guess...
but pretty bad.

Originally we had been talking about usb -> usb-storage -> scsi ->
usb-storage as the sequence.  userspace never got involved except as a
helper -- it was never in the critical path. This also put the
responsibility for faking-out devices that had been detached but not yet
logically removed in one central place -- with this model, each HBA is
responsible for faking-out devices on it's own.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] scsi_set_host_offline (resend)

[James Bottomley]

On Sat, 2003-03-29 at 16:15, Matthew Dharm wrote:
> So, the sequence goes something like this:
> 
> (1) user removes device, causing disconnect() callback in USB system to be
> called
> 
> (2) usb-storage driver markes device as gone so as to make all commands
> fail immediately, but does nothing else
> 
> (3) hotplug script is notified of USB device removal, figures out that this
> is a usb-storage device (possibly a USB/SCSI bridge), and invokes the
> hot-unplug for SCSI for each SCSI device (after unmount, etc.)  NOTE: I
> don't know how hotplug is supposed to be able to identify the bridge case.

Ideally, yes.  The driver throwing the hotplug event is supposed to be
able to distinguish between device and bridge removal.

> (4) SCSI hot-unplugging offlines the device, then removes them.
> 
> (5) Someone notices that the dead virtual host has no devices left, and
> signals the usb-storage driver that it's safe to unregister?!??!

That's the (unimplemented in scsi) refcounting in the generic device
core.  In theory, once the struct device belonging to the host's
refcount decrements to zero (and you trigger this by doing a device_put
in the first usb remove), it calls the ->release() hook.  The code
plugged into this hook knows that the devices are gone, so does the
final USB cleanup (and the scsi host removal).

> (6) SCSI mid-layer does cleanup and notifies usb-storage when it is safe to
> free resources
> 
> (7) usb-storage destroys all structures and completes cleanup
> 
> I can see how this all works, except for step #5 -- how does usb-storage
> know that it's time to unregister?
> 
> This seems awfully complicated... usb -> usb-storage -> userspace -> scsi
> -> unknown -> usb-storage -> scsi -> usb-storage is the sequence of events.

Well, USB storage devices with filesystems on top are rather complex
things to clean up after.

> It also means that if the hotplug scripts don't do the job right, we can
> wind up with lots of dead virtual HBAs lying around.  Not fatal, I guess...
> but pretty bad.
> 
> Originally we had been talking about usb -> usb-storage -> scsi ->
> usb-storage as the sequence.  userspace never got involved except as a
> helper -- it was never in the critical path. This also put the
> responsibility for faking-out devices that had been detached but not yet
> logically removed in one central place -- with this model, each HBA is
> responsible for faking-out devices on it's own.

There's just too much specific knowledge to put into the kernel.  Even
if SCSI were to offline the device, who does something about the filesys
or other applications using it?  All you gain is moving one step closer
to the solution, not the solution itself.

The way to think about this, I think, is "what would we do if the user
asked nicely before removing the device" (i.e. requested device ejection
rather than forced it).  Then the above makes perfect sense because we
have to start at the top and move down.  Since requested ejection is the
model for things like hotplug PCI busses, I'm hoping someone else is
thinking about this.

As far as the state of play today, I'm not sure if the SCSI refcounting
is correct or not.  The SCSI list traversals are certainly racy and
unsafe, and we have no SCSI hotplug event.

This is probably because no-one who develops for SCSI has access to true
hotplug hardware.  However, we do have other uses for the hotplug
infrastructure (FC device addition/removal, even SES device handling),
we're just not entirely sure how to proceed.

James

Re: [PATCH] scsi_set_host_offline (resend)

[Oliver Neukum]

> The way to think about this, I think, is "what would we do if the user
> asked nicely before removing the device" (i.e. requested device ejection
> rather than forced it).  Then the above makes perfect sense because we
> have to start at the top and move down.  Since requested ejection is the
> model for things like hotplug PCI busses, I'm hoping someone else is
> thinking about this.

No this is exactly the way not to ever think about this.
You are not asked to remove a device, you are informed it's gone.
This is the case in all but sometimes one type of bus system in the
kernel. And that is the way information has to flow. Strictly
from the driver closest to the hardware up. And there must be no
undue delay and neither failure. Failure to handle a removal is a
contradiction in terms. This means that any call to user space must not be
waited for in the kernel and failure must not be deadly. Filesystems
must already be ready to deal with incorrectible errors. They are not
your problem.

	Regards
		Oliver

Re: [PATCH] scsi_set_host_offline (resend)

[Luben Tuikov]

Oliver Neukum wrote:
>>The way to think about this, I think, is "what would we do if the user
>>asked nicely before removing the device" (i.e. requested device ejection
>>rather than forced it).  Then the above makes perfect sense because we
>>have to start at the top and move down.  Since requested ejection is the
>>model for things like hotplug PCI busses, I'm hoping someone else is
>>thinking about this.
> 
> 
> No this is exactly the way not to ever think about this.
> You are not asked to remove a device, you are informed it's gone.
> This is the case in all but sometimes one type of bus system in the
> kernel. And that is the way information has to flow. Strictly
> from the driver closest to the hardware up.

This is correct.

> And there must be no
> undue delay and neither failure. Failure to handle a removal is a
> contradiction in terms. This means that any call to user space must not be
> waited for in the kernel and failure must not be deadly. Filesystems
> must already be ready to deal with incorrectible errors. They are not
> your problem.

We've discussed this before.

Userspace must be _notified_.  Note the choice of verb.  This doesn't
*necessarily* mean that the kernel will wait on userspace to finish
some action, but it may, but *not* necessarily -- this is to the discrection
of the hotplug system working together with the kernel -- ``to each it's own''.
I.e. for some hotplug events there maybe a userspace event involved
and for others it may not.

There are several reasons for this, one of which is that the system
administrator may want to know about the removal of a device.  So why
should userspace do some kind of polling, when the kernel can simply
_notify_ userspace.

As to usb, you can fail all commands in due time, notify
the subsystem above you (which will take over) and free your
resources, and leave others to be freed by the subsystem above
you which it registered. (ideally)

-- 
Luben
P.S. We've all discussed this before, same set of ppl, same set of mailing
lists.

Re: [PATCH] scsi_set_host_offline (resend)

[Oliver Neukum]

> > No this is exactly the way not to ever think about this.
> > You are not asked to remove a device, you are informed it's gone.
> > This is the case in all but sometimes one type of bus system in the
> > kernel. And that is the way information has to flow. Strictly
> > from the driver closest to the hardware up.
>
> This is correct.

Good.

> > And there must be no
> > undue delay and neither failure. Failure to handle a removal is a
> > contradiction in terms. This means that any call to user space must not
> > be waited for in the kernel and failure must not be deadly. Filesystems
> > must already be ready to deal with incorrectible errors. They are not
> > your problem.
>
> We've discussed this before.
>
> Userspace must be _notified_.  Note the choice of verb.  This doesn't
> *necessarily* mean that the kernel will wait on userspace to finish
> some action, but it may, but *not* necessarily -- this is to the
> discrection of the hotplug system working together with the kernel -- ``to
> each it's own''. I.e. for some hotplug events there maybe a userspace event
> involved and for others it may not.

Please explain yourself further. We cannot deal with wait sometimes.
Either we may wait or we may not wait. If we may wait,
we must always deal with it like we will wait and deal with results,
which is impossible, as the only result we can deal with is success.

Notification is absolutely no problem. _Requiring_ the kernel
to wait for user space to act so that the process of removal
can be completed as far as the the low level driver is concerned
is a major problem.

All is well if a low level driver can consider a device gone for good
as soon as "/sbin/hotplug" is spawned. The key here is spawned
, not doing something or finish, but spawned.

We have indeed discussed this before and the need to notify
user space was never questioned, as far as I recall.
The point of contention always was whether the notification
had to do specific things for the process of unplugging to finish
as far as it concerns the low level driver.

> There are several reasons for this, one of which is that the system
> administrator may want to know about the removal of a device.  So why
> should userspace do some kind of polling, when the kernel can simply
> _notify_ userspace.

Oh absolutely, sure.

> As to usb, you can fail all commands in due time, notify
> the subsystem above you (which will take over) and free your
> resources, and leave others to be freed by the subsystem above
> you which it registered. (ideally)

That is what we hope for.

	Regards
		Oliver

Re: [PATCH] scsi_set_host_offline (resend)

[Luben Tuikov]

Oliver Neukum wrote:
> 
> Please explain yourself further. We cannot deal with wait sometimes.

No need to -- we agree.

> Either we may wait or we may not wait. If we may wait,
> we must always deal with it like we will wait and deal with results,
> which is impossible, as the only result we can deal with is success.
> 
> Notification is absolutely no problem. _Requiring_ the kernel
> to wait for user space to act so that the process of removal
> can be completed as far as the the low level driver is concerned
> is a major problem.

No, a LLDD will not be required to wait on userspace.  No one wants
this and the fault in this is quite evident that no one would want it.

The only point is notification, and we seem to agree on this.
 
> All is well if a low level driver can consider a device gone for good
> as soon as "/sbin/hotplug" is spawned. The key here is spawned
> , not doing something or finish, but spawned.

Ideally:

You can assume even better than that: as soon as your interconnect
tells you that the device is gone, a LLDD (== transport portal) does this:
---> calls xxx_dev_removal(dev) (marks device as gone in this subsystem),
(and which in turn:
  ---> calls scsi_device_removal(dev) (mark dev as gone in this subsys),
    ... (see my other posting))
(none of which blocks, more work/discussion is needed here, but this is
different topic)

and then the LLDD can free *its* resources which it allocated on the
device.

*But*, the _important__point_ is that the LLDD must be able to handle
requests (queuecommand()) to non-existant (== just removed) devices,
and not oops, or whatever.

So, in effect, you just call usb_dev_removal(dev) (to be written),
and free _your_ resources on the device. (note important point above)

> We have indeed discussed this before and the need to notify
> user space was never questioned, as far as I recall.
> The point of contention always was whether the notification
> had to do specific things for the process of unplugging to finish
> as far as it concerns the low level driver.

Right.  As far as I can see, those subsystem entries, xxx_dev_removal(dev),
would do things which do not block, like flip a bit, flags, integer,
wake up a thread, etc, call the above subsystem's xxx_dev_removal(dev),
and return immediately. (as per my other posting)
(this needs more thinking though)

-- 
Luben

Re: [PATCH] scsi_set_host_offline (resend)

[Oliver Neukum]

> *But*, the _important__point_ is that the LLDD must be able to handle
> requests (queuecommand()) to non-existant (== just removed) devices,
> and not oops, or whatever.

For how long? That's the question.

> So, in effect, you just call usb_dev_removal(dev) (to be written),
> and free _your_ resources on the device. (note important point above)

The way it is in USB is a little different as the USB device drivers do
not go directly to the hardware. It would be:

HCD -> USB core -> LLDD -> generic SCSI -> ...
Steps 1 to 3 work, 4 doesn't.
For a PCMCIA LLDD it would be different, but that really doesn't
matter to the generic SCSI layer.

> > We have indeed discussed this before and the need to notify
> > user space was never questioned, as far as I recall.
> > The point of contention always was whether the notification
> > had to do specific things for the process of unplugging to finish
> > as far as it concerns the low level driver.
>
> Right.  As far as I can see, those subsystem entries, xxx_dev_removal(dev),
> would do things which do not block, like flip a bit, flags, integer,
> wake up a thread, etc, call the above subsystem's xxx_dev_removal(dev),
> and return immediately. (as per my other posting)
> (this needs more thinking though)

If they can't block, they cannot clean up commands still in flight.
Eventually something needs to wait for the outstanding commands.

	Regards
		Oliver

Re: [PATCH] scsi_set_host_offline (resend)

[Luben Tuikov]

Oliver Neukum wrote:
>>*But*, the _important__point_ is that the LLDD must be able to handle
>>requests (queuecommand()) to non-existant (== just removed) devices,
>>and not oops, or whatever.
> 
> 
> For how long? That's the question.

As soon as scsi_dev_removal()/scsi_set_dev_offline() returns
you may assume you can free all _your_ resources.  I.e.
the upper subsystem should handle it from that point on.
 
>>So, in effect, you just call usb_dev_removal(dev) (to be written),
>>and free _your_ resources on the device. (note important point above)
> 
> 
> The way it is in USB is a little different as the USB device drivers do
> not go directly to the hardware. It would be:
> 
> HCD -> USB core -> LLDD -> generic SCSI -> ...
> Steps 1 to 3 work, 4 doesn't.

Not yet, soon maybe.

> If they can't block, they cannot clean up commands still in flight.
> Eventually something needs to wait for the outstanding commands.

I don't see it.

(Upper level: )
Pending commands could be invalidated at once, via their callback
to the upper subsystem; and any new incoming commands to device x
could be invalidated at the moment they come in.

(Lower level: )
Note that during the call to scsi_dev_removal() your driver
may get its host_reset/device_reset function called to abort all
pending commands (so as to leave the device in a predictable state).
At which point, depending on the transport, _your__driver_ may need
to sleep until the task management function response comes back (say over
a network).

All in all, it's not that bad.
-- 
Luben

Re: [PATCH] scsi_set_host_offline (resend)

[Matthew Dharm]

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

I happen to agree with Oliver on this...

As I see it, what we need to do is support both user-initiated removal and
hardware-initiated removal.  Refcounting should get us pretty far along for
this.

As I see it, when either usb-storage or the userspace tells SCSI that a
device is gone/going away, cleanup should begin.  Since everything is
refcounted, the virtual HBA stays around only long enough for cleanup to
occur.

If the request comes from userspace, then scripts should have made sure to
umount/close/sync everything first.  If the device is just yanked, we
simply don't have that opportunity.

Regardless of where the request comes from, the kernel needs to be able to
handle bad user apps -- if nothing else, some app could ignore the message
from the unplug request (regardless of where it comes from).  If the
userspace script did everything well, there are no references and
everything gets cleaned up.  If some app didn't close a device, the SCSI
layer can just DID_ERROR all commands in the mid-layer while the LLDD has
already cleaned-up -- but this is a case that must be handled!

Forcing usb-storage to go outside of kernelspace and then back into
kernelspace is silly and dangerous.  The potential for DoS or other evil is
just too high.

Matt

On Sun, Mar 30, 2003 at 10:23:10AM -0600, James Bottomley wrote:
> On Sat, 2003-03-29 at 16:15, Matthew Dharm wrote:
> > So, the sequence goes something like this:
> > 
> > (1) user removes device, causing disconnect() callback in USB system to be
> > called
> > 
> > (2) usb-storage driver markes device as gone so as to make all commands
> > fail immediately, but does nothing else
> > 
> > (3) hotplug script is notified of USB device removal, figures out that this
> > is a usb-storage device (possibly a USB/SCSI bridge), and invokes the
> > hot-unplug for SCSI for each SCSI device (after unmount, etc.)  NOTE: I
> > don't know how hotplug is supposed to be able to identify the bridge case.
> 
> Ideally, yes.  The driver throwing the hotplug event is supposed to be
> able to distinguish between device and bridge removal.
> 
> > (4) SCSI hot-unplugging offlines the device, then removes them.
> > 
> > (5) Someone notices that the dead virtual host has no devices left, and
> > signals the usb-storage driver that it's safe to unregister?!??!
> 
> That's the (unimplemented in scsi) refcounting in the generic device
> core.  In theory, once the struct device belonging to the host's
> refcount decrements to zero (and you trigger this by doing a device_put
> in the first usb remove), it calls the ->release() hook.  The code
> plugged into this hook knows that the devices are gone, so does the
> final USB cleanup (and the scsi host removal).
> 
> > (6) SCSI mid-layer does cleanup and notifies usb-storage when it is safe to
> > free resources
> > 
> > (7) usb-storage destroys all structures and completes cleanup
> > 
> > I can see how this all works, except for step #5 -- how does usb-storage
> > know that it's time to unregister?
> > 
> > This seems awfully complicated... usb -> usb-storage -> userspace -> scsi
> > -> unknown -> usb-storage -> scsi -> usb-storage is the sequence of events.
> 
> Well, USB storage devices with filesystems on top are rather complex
> things to clean up after.
> 
> > It also means that if the hotplug scripts don't do the job right, we can
> > wind up with lots of dead virtual HBAs lying around.  Not fatal, I guess...
> > but pretty bad.
> > 
> > Originally we had been talking about usb -> usb-storage -> scsi ->
> > usb-storage as the sequence.  userspace never got involved except as a
> > helper -- it was never in the critical path. This also put the
> > responsibility for faking-out devices that had been detached but not yet
> > logically removed in one central place -- with this model, each HBA is
> > responsible for faking-out devices on it's own.
> 
> There's just too much specific knowledge to put into the kernel.  Even
> if SCSI were to offline the device, who does something about the filesys
> or other applications using it?  All you gain is moving one step closer
> to the solution, not the solution itself.
> 
> The way to think about this, I think, is "what would we do if the user
> asked nicely before removing the device" (i.e. requested device ejection
> rather than forced it).  Then the above makes perfect sense because we
> have to start at the top and move down.  Since requested ejection is the
> model for things like hotplug PCI busses, I'm hoping someone else is
> thinking about this.
> 
> As far as the state of play today, I'm not sure if the SCSI refcounting
> is correct or not.  The SCSI list traversals are certainly racy and
> unsafe, and we have no SCSI hotplug event.
> 
> This is probably because no-one who develops for SCSI has access to true
> hotplug hardware.  However, we do have other uses for the hotplug
> infrastructure (FC device addition/removal, even SES device handling),
> we're just not entirely sure how to proceed.
> 
> James
> 
> 
> -
> 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

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

C:  They kicked your ass, didn't they?
S:  They were cheating!
					-- The Chief and Stef
User Friendly, 11/19/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] scsi_set_host_offline (resend)

[Luben Tuikov]

Matthew Dharm wrote:
> I happen to agree with Oliver on this...
> 
> As I see it, what we need to do is support both user-initiated removal and
> hardware-initiated removal.  Refcounting should get us pretty far along for
> this.
> 
> As I see it, when either usb-storage or the userspace tells SCSI that a
> device is gone/going away, cleanup should begin.  Since everything is
> refcounted, the virtual HBA stays around only long enough for cleanup to
> occur.

A way to unifyingly achieve all this is to have an entry function
which is called when a device is being removed, in each subsystem,
a la xxx_dev_removal(dev);

Case 1: LLDD notification of device removal.
I.e. came from the interconnect (USB, Internet, Fibre) fabric.
These events (should) take place:

--->LLDD/interconnect/transport detects device removal from the fabric,
  --->xxx_dev_removal(dev) ***,
    --->scsi_dev_removal(dev),
      --->block_dev_removal(dev),
        --->sysfs_dev_removal(dev) (??? not sure),
          ---> hotplug, userspace, etc.

It then comes back to the LLDD entry.

*** "xxx" could be one of usb, fc, etc, just as a unifying point.
It may not be necessary, and may just call scsi_dev_removal().

The advantage of this intrastructure is that each function
is directly entrant, and doesn't _have_to_ be called from its
lower level equivalent.

The other advantage is (Case 2):


Case 2: Userspace initiated the removal of the device.

---> Userspace calls into kernel to remove a device,
  ---> kernel finds the device and calls xxx_dev_removal(dev).
    ---> the rest of the chain is as Case 1.

This way we get infrastructure/code reusability.


> If the request comes from userspace, then scripts should have made sure to
> umount/close/sync everything first.  If the device is just yanked, we
> simply don't have that opportunity.
> 
> Regardless of where the request comes from, the kernel needs to be able to
> handle bad user apps -- if nothing else, some app could ignore the message
> from the unplug request (regardless of where it comes from).  If the
> userspace script did everything well, there are no references and
> everything gets cleaned up.


> If some app didn't close a device, the SCSI
> layer can just DID_ERROR all commands in the mid-layer while the LLDD has
> already cleaned-up -- but this is a case that must be handled!

This is true.
 
> Forcing usb-storage to go outside of kernelspace and then back into
> kernelspace is silly and dangerous.  The potential for DoS or other evil is
> just too high.

This is true, and what is needed is only for userspace to be _notified_.

Userspace can get the same effect by using some sort of active polling.
Why should it do so, when the kernel can notify it in a nice way,
and such polling implementation (passive, i.e. intr) is much better
handled in the kernel.

I.e. we need the best of both worlds.

-- 
Luben

Re: [PATCH] scsi_set_host_offline (resend)

[Oliver Neukum]

Am Samstag, 29. März 2003 22:54 schrieb James Bottomley:
> On Sat, 2003-03-29 at 14:53, Matthew Dharm wrote:
> > My understanding was that I couldn't call set_device_offline with the
> > host lock held, which is a problem because I need the host lock to
> > traverse the device list.
>
> The solution we're talking about here is from the user level hotplug
> scripts.  Once we get the remove-single-device fixed, you simply use the
> device hot unplug script to traverse the attached SCSI devices, clean up
> the user land (kill processes, unmount filesystems etc.) and then blow
> the devices away.  After the host has no devices, you should simply be
> able to trigger a host removal from within your driver.

It seems you are still trying this overcomplicated going through
user space and back thing.
There's no benefit at all in this. If you make this work you can just
as well make a straightforward thing work. Which will not be a nightmare
to make work right under all circumstances.

	Regards
		Oliver

-
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

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

Oliver Neukum [oliver@neukum.org] wrote:
> Am Samstag, 29. M?rz 2003 22:54 schrieb James Bottomley:
> > On Sat, 2003-03-29 at 14:53, Matthew Dharm wrote:
> > > My understanding was that I couldn't call set_device_offline with the
> > > host lock held, which is a problem because I need the host lock to
> > > traverse the device list.
> >
> > The solution we're talking about here is from the user level hotplug
> > scripts.  Once we get the remove-single-device fixed, you simply use the
> > device hot unplug script to traverse the attached SCSI devices, clean up
> > the user land (kill processes, unmount filesystems etc.) and then blow
> > the devices away.  After the host has no devices, you should simply be
> > able to trigger a host removal from within your driver.
> 
> It seems you are still trying this overcomplicated going through
> user space and back thing.
> There's no benefit at all in this. If you make this work you can just
> as well make a straightforward thing work. Which will not be a nightmare
> to make work right under all circumstances.

Matthew / James I believe I am out of sync with how much is being done
by userspace and how much is being done in the kernel. I believed there
was always userspace action to umount / close devices.

Matthew I know in the previous mail you wrote a numbered list of steps.
I believe item #2 should have called scsi_set_host_offline or
scsi_set_device_offline. Item #6 would only happen if scsi_remove_host
was called sometime prior.

I wrote the following text on the interface I thought we had / where
trying to complete. I would like to add to this so that we possibly
could archive an interface contract between the LLDD and scsi-mid.


2.5 SCSI host register interfaces (scsi_add_host / scsi_remove_host)

1. LLDD call graph standard

(PROBE)
LLDD                  Mid                  Sysfs
scsi_register      -->
scsi_set_device    --> (depreciated)
scsi_add_host      ---+
                      |
               scsi_sysfs_add_host      --> device_register (shost)
               scsi_scan_host         (i)-> device_register (sdev)

(REMOVE)
LLDD                  Mid                  Sysfs
scsi_remove_host  ---+
                     |
               scsi_sysfs_remove_host   --> device_unregister (shost)
               scsi_forget_host       (i)-> device_unregister (sdev)

(hotplug "remove" events for shost and sdev where generated by
device_unregister call).

(sdev device ref cnt == 0)
                                            koject_cleanup
                                                   |
                                           scsi_device_release

(shost device ref cnt == 0)
                                            koject_cleanup
                                                   |
                     +--                    scsi_host_release
                     |
                  shost->hostt->release

2. LLDD call graph offline event

(OFFLINE single device)
LLDD                       Mid                  Sysfs
scsi_set_device_offline  --+
                           |
                        scsi_eh_scmd_add

Note: Do we need hotplug events here or wait until remove???

(OFFLINE host)
LLDD                       Mid                  Sysfs
scsi_set_host_offline    --+
                           |
                      scsi_set_device_offline

(REMOVE)
Same as above.


TODO (1):
scsi_add_host scans the host and when outstanding patches are applied
the scsi_host struct device will have a ref count of 1+N where N is the
number of struct scsi_device's found off the host. In turn each struct
scsi_device struct device will have a ref count of 1+N where N is the
number of upper drivers bound or the number of opens (this may not be
correct for sometime as we do not have an option for the multiple bind
sg issue).


TODO (2):
We need a common method to stop gets to a struct device post a certain
barrier. In the short term scsi could add a scsi_host flag to stop scan
adds, but cannot effect sysfs gets.

TODO (3):
When outstanding patches are applied scsi_remove_host will cause a
device_unregister to be called on all child nodes (sdevs) of scsi_host
and scsi_host (these unregisters will generate hot plug "remove" events)
once the scsi_device ref cnt goes to zero scsi_device_release is called
and once scsi_host ref cnt goes to zero scsi_host_release is called.

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

Re: [PATCH] scsi_set_host_offline (resend)

[Matthew Dharm]

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

On Mon, Mar 31, 2003 at 06:48:06PM -0800, Mike Anderson wrote:
> Matthew / James I believe I am out of sync with how much is being done
> by userspace and how much is being done in the kernel. I believed there
> was always userspace action to umount / close devices.

I have no objection to userspace being notified -- I only object to
requiring userspace to complete the unplug operation.

> Matthew I know in the previous mail you wrote a numbered list of steps.
> I believe item #2 should have called scsi_set_host_offline or
> scsi_set_device_offline. Item #6 would only happen if scsi_remove_host
> was called sometime prior.

The assumption was that #5 was the call to scsi_remove_host() -- the only
question in my mind is: How do I know it is 'safe' to call that?

> I wrote the following text on the interface I thought we had / where
> trying to complete. I would like to add to this so that we possibly
> could archive an interface contract between the LLDD and scsi-mid.

Based on my reading of the following, userspace is notified but not
required to take any action.  Would that be accurate?

> 2.5 SCSI host register interfaces (scsi_add_host / scsi_remove_host)
> 
> 1. LLDD call graph standard
> 
> (PROBE)
> LLDD                  Mid                  Sysfs
> scsi_register      -->
> scsi_set_device    --> (depreciated)
> scsi_add_host      ---+
>                       |
>                scsi_sysfs_add_host      --> device_register (shost)
>                scsi_scan_host         (i)-> device_register (sdev)

This is how I see it (as an LLDD guy) -- I set up my internal state, then
call scsi_register() followed by scsi_add_host().

Then the world starts, and I just keep processing commands whenever I get
one.  Then, someone yanks my plug, and:

> (REMOVE)
> LLDD                  Mid                  Sysfs
> scsi_remove_host  ---+
>                      |
>                scsi_sysfs_remove_host   --> device_unregister (shost)
>                scsi_forget_host       (i)-> device_unregister (sdev)

I presume that I need to offline each device (sdev) before calling
scsi_remove_host(), as previously discussed?  That isn't shown here...

> (hotplug "remove" events for shost and sdev where generated by
> device_unregister call).

Yes.  Userspace is notified, but not required to take any action.

> (sdev device ref cnt == 0)
>                                             koject_cleanup
>                                                    |
>                                            scsi_device_release

Good.  I presume that scsi_device_release() will decrement the ref cnt on
the shost device?

> (shost device ref cnt == 0)
>                                             koject_cleanup
>                                                    |
>                      +--                    scsi_host_release
>                      |
>                   shost->hostt->release

Good.  release() then is responsible for cleaning up.

> 2. LLDD call graph offline event
> 
> (OFFLINE single device)
> LLDD                       Mid                  Sysfs
> scsi_set_device_offline  --+
>                            |
>                         scsi_eh_scmd_add
> 
> Note: Do we need hotplug events here or wait until remove???

Good question.  Here's a better one:  Why do we separately offline the
device before removal?  Why doesn't scsi_remove_host() do it all for us?

For example, when we unload a USB HCD driver, disconnect() gets called for
each device that was on that bus.

> (OFFLINE host)
> LLDD                       Mid                  Sysfs
> scsi_set_host_offline    --+
>                            |
>                       scsi_set_device_offline
> 
> (REMOVE)
> Same as above.

I presume that scsi_set_device_offline in this picture leads to
scsi_set_device_offline(), which leads to scsi_eh_scmd_add()?

> TODO (1):
> scsi_add_host scans the host and when outstanding patches are applied
> the scsi_host struct device will have a ref count of 1+N where N is the
> number of struct scsi_device's found off the host. In turn each struct
> scsi_device struct device will have a ref count of 1+N where N is the
> number of upper drivers bound or the number of opens (this may not be
> correct for sometime as we do not have an option for the multiple bind
> sg issue).

Okay.... I think we're on the same page.  It appears that you're going to
require me to keep resources around for quite some time, until everything
closes/umounts -- but, since every device is offline, I don't have to do
any command processing, yes?  That's good, because then I can free some
significant resources....

> TODO (2):
> We need a common method to stop gets to a struct device post a certain
> barrier. In the short term scsi could add a scsi_host flag to stop scan
> adds, but cannot effect sysfs gets.

Hrm... good point.  How do other busses do this?

> TODO (3):
> When outstanding patches are applied scsi_remove_host will cause a
> device_unregister to be called on all child nodes (sdevs) of scsi_host
> and scsi_host (these unregisters will generate hot plug "remove" events)
> once the scsi_device ref cnt goes to zero scsi_device_release is called
> and once scsi_host ref cnt goes to zero scsi_host_release is called.

Sounds good to me.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

My mother not mind to die for stoppink Windows NT!  She is rememberink 
Stalin!
					-- Pitr
User Friendly, 9/6/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] scsi_set_host_offline (resend)

[Mike Anderson]

Matthew Dharm [mdharm-scsi@one-eyed-alien.net] wrote:
> On Mon, Mar 31, 2003 at 06:48:06PM -0800, Mike Anderson wrote:
> > Matthew / James I believe I am out of sync with how much is being done
> > by userspace and how much is being done in the kernel. I believed there
> > was always userspace action to umount / close devices.
> 
> I have no objection to userspace being notified -- I only object to
> requiring userspace to complete the unplug operation.
> 

If the unplug operation includes cleanup (i.e. free of the memory) then
userspace needs to be involved. 

> > Matthew I know in the previous mail you wrote a numbered list of steps.
> > I believe item #2 should have called scsi_set_host_offline or
> > scsi_set_device_offline. Item #6 would only happen if scsi_remove_host
> > was called sometime prior.
> 
> The assumption was that #5 was the call to scsi_remove_host() -- the only
> question in my mind is: How do I know it is 'safe' to call that?
> 

The changes I am making would allow you to call scsi_remove_host()
anytime and have your release called later when ref counts go to zero.

I am traveling until late Friday night so hopefully I will have
something to post after I get back.

> > I wrote the following text on the interface I thought we had / where
> > trying to complete. I would like to add to this so that we possibly
> > could archive an interface contract between the LLDD and scsi-mid.
> 
> Based on my reading of the following, userspace is notified but not
> required to take any action.  Would that be accurate?
> 

Userspace could take no action. You can make all the interface calls,
but the release calls for the struct scsi_device and struct scsi_host
may not be called if a filesystem or user operation is holding a
reference.

> > 2.5 SCSI host register interfaces (scsi_add_host / scsi_remove_host)
> > 
> > 1. LLDD call graph standard
> > 
> > (PROBE)
> > LLDD                  Mid                  Sysfs
> > scsi_register      -->
> > scsi_set_device    --> (depreciated)
> > scsi_add_host      ---+
> >                       |
> >                scsi_sysfs_add_host      --> device_register (shost)
> >                scsi_scan_host         (i)-> device_register (sdev)
> 
> This is how I see it (as an LLDD guy) -- I set up my internal state, then
> call scsi_register() followed by scsi_add_host().
> 
> Then the world starts, and I just keep processing commands whenever I get
> one.  Then, someone yanks my plug, and:
> 
> > (REMOVE)
> > LLDD                  Mid                  Sysfs
> > scsi_remove_host  ---+
> >                      |
> >                scsi_sysfs_remove_host   --> device_unregister (shost)
> >                scsi_forget_host       (i)-> device_unregister (sdev)
> 
> I presume that I need to offline each device (sdev) before calling
> scsi_remove_host(), as previously discussed?  That isn't shown here...
> 

It is shown below for the offline case. This section was to show that if
no unplug event occurred the caller would not need to call the offline
interfaces to remove a scsi host.

> > (hotplug "remove" events for shost and sdev where generated by
> > device_unregister call).
> 
> Yes.  Userspace is notified, but not required to take any action.
> 

Correct.

> > (sdev device ref cnt == 0)
> >                                             koject_cleanup
> >                                                    |
> >                                            scsi_device_release
> 
> Good.  I presume that scsi_device_release() will decrement the ref cnt on
> the shost device?
> 

Correct.

> > (shost device ref cnt == 0)
> >                                             koject_cleanup
> >                                                    |
> >                      +--                    scsi_host_release
> >                      |
> >                   shost->hostt->release
> 
> Good.  release() then is responsible for cleaning up.
> 

Yes the host template release function would clean its internal data and
call scsi_unregister to free the scsi_host structure.

> > 2. LLDD call graph offline event
> > 
> > (OFFLINE single device)
> > LLDD                       Mid                  Sysfs
> > scsi_set_device_offline  --+
> >                            |
> >                         scsi_eh_scmd_add
> > 
> > Note: Do we need hotplug events here or wait until remove???
> 
> Good question.  Here's a better one:  Why do we separately offline the
> device before removal?  Why doesn't scsi_remove_host() do it all for us?
> 

My thoughts where that they where two separate actions. 

scsi_set_device_offline / host_offline may be called in the future and
then onlined without calling scsi_remove_host  (i.e. we need the online
counter parts to these functions to make them symmetric).

A LLLD may wish to call scsi_remove_host while users are still active
and only be removed once the users are off the device and drained.

To make things more confusing scsi_remove_host does set a device to the
offline state, but I believe this could be changes to not do this
action.

> For example, when we unload a USB HCD driver, disconnect() gets called for
> each device that was on that bus.
> 
> > (OFFLINE host)
> > LLDD                       Mid                  Sysfs
> > scsi_set_host_offline    --+
> >                            |
> >                       scsi_set_device_offline
> > 
> > (REMOVE)
> > Same as above.
> 
> I presume that scsi_set_device_offline in this picture leads to
> scsi_set_device_offline(), which leads to scsi_eh_scmd_add()?

Yes,
	scsi_set_host_offline will call scsi_set_device_offline for all
	children devices of the host and if IOs are active
	scsi_eh_scmd_add will cancel those commands.
> 
> > TODO (1):
> > scsi_add_host scans the host and when outstanding patches are applied
> > the scsi_host struct device will have a ref count of 1+N where N is the
> > number of struct scsi_device's found off the host. In turn each struct
> > scsi_device struct device will have a ref count of 1+N where N is the
> > number of upper drivers bound or the number of opens (this may not be
> > correct for sometime as we do not have an option for the multiple bind
> > sg issue).
> 
> Okay.... I think we're on the same page.  It appears that you're going to
> require me to keep resources around for quite some time, until everything
> closes/umounts -- but, since every device is offline, I don't have to do
> any command processing, yes?  That's good, because then I can free some
> significant resources....
> 

Yes, The goal is that no more IO will be sent to driver.

> > TODO (2):
> > We need a common method to stop gets to a struct device post a certain
> > barrier. In the short term scsi could add a scsi_host flag to stop scan
> > adds, but cannot effect sysfs gets.
> 
> Hrm... good point.  How do other busses do this?
> 

I believe others do not need this at least in your example there is one
device per host. Since SCSI provides the scan interface it might be the
earliest and best place to stop future registers.

> > TODO (3):
> > When outstanding patches are applied scsi_remove_host will cause a
> > device_unregister to be called on all child nodes (sdevs) of scsi_host
> > and scsi_host (these unregisters will generate hot plug "remove" events)
> > once the scsi_device ref cnt goes to zero scsi_device_release is called
> > and once scsi_host ref cnt goes to zero scsi_host_release is called.
> 
> Sounds good to me.
> 

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