Re: Notify about errors during initialization

Re: Notify about errors during initialization

[Alan Stern]

Cross-posted to the linux-scsi list in the hope that someone there can 
confirm this answer...

On Sat, 6 Dec 2003, Matthew Dharm wrote:

> Question: Are we guaranteed that scsi_scan_host() doesn't return until the
> scanning process is complete?

I'm not absolutely certain -- best to check with someone on the linux-scsi
list.  But a quick look through the call chain in scsi_scan.c indicates
that scsi_scan_host() doesn't return until the scanning process is indeed
complete.  The real work is done in scsi_probe_lun() (called from
scsi_probe_and_add_lun()), and that uses scsi_wait_req().  After all, it
can't tell whether the LUN is there or not until it gets a reply to the
INQUIRY!

> I recall a problem that we had with the setting of flags and the call to
> scan the host -- without pre-emption, it worked, but with pre-emption
> things happened in the wrong order.  Could that happen here?

I don't remember that problem.  But I don't think pre-emption would cause 
any difficulties here.  All the flags should be set before we call 
scsi_scan_host().  If you can provide more details or a reference to what 
you're thinking of, I could reply in more detail.

When pre-emption does cause a problem it's generally because of some
assumption that the current task won't be scheduled out when it calls up()
or complete() or something like that.

Alan Stern

Re: Notify about errors during initialization

[Matthew Dharm]

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

Did you ever get an answer from the linux-scsi people on this?

Matt

On Mon, Dec 08, 2003 at 11:15:06AM -0500, Alan Stern wrote:
> Cross-posted to the linux-scsi list in the hope that someone there can 
> confirm this answer...
> 
> On Sat, 6 Dec 2003, Matthew Dharm wrote:
> 
> > Question: Are we guaranteed that scsi_scan_host() doesn't return until the
> > scanning process is complete?
> 
> I'm not absolutely certain -- best to check with someone on the linux-scsi
> list.  But a quick look through the call chain in scsi_scan.c indicates
> that scsi_scan_host() doesn't return until the scanning process is indeed
> complete.  The real work is done in scsi_probe_lun() (called from
> scsi_probe_and_add_lun()), and that uses scsi_wait_req().  After all, it
> can't tell whether the LUN is there or not until it gets a reply to the
> INQUIRY!
> 
> > I recall a problem that we had with the setting of flags and the call to
> > scan the host -- without pre-emption, it worked, but with pre-emption
> > things happened in the wrong order.  Could that happen here?
> 
> I don't remember that problem.  But I don't think pre-emption would cause 
> any difficulties here.  All the flags should be set before we call 
> scsi_scan_host().  If you can provide more details or a reference to what 
> you're thinking of, I could reply in more detail.
> 
> When pre-emption does cause a problem it's generally because of some
> assumption that the current task won't be scheduled out when it calls up()
> or complete() or something like that.
> 
> Alan Stern
> 
> 
> 
> 
> 
> 

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

I'm just trying to think of a way to say "up yours" without getting fired.
					-- Stef
User Friendly, 10/8/1998

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

Re: Notify about errors during initialization

[Alan Stern]

James:

Can you or someone else on the SCSI development list answer this question?

When a new host is registered and scanned for devices, are we guaranteed
that the scanning procedure will be finished when scsi_scan_host()  
returns?

It sure looks that way to me, but I want to be certain -- Matt Dharm 
expressed some doubts.

The reason for asking is so that the usb-storage driver can test whether 
any devices were found.  Depressingly often some incompatibility causes 
the scan to fail, and when that happens it would be nice to put a 
notification in the system log, unregister the host, and generally avoid 
loading the driver as though everything was okay.

Thanks,

Alan Stern


> > On Sat, 6 Dec 2003, Matthew Dharm wrote:
> > 
> > > Question: Are we guaranteed that scsi_scan_host() doesn't return until the
> > > scanning process is complete?
> > 
> > > I recall a problem that we had with the setting of flags and the call to
> > > scan the host -- without pre-emption, it worked, but with pre-emption
> > > things happened in the wrong order.  Could that happen here?

Re: Notify about errors during initialization

[Christoph Hellwig]

On Mon, Jan 05, 2004 at 05:04:59PM -0500, Alan Stern wrote:
> James:
> 
> Can you or someone else on the SCSI development list answer this question?
> 
> When a new host is registered and scanned for devices, are we guaranteed
> that the scanning procedure will be finished when scsi_scan_host()  
> returns?

