Re: [RFC] FC transport: Disable LUN scanning from low level driver
I'd prefer a flag/bit in the fc template to indicate no scanning by the
transport. Thus, it's up to the driver to call __scsi_add_device, or
perform the appropriate scans...
Note: this doesn't stop the problem in absolute. as long as there's scan
interfaces via sysfs, any tool/admin could request a scan and hit your
issues. To truly solve it, you need, within the LLDD, to munge what the
midlayer sees for LUN lists.
-- james s
Christof Schmitt wrote:
The FC transport class calls scsi_scan_target with the SCAN_WILD_CARD
flag to automatically scan for logical units. zfcp, on the other hand,
only uses the units that are configured via the zfcp sysfs interface.
The main reason for this, is that the adapter behind zfcp supports
adapter sharing without NPIV: The adapter is logged into the SAN once,
and each unit can be used by one Linux system. If one Linux would grab
all LUNs, no other one can use them.
If a unit has the LUN 0, then the SCSI midlayer issue a REPORT LUNS,
checks the found LUN against the max_lun of the hostadapter. zfcp sets
this to 1 to only use its own managed units. If there is a LUN 0 that
is used by zfcp, then the SCSI midlayer produces lots of messages like
"scsi: host 0 channel 0 id 1 lun2 has a LUN larger than allowed by the
host adapter" to indicate the mismatch between the actual LUN and the
max_lun setting. These messages only confuse a user in the case of
zfcp, since there is no error.
To fix this problem, the LLD (zfcp) has to be able to prevent the
automatic scanning from the FC transport class. Attached is a patch
that adds a parameter to fc_remote_port_add(), another approach would
be an additional flag in the FC transport template.
What do others think? If there is an agreement, i will followup with a
new patch.
This can go away, as soon as zfcp does not have to support the
non-NPIV adapter sharing anymore, but this won't happen in the
foreseeable future.
Christof Schmitt
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-08-14 17:00:07.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-08-20 12:54:03.0
+0200
@@ -3175,7 +3175,7 @@ zfcp_erp_action_cleanup(int action, stru
ids.port_id = port->d_id;
ids.roles = FC_RPORT_ROLE_FCP_TARGET;
port->rport =
- fc_remote_port_add(adapter->scsi_host, 0, &ids);
+ fc_remote_port_add(adapter->scsi_host, 0, &ids,
0);
if (!port->rport)
ZFCP_LOG_NORMAL("failed registration of rport"
"(adapter %s, wwpn=0x%016Lx)\n",
--- linux-2.6.orig/drivers/scsi/scsi_transport_fc.c 2007-08-14
17:00:07.0 +0200
+++ linux-2.6/drivers/scsi/scsi_transport_fc.c 2007-08-20 12:56:15.0
+0200
@@ -2360,7 +2360,7 @@ fc_rport_final_delete(struct work_struct
**/
static struct fc_rport *
fc_rport_create(struct Scsi_Host *shost, int channel,
- struct fc_rport_identifiers *ids)
+ struct fc_rport_identifiers *ids, int scan_target)
{
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
struct fc_internal *fci = to_fc_internal(shost->transportt);
@@ -2424,6 +2424,9 @@ fc_rport_create(struct Scsi_Host *shost,
transport_add_device(dev);
transport_configure_device(dev);
+ if (scan_target)
+ rport->flags |= FC_RPORT_SCAN_TARGET;
+
if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
/* initiate a scan of the target */
rport->flags |= FC_RPORT_SCAN_PENDING;
@@ -2484,7 +2487,7 @@ delete_rport:
**/
struct fc_rport *
fc_remote_port_add(struct Scsi_Host *shost, int channel,
- struct fc_rport_identifiers *ids)
+ struct fc_rport_identifiers *ids, int scan_target)
{
struct fc_internal *fci = to_fc_internal(shost->transportt);
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
@@ -2574,6 +2577,10 @@ fc_remote_port_add(struct Scsi_Host *sho
spin_lock_irqsave(shost->host_lock, flags);
rport->flags &= ~FC_RPORT_DEVLOSS_PENDING;
+ if (scan_target)
+ rport->flags |= FC_RPORT_SCAN_TARGET;
+ else
+ rport->flags &= ~FC_RPORT_SCAN_TARGET;
/* if target, initiate a scan */
if (rport->scsi_target_id != -1) {
@@ -2657,7 +2664,7 @@ fc_remote_port_add(struct Scsi_Host *sho
spin_unlock_irqrestore(shost->host_lock, flags);
/* No consistent binding found - create new remote port entry */
- rport = fc_rport_create(shost, channel, ids);
+ rport = fc_rport_create(shost, channel, ids, scan_target);
return rport;
}
@@ -2991,7 +2998,8 @@ fc_scsi_scan_rport(struct work_struct *w
unsigned long flags;
if ((rport->port_state == FC
more SCSI API doc. question
Where should other (undocumented) SCSI API docs be added? I'll be glad to send updates/patches/ E.g.: scsi_bufflen(cmnd) scsi_sg_count(cmnd) scsi_sglist(cmnd) scsi_get_resid(cmnd) scsi_set_resid(cmnd, int) and all kernel-doc blocks from scsi_lib.c Thanks, --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi_scan: Cope with kthread_run failing
Oops. I fail to drive git-send-email properly again.
James, when you're committing this, please use the
Subject: [PATCH] scsi_scan: Cope with kthread_run failing
On Mon, Aug 20, 2007 at 09:18:48AM -0600, Matthew Wilcox wrote:
> If kthread_run failed, we would fail to scan the host, and leak the
> allocated async_scan_data. Since using a separate thread is just an
> optimisation, do the scan synchronously if we fail to spawn a thread.
>
> Signed-off-by: Matthew Wilcox <[EMAIL PROTECTED]>
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 309b224..a001ca1 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1799,6 +1799,7 @@ static int do_scan_async(void *_data)
> **/
> void scsi_scan_host(struct Scsi_Host *shost)
> {
> + struct task_struct *p;
> struct async_scan_data *data;
>
> if (strncmp(scsi_scan_type, "none", 4) == 0)
> @@ -1810,7 +1811,9 @@ void scsi_scan_host(struct Scsi_Host *shost)
> return;
> }
>
> - kthread_run(do_scan_async, data, "scsi_scan_%d", shost->host_no);
> + p = kthread_run(do_scan_async, data, "scsi_scan_%d", shost->host_no);
> + if (IS_ERR(p))
> + do_scan_async(data);
> }
> EXPORT_SYMBOL(scsi_scan_host);
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[no subject]
If kthread_run failed, we would fail to scan the host, and leak the
allocated async_scan_data. Since using a separate thread is just an
optimisation, do the scan synchronously if we fail to spawn a thread.
Signed-off-by: Matthew Wilcox <[EMAIL PROTECTED]>
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 309b224..a001ca1 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1799,6 +1799,7 @@ static int do_scan_async(void *_data)
**/
void scsi_scan_host(struct Scsi_Host *shost)
{
+ struct task_struct *p;
struct async_scan_data *data;
if (strncmp(scsi_scan_type, "none", 4) == 0)
@@ -1810,7 +1811,9 @@ void scsi_scan_host(struct Scsi_Host *shost)
return;
}
- kthread_run(do_scan_async, data, "scsi_scan_%d", shost->host_no);
+ p = kthread_run(do_scan_async, data, "scsi_scan_%d", shost->host_no);
+ if (IS_ERR(p))
+ do_scan_async(data);
}
EXPORT_SYMBOL(scsi_scan_host);
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch] plug async scan race at 1st node scan
On Mon, Aug 20, 2007 at 10:02:45AM -0400, James Smart wrote:
> Well - depends on what the semantics of scan_start are to date, there
> really are none and what does requiring it mean ?
OK, and to make this discussion exceptionally useful, all comments are
to be made in the context of modifications to the documentation:
/*
* If the host wants to be called before the scan starts, but
* after the midlayer has set up ready for the scan, it can fill
* in this function.
*/
When I say "require it", I mean that currently we have the code:
if (shost->hostt->scan_finished) {
unsigned long start = jiffies;
if (shost->hostt->scan_start)
shost->hostt->scan_start(shost);
I propose removing that second 'if':
if (shost->hostt->scan_finished) {
unsigned long start = jiffies;
shost->hostt->scan_start(shost);
And here's my proposal for new documentation for scan_start():
/*
* If the host fills in scan_finished, above, it must also fill in
* scan_start. scan_start will be called by the midlayer to inform
* the host that it is now ready to receive requests to scan targets.
*/
> What's implied is that you want "bring up link" to be enabled in
> scan_start().
That's certainly one obvious way to do it. Another way would be to have
a flag in the driver blocking calls to the midlayer ... I think you have
that already?
> Doable, but the code paths weren't put together expecting this, so it may be
> a bit of work. I'll have to look at it. Also, you're asking me to fix one
> driver, without thinking about the structure in others I'd rather the
> api itself locked down state/behavior, not simply the LLD coding.
I think other drivers already do this. We only have three drivers
currently using this API -- lpfc, qla2xxx and aic94xx. asd_scan_start()
enables the PHYs. qla2xxx_scan_start sets some bits, but I'm not quite
sure of their effect.
> Before starting, I'd rather we setting on what the semantics of the api is.
> To the uninitiated, requiring a driver to call scsi_scan_host(), when the
> transport drives all discovery, and where the scan_host really has nothing
> to do with scanning, but rather creates a firewall delay to hold off
> serializing
> of device enumerations - is all very confusing. I'd rather we had a
> different
> entry point for those things that supply start/finish routines and it was
> named more in line with "start discovery delay".
The name certainly isn't great, but I thought we agreed that having a
common entry point for all SCSI hosts was a good idea.
> Adding the notion of scan_start bringing up the link sounds reasonable.
> However,
> how does this translate to other bus types ?
It works for SAS and FC ... I don't see why it wouldn't work for other
bus types. Obviously, we don't call it for SPI.
> -- james s
>
> Matthew Wilcox wrote:
> >On Mon, Aug 20, 2007 at 09:10:35AM -0400, James Smart wrote:
> >>In testing 2.6.23-rc3, there is a small window where the async-per-target
> >>scan of the transport can beat the call from the LLDD to scsi_scan_host().
> >
> >I'd assumed that events wouldn't come in until ->scan_start was called.
> >I see lpfc doesn't have one; is it possible to restructure it to have one?
> >
> >(In any case, good job tracking this down; it was really annoying me.)
> >
> >Possibly we should be less forgiving, and require drivers to have a
> >scan_start, otherwise they can't avoid this race.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add use_sg_chaining option to scsi_host_template
On Mon, 20 Aug 2007 15:05:22 +0200 Jens Axboe <[EMAIL PROTECTED]> wrote: > On Mon, Aug 20 2007, FUJITA Tomonori wrote: > > On Mon, 20 Aug 2007 09:10:31 +0200 > > Jens Axboe <[EMAIL PROTECTED]> wrote: > > > > > On Sat, Aug 18 2007, FUJITA Tomonori wrote: > > > > On Fri, 17 Aug 2007 01:47:59 +0900 > > > > FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > > > > > > > > > This is for Jens' sglist branch in the block git tree. > > > > > > > > > > It enables sg chaining support for the LLDs that use scsi_for_each_sg > > > > > accessor properly. > > > > > > > > > > --- > > > > > >From a6e50a3b476bc193de103e8c1d95877ced38918e Mon Sep 17 00:00:00 > > > > > >2001 > > > > > From: FUJITA Tomonori <[EMAIL PROTECTED]> > > > > > Date: Fri, 17 Aug 2007 01:35:41 +0900 > > > > > Subject: [PATCH] add use_sg_chaining option to scsi_host_template > > > > > > > > > > This option is true if a low-level driver can support sg > > > > > chaining. This will be removed eventually when all the drivers are > > > > > converted to support sg chaining. q->max_phys_segments is set to > > > > > SCSI_MAX_SG_SEGMENTS if false. > > > > > > > > > > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> > > > > > --- > > > > > arch/ia64/hp/sim/simscsi.c|1 + > > > > > drivers/scsi/3w-9xxx.c|1 + > > > > > drivers/scsi/3w-.c|1 + > > > > > drivers/scsi/BusLogic.c |1 + > > > > > drivers/scsi/NCR53c406a.c |3 ++- > > > > > drivers/scsi/a100u2w.c|1 + > > > > > drivers/scsi/aacraid/linit.c |1 + > > > > > drivers/scsi/aha1740.c|1 + > > > > > drivers/scsi/aic7xxx/aic79xx_osm.c|1 + > > > > > drivers/scsi/aic7xxx/aic7xxx_osm.c|1 + > > > > > drivers/scsi/aic7xxx_old.c|1 + > > > > > drivers/scsi/arcmsr/arcmsr_hba.c |1 + > > > > > drivers/scsi/dc395x.c |1 + > > > > > drivers/scsi/dpt_i2o.c|1 + > > > > > drivers/scsi/eata.c |3 ++- > > > > > drivers/scsi/hosts.c |1 + > > > > > drivers/scsi/hptiop.c |1 + > > > > > drivers/scsi/ibmmca.c |1 + > > > > > drivers/scsi/ibmvscsi/ibmvscsi.c |1 + > > > > > drivers/scsi/initio.c |1 + > > > > > drivers/scsi/ipr.c|1 + > > > > > > > > I should have dropped ipr since we haven't converted libata yet. > > > > > > But we have, are there still bits missing? > > > > I'm just a bit nervous about possible bugs. > > I hope there aren't any left in libata :-) > > > Probably we need to remove blk_queue_max_phys_segments() in libata > > for a possible lower sg list size (< 128). > > Indeed, that should go, even in mainline now. > > > BTW, I think that it would be good to test the sglist with a lower sg > > list size (like 32) to test all the llds. > > That's a good idea, then we'll get the chaining stuff exercised much > more easily! Yeah, if you push sglist with setting SCSI_MAX_SG_SEGMENTS to 32, sglist can get tons of tests. I didn't found the ppc sglist bugs and didn't lose my data until I tried a small sg list size. :) - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch] plug async scan race at 1st node scan
Well - depends on what the semantics of scan_start are to date, there really are none and what does requiring it mean ? What's implied is that you want "bring up link" to be enabled in scan_start(). Doable, but the code paths weren't put together expecting this, so it may be a bit of work. I'll have to look at it. Also, you're asking me to fix one driver, without thinking about the structure in others I'd rather the api itself locked down state/behavior, not simply the LLD coding. Before starting, I'd rather we setting on what the semantics of the api is. To the uninitiated, requiring a driver to call scsi_scan_host(), when the transport drives all discovery, and where the scan_host really has nothing to do with scanning, but rather creates a firewall delay to hold off serializing of device enumerations - is all very confusing. I'd rather we had a different entry point for those things that supply start/finish routines and it was named more in line with "start discovery delay". Adding the notion of scan_start bringing up the link sounds reasonable. However, how does this translate to other bus types ? -- james s Matthew Wilcox wrote: On Mon, Aug 20, 2007 at 09:10:35AM -0400, James Smart wrote: In testing 2.6.23-rc3, there is a small window where the async-per-target scan of the transport can beat the call from the LLDD to scsi_scan_host(). I'd assumed that events wouldn't come in until ->scan_start was called. I see lpfc doesn't have one; is it possible to restructure it to have one? (In any case, good job tracking this down; it was really annoying me.) Possibly we should be less forgiving, and require drivers to have a scan_start, otherwise they can't avoid this race. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch] plug async scan race at 1st node scan
On Mon, Aug 20, 2007 at 09:10:35AM -0400, James Smart wrote: > In testing 2.6.23-rc3, there is a small window where the async-per-target > scan of the transport can beat the call from the LLDD to scsi_scan_host(). I'd assumed that events wouldn't come in until ->scan_start was called. I see lpfc doesn't have one; is it possible to restructure it to have one? (In any case, good job tracking this down; it was really annoying me.) Possibly we should be less forgiving, and require drivers to have a scan_start, otherwise they can't avoid this race. -- "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch] plug async scan race at 1st node scan
In testing 2.6.23-rc3, there is a small window where the async-per-target
scan of the transport can beat the call from the LLDD to scsi_scan_host().
If so, the target scan starts, and can add the sdev (and the sysfslinks) before
the flags for async scan can prevent it. Thus, when async scan finishes and
asks for all sdevs to be enumerated -EEXIST errors will pop up.
This patch has scsi_alloc_host() look for the async scan hooks, and if they
exist, sets the async_scan flag to a pre-state, which still allows the async
target scan to continue, but stops the sysfs enumeration.
-- james s
This patch cut against 2.6.23-rc3 plus the following patch:
http://marc.info/?l=linux-scsi&m=118289275414202&w=2
PS: there really should be better hooks for knowing if the driver expects
async or background scanning (perhaps the whole pre-state should be set
by the driver).
Signed-off-by: James Smart <[EMAIL PROTECTED]>
diff -upNr a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c 2007-08-17 11:49:47.0 -0400
+++ b/drivers/scsi/hosts.c 2007-08-19 12:23:06.0 -0400
@@ -343,6 +343,12 @@ struct Scsi_Host *scsi_host_alloc(struct
shost->use_clustering = sht->use_clustering;
shost->ordered_tag = sht->ordered_tag;
+ if ((strncmp(scsi_scan_type, "async", 5) == 0) &&
+ (shost->hostt->scan_finished))
+ shost->async_scan = ASYNC_SCAN_PREASYNC;
+ else
+ shost->async_scan = ASYNC_SCAN_CMPLT;
+
if (sht->max_host_blocked)
shost->max_host_blocked = sht->max_host_blocked;
else
diff -upNr a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
--- a/drivers/scsi/scsi_priv.h 2007-07-08 19:32:17.0 -0400
+++ b/drivers/scsi/scsi_priv.h 2007-08-19 12:23:49.0 -0400
@@ -38,9 +38,6 @@ static inline void scsi_log_completion(s
{ };
#endif
-/* scsi_scan.c */
-int scsi_complete_async_scans(void);
-
/* scsi_devinfo.c */
extern int scsi_get_device_flags(struct scsi_device *sdev,
const unsigned char *vendor,
@@ -96,6 +93,8 @@ extern int scsi_scan_host_selected(struc
unsigned int, unsigned int, int);
extern void scsi_forget_host(struct Scsi_Host *);
extern void scsi_rescan_device(struct device *);
+extern char scsi_scan_type[];
+int scsi_complete_async_scans(void);
/* scsi_sysctl.c */
#ifdef CONFIG_SYSCTL
diff -upNr a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c 2007-08-19 12:21:10.0 -0400
+++ b/drivers/scsi/scsi_scan.c 2007-08-19 12:33:06.0 -0400
@@ -95,7 +95,7 @@ MODULE_PARM_DESC(max_luns,
#define SCSI_SCAN_TYPE_DEFAULT "sync"
#endif
-static char scsi_scan_type[6] = SCSI_SCAN_TYPE_DEFAULT;
+char scsi_scan_type[6] = SCSI_SCAN_TYPE_DEFAULT;
module_param_string(scan, scsi_scan_type, sizeof(scsi_scan_type), S_IRUGO);
MODULE_PARM_DESC(scan, "sync, async or none");
@@ -908,7 +908,7 @@ static int scsi_add_lun(struct scsi_devi
* register it and tell the rest of the kernel
* about it.
*/
- if (!async && scsi_sysfs_add_sdev(sdev) != 0)
+ if ((async == ASYNC_SCAN_CMPLT) && scsi_sysfs_add_sdev(sdev) != 0)
return SCSI_SCAN_NO_RESPONSE;
return SCSI_SCAN_LUN_PRESENT;
@@ -1472,7 +1472,7 @@ struct scsi_device *__scsi_add_device(st
return ERR_PTR(-ENOMEM);
mutex_lock(&shost->scan_mutex);
- if (!shost->async_scan)
+ if (shost->async_scan == ASYNC_SCAN_CMPLT)
scsi_complete_async_scans();
if (scsi_host_scan_allowed(shost))
@@ -1588,7 +1588,7 @@ void scsi_scan_target(struct device *par
return;
mutex_lock(&shost->scan_mutex);
- if (!shost->async_scan)
+ if (shost->async_scan == ASYNC_SCAN_CMPLT)
scsi_complete_async_scans();
if (scsi_host_scan_allowed(shost))
@@ -1641,7 +1641,7 @@ int scsi_scan_host_selected(struct Scsi_
return -EINVAL;
mutex_lock(&shost->scan_mutex);
- if (!shost->async_scan)
+ if (shost->async_scan == ASYNC_SCAN_CMPLT)
scsi_complete_async_scans();
if (scsi_host_scan_allowed(shost)) {
@@ -1686,7 +1686,7 @@ static struct async_scan_data *scsi_prep
if (strncmp(scsi_scan_type, "sync", 4) == 0)
return NULL;
- if (shost->async_scan) {
+ if (shost->async_scan == ASYNC_SCAN_RUNNING) {
printk("%s called twice for host %d", __FUNCTION__,
shost->host_no);
dump_stack();
@@ -1703,7 +1703,7 @@ static struct async_scan_data *scsi_prep
mutex_lock(&shost->scan_mutex);
spin_lock_irqsave(shost->host_lock, flags);
- shost->async_scan = 1;
+ shost->async_scan = ASYNC_SCAN_RUNNING;
spin_unlock_irqrestore(shost->host_lock, flags);
mutex_unlock(&shost->scan_mutex);
Re: [PATCH] add use_sg_chaining option to scsi_host_template
On Mon, Aug 20 2007, FUJITA Tomonori wrote: > On Mon, 20 Aug 2007 09:10:31 +0200 > Jens Axboe <[EMAIL PROTECTED]> wrote: > > > On Sat, Aug 18 2007, FUJITA Tomonori wrote: > > > On Fri, 17 Aug 2007 01:47:59 +0900 > > > FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > > > > > > > This is for Jens' sglist branch in the block git tree. > > > > > > > > It enables sg chaining support for the LLDs that use scsi_for_each_sg > > > > accessor properly. > > > > > > > > --- > > > > >From a6e50a3b476bc193de103e8c1d95877ced38918e Mon Sep 17 00:00:00 2001 > > > > From: FUJITA Tomonori <[EMAIL PROTECTED]> > > > > Date: Fri, 17 Aug 2007 01:35:41 +0900 > > > > Subject: [PATCH] add use_sg_chaining option to scsi_host_template > > > > > > > > This option is true if a low-level driver can support sg > > > > chaining. This will be removed eventually when all the drivers are > > > > converted to support sg chaining. q->max_phys_segments is set to > > > > SCSI_MAX_SG_SEGMENTS if false. > > > > > > > > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> > > > > --- > > > > arch/ia64/hp/sim/simscsi.c|1 + > > > > drivers/scsi/3w-9xxx.c|1 + > > > > drivers/scsi/3w-.c|1 + > > > > drivers/scsi/BusLogic.c |1 + > > > > drivers/scsi/NCR53c406a.c |3 ++- > > > > drivers/scsi/a100u2w.c|1 + > > > > drivers/scsi/aacraid/linit.c |1 + > > > > drivers/scsi/aha1740.c|1 + > > > > drivers/scsi/aic7xxx/aic79xx_osm.c|1 + > > > > drivers/scsi/aic7xxx/aic7xxx_osm.c|1 + > > > > drivers/scsi/aic7xxx_old.c|1 + > > > > drivers/scsi/arcmsr/arcmsr_hba.c |1 + > > > > drivers/scsi/dc395x.c |1 + > > > > drivers/scsi/dpt_i2o.c|1 + > > > > drivers/scsi/eata.c |3 ++- > > > > drivers/scsi/hosts.c |1 + > > > > drivers/scsi/hptiop.c |1 + > > > > drivers/scsi/ibmmca.c |1 + > > > > drivers/scsi/ibmvscsi/ibmvscsi.c |1 + > > > > drivers/scsi/initio.c |1 + > > > > drivers/scsi/ipr.c|1 + > > > > > > I should have dropped ipr since we haven't converted libata yet. > > > > But we have, are there still bits missing? > > I'm just a bit nervous about possible bugs. I hope there aren't any left in libata :-) > Probably we need to remove blk_queue_max_phys_segments() in libata > for a possible lower sg list size (< 128). Indeed, that should go, even in mainline now. > BTW, I think that it would be good to test the sglist with a lower sg > list size (like 32) to test all the llds. That's a good idea, then we'll get the chaining stuff exercised much more easily! -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Errors on USB-storage device are ignored.
On Mon, 2007-08-20 at 10:37 +0200, Rogier Wolff wrote: > On Sun, Aug 19, 2007 at 11:09:46PM -0700, Matthew Dharm wrote: > > Off the cuff, this really looks like something for the SCSI layer. I'll > > bet there are SCSI devices that do something similar... > > > > And, I'm generally reluctant to modify the data to/from a device unless > > usb-storage *really* has to in order to make it work > > I fully agree, but as the scsi layer has been in existance almost 10 > years longer than USB-storage, and that nobody seems to have encountered > a device requesting auto-sense, and then reporting no error, I would > say this is just a classical: "shitty USB implementation". I'm afraid it is. Reporting NO SENSE is actually a legitimate thing for a SCSI device to do. Usually it just means that the problem it was reporting went away in the interim. So, making it mean there's a serious drive problem would put us in volation of the spec. If this can be fixed, it will have to be in USB ... probably tied as specificially as possible to this particular device ... USB storage that correctly follows the RBC spec is allowed to report NO SENSE for the same reason as standard SCSI. James - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] FC transport: Disable LUN scanning from low level driver
The FC transport class calls scsi_scan_target with the SCAN_WILD_CARD
flag to automatically scan for logical units. zfcp, on the other hand,
only uses the units that are configured via the zfcp sysfs interface.
The main reason for this, is that the adapter behind zfcp supports
adapter sharing without NPIV: The adapter is logged into the SAN once,
and each unit can be used by one Linux system. If one Linux would grab
all LUNs, no other one can use them.
If a unit has the LUN 0, then the SCSI midlayer issue a REPORT LUNS,
checks the found LUN against the max_lun of the hostadapter. zfcp sets
this to 1 to only use its own managed units. If there is a LUN 0 that
is used by zfcp, then the SCSI midlayer produces lots of messages like
"scsi: host 0 channel 0 id 1 lun2 has a LUN larger than allowed by the
host adapter" to indicate the mismatch between the actual LUN and the
max_lun setting. These messages only confuse a user in the case of
zfcp, since there is no error.
To fix this problem, the LLD (zfcp) has to be able to prevent the
automatic scanning from the FC transport class. Attached is a patch
that adds a parameter to fc_remote_port_add(), another approach would
be an additional flag in the FC transport template.
What do others think? If there is an agreement, i will followup with a
new patch.
This can go away, as soon as zfcp does not have to support the
non-NPIV adapter sharing anymore, but this won't happen in the
foreseeable future.
Christof Schmitt
--- linux-2.6.orig/drivers/s390/scsi/zfcp_erp.c 2007-08-14 17:00:07.0
+0200
+++ linux-2.6/drivers/s390/scsi/zfcp_erp.c 2007-08-20 12:54:03.0
+0200
@@ -3175,7 +3175,7 @@ zfcp_erp_action_cleanup(int action, stru
ids.port_id = port->d_id;
ids.roles = FC_RPORT_ROLE_FCP_TARGET;
port->rport =
- fc_remote_port_add(adapter->scsi_host, 0, &ids);
+ fc_remote_port_add(adapter->scsi_host, 0, &ids,
0);
if (!port->rport)
ZFCP_LOG_NORMAL("failed registration of rport"
"(adapter %s, wwpn=0x%016Lx)\n",
--- linux-2.6.orig/drivers/scsi/scsi_transport_fc.c 2007-08-14
17:00:07.0 +0200
+++ linux-2.6/drivers/scsi/scsi_transport_fc.c 2007-08-20 12:56:15.0
+0200
@@ -2360,7 +2360,7 @@ fc_rport_final_delete(struct work_struct
**/
static struct fc_rport *
fc_rport_create(struct Scsi_Host *shost, int channel,
- struct fc_rport_identifiers *ids)
+ struct fc_rport_identifiers *ids, int scan_target)
{
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
struct fc_internal *fci = to_fc_internal(shost->transportt);
@@ -2424,6 +2424,9 @@ fc_rport_create(struct Scsi_Host *shost,
transport_add_device(dev);
transport_configure_device(dev);
+ if (scan_target)
+ rport->flags |= FC_RPORT_SCAN_TARGET;
+
if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
/* initiate a scan of the target */
rport->flags |= FC_RPORT_SCAN_PENDING;
@@ -2484,7 +2487,7 @@ delete_rport:
**/
struct fc_rport *
fc_remote_port_add(struct Scsi_Host *shost, int channel,
- struct fc_rport_identifiers *ids)
+ struct fc_rport_identifiers *ids, int scan_target)
{
struct fc_internal *fci = to_fc_internal(shost->transportt);
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
@@ -2574,6 +2577,10 @@ fc_remote_port_add(struct Scsi_Host *sho
spin_lock_irqsave(shost->host_lock, flags);
rport->flags &= ~FC_RPORT_DEVLOSS_PENDING;
+ if (scan_target)
+ rport->flags |= FC_RPORT_SCAN_TARGET;
+ else
+ rport->flags &= ~FC_RPORT_SCAN_TARGET;
/* if target, initiate a scan */
if (rport->scsi_target_id != -1) {
@@ -2657,7 +2664,7 @@ fc_remote_port_add(struct Scsi_Host *sho
spin_unlock_irqrestore(shost->host_lock, flags);
/* No consistent binding found - create new remote port entry */
- rport = fc_rport_create(shost, channel, ids);
+ rport = fc_rport_create(shost, channel, ids, scan_target);
return rport;
}
@@ -2991,7 +2998,8 @@ fc_scsi_scan_rport(struct work_struct *w
unsigned long flags;
if ((rport->port_state == FC_PORTSTATE_ONLINE) &&
- (rport->roles & FC_PORT_ROLE_FCP_TARGET)) {
+ (rport->roles & FC_PORT_ROLE_FCP_TARGET) &&
+ (rport->flags & FC_RPORT_SCAN_TARGET)) {
scsi_scan_target(&rport->dev, rport->channel,
rport->scsi_target_id, SCAN_WILD_CARD, 1);
}
--- linux-2.6.orig/include/scsi/scsi_transport_fc.h 2007-
Re: [PATCH] add use_sg_chaining option to scsi_host_template
On Mon, 20 Aug 2007 09:10:31 +0200 Jens Axboe <[EMAIL PROTECTED]> wrote: > On Sat, Aug 18 2007, FUJITA Tomonori wrote: > > On Fri, 17 Aug 2007 01:47:59 +0900 > > FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > > > > > This is for Jens' sglist branch in the block git tree. > > > > > > It enables sg chaining support for the LLDs that use scsi_for_each_sg > > > accessor properly. > > > > > > --- > > > >From a6e50a3b476bc193de103e8c1d95877ced38918e Mon Sep 17 00:00:00 2001 > > > From: FUJITA Tomonori <[EMAIL PROTECTED]> > > > Date: Fri, 17 Aug 2007 01:35:41 +0900 > > > Subject: [PATCH] add use_sg_chaining option to scsi_host_template > > > > > > This option is true if a low-level driver can support sg > > > chaining. This will be removed eventually when all the drivers are > > > converted to support sg chaining. q->max_phys_segments is set to > > > SCSI_MAX_SG_SEGMENTS if false. > > > > > > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> > > > --- > > > arch/ia64/hp/sim/simscsi.c|1 + > > > drivers/scsi/3w-9xxx.c|1 + > > > drivers/scsi/3w-.c|1 + > > > drivers/scsi/BusLogic.c |1 + > > > drivers/scsi/NCR53c406a.c |3 ++- > > > drivers/scsi/a100u2w.c|1 + > > > drivers/scsi/aacraid/linit.c |1 + > > > drivers/scsi/aha1740.c|1 + > > > drivers/scsi/aic7xxx/aic79xx_osm.c|1 + > > > drivers/scsi/aic7xxx/aic7xxx_osm.c|1 + > > > drivers/scsi/aic7xxx_old.c|1 + > > > drivers/scsi/arcmsr/arcmsr_hba.c |1 + > > > drivers/scsi/dc395x.c |1 + > > > drivers/scsi/dpt_i2o.c|1 + > > > drivers/scsi/eata.c |3 ++- > > > drivers/scsi/hosts.c |1 + > > > drivers/scsi/hptiop.c |1 + > > > drivers/scsi/ibmmca.c |1 + > > > drivers/scsi/ibmvscsi/ibmvscsi.c |1 + > > > drivers/scsi/initio.c |1 + > > > drivers/scsi/ipr.c|1 + > > > > I should have dropped ipr since we haven't converted libata yet. > > But we have, are there still bits missing? I'm just a bit nervous about possible bugs. Probably we need to remove blk_queue_max_phys_segments() in libata for a possible lower sg list size (< 128). BTW, I think that it would be good to test the sglist with a lower sg list size (like 32) to test all the llds. diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index d23a181..01a0197 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -795,8 +795,6 @@ int ata_scsi_slave_config(struct scsi_device *sdev) ata_scsi_sdev_config(sdev); - blk_queue_max_phys_segments(sdev->request_queue, LIBATA_MAX_PRD); - sdev->manage_start_stop = 1; if (dev) - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Errors on USB-storage device are ignored.
On Sun, Aug 19, 2007 at 11:09:46PM -0700, Matthew Dharm wrote:
> Off the cuff, this really looks like something for the SCSI layer. I'll
> bet there are SCSI devices that do something similar...
>
> And, I'm generally reluctant to modify the data to/from a device unless
> usb-storage *really* has to in order to make it work
I fully agree, but as the scsi layer has been in existance almost 10
years longer than USB-storage, and that nobody seems to have encountered
a device requesting auto-sense, and then reporting no error, I would
say this is just a classical: "shitty USB implementation".
The reverse situation is also probelematic. If a device happens to
occasionally request an autosense, but perhaps the problem solves
itself before the sense request comes through, then it might report
"no error". If that is actually true, for some SCSI harddisks, then
changing this at the scsi layer will make those harddrives report an
IO error every now and then, which is spurious.
That is why I chose to implement it at the lowest possible level,
as close as possible to the known non-conformant device.
Yes, I've generalized to: "Maybe there are other USB-IDE converters
that may make this mistake", but won't go as far as: "Maybe there
are other SCSI devices that make this mistake".
Roger.
>
> Matt
>
> On Thu, Aug 16, 2007 at 04:58:17PM +0200, Rogier Wolff wrote:
> >
> > Hi,
> >
> > I have an usb-storage enclosure that houses a normal desktop
> > harddrive. I have been wondering why disks in that enclosure
> > seemed to be having less errors than when connected to a
> > normal IDE connector.
> >
> > The reason is: USB-storage is ignoring a hint that something is
> > wrong. Probably my enclosure is also not completely following specs,
> > but Linux is ignoring the hint as well
> >
> > On hitting a bad block, the disk reports error. The USB converter
> > then reports "auto-sense-required", and this is carried out. However
> > at this point, my USB enclosure returns all-zeroes. This is
> > considered non-fatal by the kernel.
> >
> > I'm guessing not many people are testing these things with
> > bad drives. So, I don't know wether or not other USB converters
> > handle this situation more gracefully.
> >
> > As a patch, I've decided to set the sense key to "vendor specific"
> > (9), and then no "additional sense" (0:0), if, and only if the device
> > didn't return any valid sense info.
> >
> > The rest of the kernel then correctly interprets the situation
> > as an IO error.
> >
> > Roger Wolff.
> >
> > --- linux-2.6.20.3.clean/drivers/usb/storage/transport.c2007-03-13
> > 19:27:08.0 +0100
> > +++ linux-2.6.20.3.kostunrix/drivers/usb/storage/transport.c
> > 2007-08-16 16:47:00.0 +0200
> > @@ -629,6 +629,14 @@
> >
> > /* let's clean up right away */
> > memcpy(srb->sense_buffer, us->sensebuf, US_SENSE_SIZE);
> > + if (((srb->sense_buffer[2]&0xf) == 0) &&
> > + (srb->sense_buffer[12] == 0) &&
> > + (srb->sense_buffer[13] == 0)) {
> > + /* Hm. The device requested sense, but then
> > + declined to give us more info -- REW */
> > + srb->sense_buffer[2] |= 0x09; /* Vendor specific */
> > + }
> > +
> > srb->resid = old_resid;
> > srb->request_buffer = old_request_buffer;
> > srb->request_bufflen = old_request_bufflen;
> >
> >
> >
> > --
> > ** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
> > **Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233**
> > *-- BitWizard writes Linux device drivers for any device you may have! --*
> > Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement.
> > Does it sit on the couch all day? Is it unemployed? Please be specific!
> > Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ
>
> --
> Matthew Dharm Home: [EMAIL PROTECTED]
> Maintainer, Linux USB Mass Storage Driver
>
> G: Money isn't everything, A.J.
> AJ: Who convinced you of that?
> G: The Chief, at my last salary review.
> -- Mike and Greg
> User Friendly, 11/3/1998
--
** [EMAIL PROTECTED] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233**
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement.
Does it sit on the couch all day? Is it unemployed? Please be specific!
Define 'it' and what it isn't doing. - Adapted from lxrbot FAQ
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add use_sg_chaining option to scsi_host_template
On Fri, Aug 17 2007, FUJITA Tomonori wrote: > This is for Jens' sglist branch in the block git tree. > > It enables sg chaining support for the LLDs that use scsi_for_each_sg > accessor properly. > > --- > From a6e50a3b476bc193de103e8c1d95877ced38918e Mon Sep 17 00:00:00 2001 > From: FUJITA Tomonori <[EMAIL PROTECTED]> > Date: Fri, 17 Aug 2007 01:35:41 +0900 > Subject: [PATCH] add use_sg_chaining option to scsi_host_template > > This option is true if a low-level driver can support sg > chaining. This will be removed eventually when all the drivers are > converted to support sg chaining. q->max_phys_segments is set to > SCSI_MAX_SG_SEGMENTS if false. This looks good to me. I'd normally complain about the silly ENABLE_SG_CHAINING defines (just use 0/1), but since this is a short term approach, it's fine with me. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add use_sg_chaining option to scsi_host_template
On Sat, Aug 18 2007, FUJITA Tomonori wrote: > On Fri, 17 Aug 2007 01:47:59 +0900 > FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > > > This is for Jens' sglist branch in the block git tree. > > > > It enables sg chaining support for the LLDs that use scsi_for_each_sg > > accessor properly. > > > > --- > > >From a6e50a3b476bc193de103e8c1d95877ced38918e Mon Sep 17 00:00:00 2001 > > From: FUJITA Tomonori <[EMAIL PROTECTED]> > > Date: Fri, 17 Aug 2007 01:35:41 +0900 > > Subject: [PATCH] add use_sg_chaining option to scsi_host_template > > > > This option is true if a low-level driver can support sg > > chaining. This will be removed eventually when all the drivers are > > converted to support sg chaining. q->max_phys_segments is set to > > SCSI_MAX_SG_SEGMENTS if false. > > > > Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> > > --- > > arch/ia64/hp/sim/simscsi.c|1 + > > drivers/scsi/3w-9xxx.c|1 + > > drivers/scsi/3w-.c|1 + > > drivers/scsi/BusLogic.c |1 + > > drivers/scsi/NCR53c406a.c |3 ++- > > drivers/scsi/a100u2w.c|1 + > > drivers/scsi/aacraid/linit.c |1 + > > drivers/scsi/aha1740.c|1 + > > drivers/scsi/aic7xxx/aic79xx_osm.c|1 + > > drivers/scsi/aic7xxx/aic7xxx_osm.c|1 + > > drivers/scsi/aic7xxx_old.c|1 + > > drivers/scsi/arcmsr/arcmsr_hba.c |1 + > > drivers/scsi/dc395x.c |1 + > > drivers/scsi/dpt_i2o.c|1 + > > drivers/scsi/eata.c |3 ++- > > drivers/scsi/hosts.c |1 + > > drivers/scsi/hptiop.c |1 + > > drivers/scsi/ibmmca.c |1 + > > drivers/scsi/ibmvscsi/ibmvscsi.c |1 + > > drivers/scsi/initio.c |1 + > > drivers/scsi/ipr.c|1 + > > I should have dropped ipr since we haven't converted libata yet. But we have, are there still bits missing? -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata sg chaining support fix
On Sat, Aug 18 2007, FUJITA Tomonori wrote:
> This fixes 'trailing data' bug in the sglist libata patch in Jens'
> block git tree.
>
> Aug 18 16:03:03 tulip kernel: ata1.00: 36 bytes trailing data
> Aug 18 16:03:03 tulip kernel: scsi scan: INQUIRY result too short (5), using
> 36
>
>
> With the patch, sglist could be sent to -mm (I guess that Andrew hit
> this bug). But I'm still not sure about the sglist libata patch
> (especially about the padding path). It would be better to drop the
> sglist libata patch with my use_sg_chaining option patch and send
> sglist to -mm, I think.
>
> http://marc.info/?l=linux-scsi&m=118728307617958&w=2
>
> I guess that it would be better to leave sglist libata conversion in
> the maintainers' hands.
>
> ---
> From 86d7a796417bde84fc3cbe6ac6ebaaa524266092 Mon Sep 17 00:00:00 2001
> From: FUJITA Tomonori <[EMAIL PROTECTED]>
> Date: Sat, 18 Aug 2007 18:27:36 +0900
> Subject: [PATCH] libata sg chaining support fix
>
> Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> ---
> drivers/ata/libata-core.c |7 ++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f37eecc..52f75c3 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4648,16 +4648,18 @@ static void __atapi_pio_bytes(struct ata_queued_cmd
> *qc, unsigned int bytes)
> {
> int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
> struct scatterlist *sg = qc->__sg;
> + struct scatterlist *lsg = sg_last(qc->__sg, qc->n_elem);
> struct ata_port *ap = qc->ap;
> struct page *page;
> unsigned char *buf;
> unsigned int offset, count;
> + int no_more_sg = 0;
>
> if (qc->curbytes + bytes >= qc->nbytes)
> ap->hsm_task_state = HSM_ST_LAST;
>
> next_sg:
> - if (unlikely(qc->cursg == sg_last(qc->__sg, qc->n_elem))) {
> + if (unlikely(no_more_sg)) {
> /*
>* The end of qc->sg is reached and the device expects
>* more data to transfer. In order not to overrun qc->sg
> @@ -4719,6 +4721,9 @@ next_sg:
> qc->cursg_ofs += count;
>
> if (qc->cursg_ofs == sg->length) {
> + if (qc->cursg == lsg)
> + no_more_sg = 1;
> +
> qc->cursg = sg_next(qc->cursg);
> qc->cursg_ofs = 0;
> }
Tomo, this looks promising! Andrew, I have updated #for-akpm with the
sglist bits so it's enabled for the next -mm in the hope that this fixes
the issue.
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