With the current code the scanning activity has finished when we return.
But I'm not sure whether we should document that assumption - if we're
moving to parallel scanning that assumption might soon be void.

> The reason for asking is so that the usb-storage driver can test whether 
> any devices were found.  Depressingly often some incompatibility causes 
> the scan to fail, and when that happens it would be nice to put a 
> notification in the system log, unregister the host, and generally avoid 
> loading the driver as though everything was okay.

Hmm.  maybe we should guarantee that scsi_scan_host will have finished
and introduce a scsi_scan_host_parallel when we go down that route.  Or
do it the other way and add a serialized version for usb-storage..

Re: Notify about errors during initialization

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> On Mon, Jan 05, 2004 at 05:04:59PM -0500, Alan Stern wrote:
> > James:
> > 
> > Can you or someone else on the SCSI development list answer this question?
> > 
> > When a new host is registered and scanned for devices, are we guaranteed
> > that the scanning procedure will be finished when scsi_scan_host()  
> > returns?
> 
> With the current code the scanning activity has finished when we return.
> But I'm not sure whether we should document that assumption - if we're
> moving to parallel scanning that assumption might soon be void.
> 

It would appear that saying the scan requested by scsi_scan_host is
completed when the function returns would be the expected result. The
LLDD writer should be aware that once scsi_add_host has been called
hotplug events to user space could allow the user space to scan and
delete devices through sysfs making assumptions on devices found or not
found iffy.

> > The reason for asking is so that the usb-storage driver can test whether 
> > any devices were found.  Depressingly often some incompatibility causes 
> > the scan to fail, and when that happens it would be nice to put a 
> > notification in the system log, unregister the host, and generally avoid 
> > loading the driver as though everything was okay.
> 
> Hmm.  maybe we should guarantee that scsi_scan_host will have finished
> and introduce a scsi_scan_host_parallel when we go down that route.  Or
> do it the other way and add a serialized version for usb-storage..

I thought we where moving to not calling a scan api from the LLDD and
using hotplug events. Then user space would define the scan policy it
wanted.

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

Re: Notify about errors during initialization

[Christoph Hellwig]

On Mon, Jan 05, 2004 at 03:32:21PM -0800, Mike Anderson wrote:
> It would appear that saying the scan requested by scsi_scan_host is
> completed when the function returns would be the expected result. The
> LLDD writer should be aware that once scsi_add_host has been called
> hotplug events to user space could allow the user space to scan and
> delete devices through sysfs making assumptions on devices found or not
> found iffy.

> ...

> I thought we where moving to not calling a scan api from the LLDD and
> using hotplug events. Then user space would define the scan policy it
> wanted.

My idea was that in the brave new hotplug world scsi_scan_host would
do nothing but simply sending a SCAN hotplug even.  With that design
we'd have a hard time to find out when the scanning has finished in
kernelspace.

Re: Notify about errors during initialization

[Patrick Mansfield]

On Tue, Jan 06, 2004 at 04:17:31PM +0000, Christoph Hellwig wrote:

> My idea was that in the brave new hotplug world scsi_scan_host would
> do nothing but simply sending a SCAN hotplug even.  With that design
> we'd have a hard time to find out when the scanning has finished in
> kernelspace.

Why not trigger off the scsi_host hotplug event? Though we will have races
similiar to udev where the sysfs attribute files are created after the
hotplug event - we really ought to have a device_init/setup, create the
sysfs files, then device_add/enable.

For user space scanning, we could special case the USB driver to check
that we find the expected number of targets.

-- Patrick Mansfield

Re: Notify about errors during initialization

[Christoph Hellwig]

On Tue, Jan 06, 2004 at 09:15:39AM -0800, Patrick Mansfield wrote:
> Why not trigger off the scsi_host hotplug event? Though we will have races
> similiar to udev where the sysfs attribute files are created after the
> hotplug event - we really ought to have a device_init/setup, create the
> sysfs files, then device_add/enable.

Because some drivers (like the ieee1394) don't want scanning to happen.
They can easily chose that by not sending a scan event.

Sysfs attribute file for max_sectors

[Alan Stern]

Some USB Mass Storage devices need max_sectors set as low as 64 KB per
transfer.  Many devices don't have this restriction, though, and it would
be nice to give users an easy way to change the value.  Especially if we
play it safe by starting with the default value set low; that way more
capable devices wouldn't have to be penalized.

The simplest way to export max_sectors to user processes is as a sysfs 
attribute.  We have code that will do this for USB devices, but perhaps it 
would be better as a general feature available for all SCSI devices.

What's your feeling?  Should max_sectors be exported for all SCSI devices 
or only for USB mass storage devices?

Alan Stern

Re: Notify about errors during initialization

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> On Tue, Jan 06, 2004 at 09:15:39AM -0800, Patrick Mansfield wrote:
> > Why not trigger off the scsi_host hotplug event? Though we will have races
> > similiar to udev where the sysfs attribute files are created after the
> > hotplug event - we really ought to have a device_init/setup, create the
> > sysfs files, then device_add/enable.
> 
> Because some drivers (like the ieee1394) don't want scanning to happen.
> They can easily chose that by not sending a scan event.
> 

Couldn't the user space also set the policy to scan or not scan? It
would seem that user space would be more flexible and not need a new
api (Just do not call scsi_scan_host).


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

Re: Notify about errors during initialization

[Christoph Hellwig]

On Tue, Jan 06, 2004 at 10:08:14AM -0800, Mike Anderson wrote:
> > Because some drivers (like the ieee1394) don't want scanning to happen.
> > They can easily chose that by not sending a scan event.
> > 
> 
> Couldn't the user space also set the policy to scan or not scan? It
> would seem that user space would be more flexible and not need a new
> api (Just do not call scsi_scan_host).

For ieee1394 scanning doesn't make any sense, so I don't see why it should
scan at all.

And I don't understand that API argument at all.  With my suggestion
scsi_scan_host would be changed to send the hotplug even - not change in
API at all.

Re: Sysfs attribute file for max_sectors

[James Bottomley]

On Tue, 2004-01-06 at 12:07, Alan Stern wrote:
> Some USB Mass Storage devices need max_sectors set as low as 64 KB per
> transfer.  Many devices don't have this restriction, though, and it would
> be nice to give users an easy way to change the value.  Especially if we
> play it safe by starting with the default value set low; that way more
> capable devices wouldn't have to be penalized.

This is treading on dangerous ground.  Setting max_sectors too high is a
sure fire way to hang a driver.  Its value isn't really "policy" which
belongs in user land, its an expression of a driver constraint.

> The simplest way to export max_sectors to user processes is as a sysfs 
> attribute.  We have code that will do this for USB devices, but perhaps it 
> would be better as a general feature available for all SCSI devices.
> 
> What's your feeling?  Should max_sectors be exported for all SCSI devices 
> or only for USB mass storage devices?

The value is useful to know for all drivers, so exporting it in sysfs
should be fine.  If you want to do an attribute override in usb-storage,
you can...but I'd be very careful not to let the user set it to an
obviously wrong value (like larger than the sg array or something).

James

Re: Notify about errors during initialization

[Mike Anderson]

Christoph Hellwig [hch@infradead.org] wrote:
> On Tue, Jan 06, 2004 at 10:08:14AM -0800, Mike Anderson wrote:
> > > Because some drivers (like the ieee1394) don't want scanning to happen.
> > > They can easily chose that by not sending a scan event.
> > > 
> > 
> > Couldn't the user space also set the policy to scan or not scan? It
> > would seem that user space would be more flexible and not need a new
> > api (Just do not call scsi_scan_host).
> 
> For ieee1394 scanning doesn't make any sense, so I don't see why it should
> scan at all.
> 
> And I don't understand that API argument at all.  With my suggestion
> scsi_scan_host would be changed to send the hotplug even - not change in
> API at all.

ok, I was reading two replies back when you mentioned
"scsi_scan_host_parallel". I re-read the last reply and see that
scsi_scan_host would just send a scan event. 

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

Re: Sysfs attribute file for max_sectors

[Alan Stern]

On 6 Jan 2004, James Bottomley wrote:

> > The simplest way to export max_sectors to user processes is as a sysfs 
> > attribute.  We have code that will do this for USB devices, but perhaps it 
> > would be better as a general feature available for all SCSI devices.
> > 
> > What's your feeling?  Should max_sectors be exported for all SCSI devices 
> > or only for USB mass storage devices?
> 
> The value is useful to know for all drivers, so exporting it in sysfs
> should be fine.  If you want to do an attribute override in usb-storage,
> you can...but I'd be very careful not to let the user set it to an
> obviously wrong value (like larger than the sg array or something).

I was going to have a store() routine that would accept any value greater
than 0 and <= the value in the host template (or SCSI_DEFAULT_MAX_SECTORS
if the host template value is 0 -- that's what scsi_host_alloc() does).  
If this is writeable only by root, would that be acceptable for all SCSI
devices?

Alan Stern

Re: Sysfs attribute file for max_sectors

[James Bottomley]

On Tue, 2004-01-06 at 13:31, Alan Stern wrote:
> I was going to have a store() routine that would accept any value greater
> than 0 and <= the value in the host template (or SCSI_DEFAULT_MAX_SECTORS
> if the host template value is 0 -- that's what scsi_host_alloc() does).  
> If this is writeable only by root, would that be acceptable for all SCSI
> devices?

Not really.  For most drivers, the max_sectors is a driver table
parameter and the user shouldn't be changing it.

The value can be acceptably exported read only in scsi_sysfs.c;  then,
you could use the attribute overriding code to make this read/write for
usb-storage only.

James

Re: Sysfs attribute file for max_sectors

[Patrick Mansfield]

Alan -

Given that we currently don't have any user level intitiated scanning, how
can you modify max_sectors in user space prior to the scsi_alloc_queue()
call done during scan?

Is there a block interface to modify max_sectors?

-- Patrick Mansfield

Re: Sysfs attribute file for max_sectors

[Jens Axboe]

On Tue, Jan 06 2004, Patrick Mansfield wrote:
> Alan -
> 
> Given that we currently don't have any user level intitiated scanning, how
> can you modify max_sectors in user space prior to the scsi_alloc_queue()
> call done during scan?
> 
> Is there a block interface to modify max_sectors?

No. As James very clearly stated (and that I back 100%), max_sectors is
a driver template setting. It doesn't make sense to expose it, it's not
a configurable item.

-- 
Jens Axboe

Re: Sysfs attribute file for max_sectors

[Alan Stern]

On Tue, 6 Jan 2004, Patrick Mansfield wrote:

> Alan -
> 
> Given that we currently don't have any user level intitiated scanning, how
> can you modify max_sectors in user space prior to the scsi_alloc_queue()
> call done during scan?

We can't.  But scan doesn't put much of a load on a device, and all sd
initially does is read partition table entries from single sectors
scattered around a disk.  Even mount doesn't seem to transfer very large
buffers.  It's only when people start using the devices and doing large
contiguous data transfers that we begin running against limitations.

So for us it would be enough to have scsi_slave_configure() set 
max_sectors to 128, insuring that this default value is workable on every 
device.  Then user space tools could increase that value for more capable 
devices at any time and reap the benefits of decreased overhead.

Making the store() method a USB-specific override is okay with me.

Alan Stern

PATCH: (as168) Sysfs attribute file for max_sectors

[Alan Stern]

On 6 Jan 2004, James Bottomley wrote:

> Not really.  For most drivers, the max_sectors is a driver table
> parameter and the user shouldn't be changing it.
> 
> The value can be acceptably exported read only in scsi_sysfs.c;  then,
> you could use the attribute overriding code to make this read/write for
> usb-storage only.

Is this patch okay?  (Along with exporting max_sectors read-only, it fixes
two small typos.)

Alan Stern


===== scsi_sysfs.c 1.24 vs edited =====
--- 1.24/drivers/scsi/scsi_sysfs.c	Wed Nov 26 18:52:56 2003
+++ edited/drivers/scsi/scsi_sysfs.c	Wed Jan  7 11:11:44 2004
@@ -200,7 +200,7 @@
 
 
 /*
- * sdev_rd_attr: create a function and attribute variable for a
+ * sdev_rw_attr: create a function and attribute variable for a
  * read/write field.
  */
 #define sdev_rw_attr(field, format_string)				\
@@ -217,7 +217,7 @@
 static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field)
 
 /*
- * sdev_rd_attr: create a function and attribute variable for a
+ * sdev_rw_attr: create a function and attribute variable for a
  * read/write bit field.
  */
 #define sdev_rw_attr_bit(field)						\
@@ -283,6 +283,16 @@
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
+static ssize_t
+sdev_show_max_sectors(struct device *dev, char *buf)
+{
+	struct scsi_device *sdev;
+	sdev = to_scsi_device(dev);
+	return snprintf (buf, 20, "%d\n", sdev->request_queue->max_sectors);
+}
+static DEVICE_ATTR(max_sectors, S_IRUGO, sdev_show_max_sectors, NULL);
+
+
 /* Default template for device attributes.  May NOT be modified */
 static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
 	&dev_attr_device_blocked,
@@ -295,6 +305,7 @@
 	&dev_attr_online,
 	&dev_attr_rescan,
 	&dev_attr_delete,
+	&dev_attr_max_sectors,
 	NULL
 };