Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On 04/06/2013 11:08 AM, James Bottomley wrote: On Fri, 2013-03-15 at 10:46 +0100, Hannes Reinecke wrote: SAM advertises the use of a Well-known LUN (W_LUN) for scanning. As this avoids exposing LUN 0 (which might be a valid LUN) for all initiators it is the preferred method for LUN scanning on some arrays. So we should be using W_LUN for scanning, too. If the W_LUN is not supported we'll fall back to use LUN 0. For broken W_LUN implementations a new blacklist flag 'BLIST_NO_WLUN' is added. Well, we could do this, but I don't really see the point. By the time we get into the report lun code, we've already probed LUN 0, so it's as goeod as any for a REPORT LUN scan. Did we? I thought I had avoided that and directly went for probing W_LUN _first_. Will be cross-checking. What worries me slightly about the W-LUN is that for the first time we'll be assuming a device supports a particular address method (Extended Logical Unit addressing) rather than treating LUNs as opaque handles we keep and pass back to the target. I appreciate you now have a blacklist for failures, but if we didn't use W-LUNs we wouldn't need that blacklist. So could you answer two questions clearly: 1. What does this buy us over the current LUN0 method? I don't see LUN0 might be a valid LUN being a convincing reason. LUN masking. Some HBAs / virtualised devices use LUN masking to forward LUNs to the virtual machines. So for LUN0 you have the choice of exposing it to every virtual machine, meaning you cannot assign a device to LUN0, or have LUN0 as a no-device LUN which then can be exposed to every virtual machine. At which point you run into hardware limitations, as not every storage array allow for the first option. And not every LUN masking implementation allows you to expose a single LUN to several virtual machines. So you might be screwed either way. This was the main reason why zfcp could not use the standard LUN scanning method like every other HBA LLDD and had to resort to manual LUN activation. 2. What devices have you actually tested this on? Netapp FAS, HP EVA, HP P2000 / MSA, EMC Clariion. But as mentioned, I'll be rechecking the patch. We should _not_ try to probe LUN0 first, but rather send REPORT_LUNS to the W_LUN directly. If it responds, good. If not, we'll fall back to LUN0. Cheers, Hannes -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [OT] LDD3 Query
Hi, On Sat, Apr 6, 2013 at 5:23 AM, Greg KH gre...@linuxfoundation.org wrote: On Fri, Apr 05, 2013 at 12:17:54PM +0530, Vijay Chauhan wrote: Hi, I am new to Linux Device Driver and reading LDD3. It mentions that the disadvantage of dynamic major number allocation is that you can’t create the device nodes in advance, because the major number assigned to your module will vary. That's extremely out of date, all distros now use udev (or mdev) and as such, dynamic major/minor are fine to use, and recommended. To solve this problem, it says that create a script to invoke insmod and after calling insmod, reads /proc/devices in order to create the special file(s). Not needed anymore, udev handles this for you. Hope this helps, Thanks Greg. I realizes it later :) Vijay greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On Sun, 2013-04-07 at 15:31 +0200, Hannes Reinecke wrote: On 04/06/2013 11:08 AM, James Bottomley wrote: On Fri, 2013-03-15 at 10:46 +0100, Hannes Reinecke wrote: SAM advertises the use of a Well-known LUN (W_LUN) for scanning. As this avoids exposing LUN 0 (which might be a valid LUN) for all initiators it is the preferred method for LUN scanning on some arrays. So we should be using W_LUN for scanning, too. If the W_LUN is not supported we'll fall back to use LUN 0. For broken W_LUN implementations a new blacklist flag 'BLIST_NO_WLUN' is added. Well, we could do this, but I don't really see the point. By the time we get into the report lun code, we've already probed LUN 0, so it's as goeod as any for a REPORT LUN scan. Did we? I thought I had avoided that and directly went for probing W_LUN _first_. Will be cross-checking. What worries me slightly about the W-LUN is that for the first time we'll be assuming a device supports a particular address method (Extended Logical Unit addressing) rather than treating LUNs as opaque handles we keep and pass back to the target. I appreciate you now have a blacklist for failures, but if we didn't use W-LUNs we wouldn't need that blacklist. So could you answer two questions clearly: 1. What does this buy us over the current LUN0 method? I don't see LUN0 might be a valid LUN being a convincing reason. LUN masking. Some HBAs / virtualised devices use LUN masking to forward LUNs to the virtual machines. So for LUN0 you have the choice of exposing it to every virtual machine, meaning you cannot assign a device to LUN0, or have LUN0 as a no-device LUN which then can be exposed to every virtual machine. That shouldn't matter, should it? The spec says that even a masked LUN must respond to an inquiry (with PQ indicating appropriate inaccessibility). At which point you run into hardware limitations, as not every storage array allow for the first option. And not every LUN masking implementation allows you to expose a single LUN to several virtual machines. So you might be screwed either way. This was the main reason why zfcp could not use the standard LUN scanning method like every other HBA LLDD and had to resort to manual LUN activation. So this is an out of spec implementation of LUN masking ... as in it doesn't respond correctly to an INQUIRY? 2. What devices have you actually tested this on? Netapp FAS, HP EVA, HP P2000 / MSA, EMC Clariion. But as mentioned, I'll be rechecking the patch. We should _not_ try to probe LUN0 first, but rather send REPORT_LUNS to the W_LUN directly. If it responds, good. If not, we'll fall back to LUN0. I don't think we can ever do that ... what about SCSI 2 devices that don't support REPORT LUNS or USB devices that will crash on it? We might be able to try a host type whitelist, where if we were a USB or traditional bus host (SPI) we never try this, but if we're a modern one (SAS, FC) we do. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 0/9] More device removal fixes
Fix a few issues that can be triggered by removing a device: - Fix a race between starved list processing and device removal. - Avoid that a SCSI LLD callback can get invoked after scsi_remove_host() finished. - Speed up device removal by stopping error handling as soon as the SHOST_DEL or SHOST_DEL_RECOVERY state has been reached. - Save and restore the host_scribble field during error handling. These patches have been tested on top of kernel v3.9-rc5. Changes compared to v9: - Changed one WARN_ON() statement into a WARN() statement. Changes compared to v8: - Addressed the feedback from Joe Lawrence - dropped the patch that makes scsi_remove_host() wait until the last sdev user is gone. - Eliminated Scsi_Host.tmf_in_progress since it duplicates state information available in Scsi_Host.eh_active. - Added a patch to avoid reenabling I/O after the transport layer became offline. Changes compared to v7: - Addressed the review comments posted by Hannes Reinecke and Rolf Eike Beer. - Modified patch Make scsi_remove_host() wait until error handling finished such that it is also safe for SCSI timeout values below the maximum LLD response time by modifying scsi_send_eh_cmnd() such that it does not invoke any LLD code after scsi_remove_host() started. - Added a patch to save and restore the host_scribble field. - Refined / clarified several patch descriptions. - Rebased and retested on top of kernel v3.8-rc6. Changes compared to v6: - Dropped the first six patches since Jens queued these for 3.8. - Added patch to avoid that __scsi_remove_device() is invoked twice. - Restore error recovery in the SHOST_CANCEL state. Changes compared to v5: - Avoid that block layer work can be scheduled on a dead queue. - Do not invoke any SCSI LLD callback after scsi_remove_host() finished. - Stop error handling as soon as scsi_remove_host() started. - Remove the unused function bsg_goose_queue(). - Avoid that scsi_device_set_state() triggers a race condition. Changes compared to v4: - Moved queue_flag_set(QUEUE_FLAG_DEAD, q) from blk_drain_queue() into blk_cleanup_queue(). - Declared the new __blk_run_queue_uncond() function inline. Checked in the generated assembler code that this function is really inlined in __blk_run_queue(). - Elaborated several patch descriptions. - Added sparse annotations to scsi_request_fn(). - Split several patches. Changes compared to v3: - Fixed a race condition by setting QUEUE_FLAG_DEAD earlier. - Added a patch for fixing a race between starved list processing and device removal to this series. Changes compared to v2: - Split second patch into two patches. - Refined patch descriptions. Changes compared to v1: - Included a patch to rename QUEUE_FLAG_DEAD. - Refined the descriptions of the __blk_run_queue_uncond() and blk_cleanup_queue() functions. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 1/9] Fix race between starved list processing and device removal
scsi_run_queue() examines all SCSI devices that are present on
the starved list. Since scsi_run_queue() unlocks the SCSI host
lock before running a queue a SCSI device can get removed after
it has been removed from the starved list and before its queue
is run. Protect against that race condition by increasing the
sdev refcount before running the sdev queue. Also, remove a
SCSI device from the starved list before __scsi_remove_device()
decreases the sdev refcount such that the get_device() call
added in scsi_run_queue() is guaranteed to succeed.
Signed-off-by: Bart Van Assche bvanass...@acm.org
Reported-and-tested-by: Chanho Min chanho@lge.com
Reference: http://lkml.org/lkml/2012/8/2/96
Acked-by: Tejun Heo t...@kernel.org
Reviewed-by: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: sta...@vger.kernel.org
---
drivers/scsi/scsi_lib.c | 16 +++-
drivers/scsi/scsi_sysfs.c | 14 +-
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c31187d..8bf0f7e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -457,11 +457,17 @@ static void scsi_run_queue(struct request_queue *q)
continue;
}
- spin_unlock(shost-host_lock);
- spin_lock(sdev-request_queue-queue_lock);
- __blk_run_queue(sdev-request_queue);
- spin_unlock(sdev-request_queue-queue_lock);
- spin_lock(shost-host_lock);
+ /*
+* Obtain a reference before unlocking the host_lock such that
+* the sdev can't disappear before blk_run_queue() is invoked.
+*/
+ get_device(sdev-sdev_gendev);
+ spin_unlock_irqrestore(shost-host_lock, flags);
+
+ blk_run_queue(sdev-request_queue);
+ put_device(sdev-sdev_gendev);
+
+ spin_lock_irqsave(shost-host_lock, flags);
}
/* put any unprocessed entries back */
list_splice(starved_list, shost-starved_list);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 931a7d9..34f1b39 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -345,7 +345,6 @@ static void scsi_device_dev_release_usercontext(struct
work_struct *work)
starget-reap_ref++;
list_del(sdev-siblings);
list_del(sdev-same_target_siblings);
- list_del(sdev-starved_entry);
spin_unlock_irqrestore(sdev-host-host_lock, flags);
cancel_work_sync(sdev-event_work);
@@ -953,6 +952,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
void __scsi_remove_device(struct scsi_device *sdev)
{
struct device *dev = sdev-sdev_gendev;
+ struct Scsi_Host *shost = sdev-host;
+ unsigned long flags;
if (sdev-is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
@@ -974,6 +975,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
blk_cleanup_queue(sdev-request_queue);
cancel_work_sync(sdev-requeue_work);
+ /*
+* Remove a SCSI device from the starved list after blk_cleanup_queue()
+* finished such that scsi_request_fn() can't add it back to that list.
+* Also remove an sdev from the starved list before invoking
+* put_device() such that get_device() is guaranteed to succeed for an
+* sdev present on the starved list.
+*/
+ spin_lock_irqsave(shost-host_lock, flags);
+ list_del(sdev-starved_entry);
+ spin_unlock_irqrestore(shost-host_lock, flags);
+
if (sdev-host-hostt-slave_destroy)
sdev-host-hostt-slave_destroy(sdev);
transport_destroy_device(dev);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 2/9] Remove get_device() / put_device() pair from scsi_request_fn()
Now that all scsi_request_fn() callers hold a reference on the
SCSI device that function is invoked for and since
blk_cleanup_queue() waits until scsi_request_fn() has finished
it is safe to remove the get_device() / put_device() pair from
scsi_request_fn().
Signed-off-by: Bart Van Assche bvanass...@acm.org
Acked-by: Tejun Heo t...@kernel.org
Cc: James Bottomley jbottom...@parallels.com
Cc: Hannes Reinecke h...@suse.de
Cc: Mike Christie micha...@cs.wisc.edu
---
drivers/scsi/scsi_lib.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8bf0f7e..d84cbd0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1534,16 +1534,14 @@ static void scsi_softirq_done(struct request *rq)
* Lock status: IO request lock assumed to be held when called.
*/
static void scsi_request_fn(struct request_queue *q)
+ __releases(q-queue_lock)
+ __acquires(q-queue_lock)
{
struct scsi_device *sdev = q-queuedata;
struct Scsi_Host *shost;
struct scsi_cmnd *cmd;
struct request *req;
- if(!get_device(sdev-sdev_gendev))
- /* We must be tearing the block queue down already */
- return;
-
/*
* To start with, we keep looping until the queue is empty, or until
* the host is no longer able to accept any more requests.
@@ -1632,7 +1630,7 @@ static void scsi_request_fn(struct request_queue *q)
goto out_delay;
}
- goto out;
+ return;
not_ready:
spin_unlock_irq(shost-host_lock);
@@ -1651,12 +1649,6 @@ static void scsi_request_fn(struct request_queue *q)
out_delay:
if (sdev-device_busy == 0)
blk_delay_queue(q, SCSI_QUEUE_DELAY);
-out:
- /* must be careful here...if we trigger the -remove() function
-* we cannot be holding the q lock */
- spin_unlock_irq(q-queue_lock);
- put_device(sdev-sdev_gendev);
- spin_lock_irq(q-queue_lock);
}
u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 3/9] Avoid calling __scsi_remove_device() twice
SCSI devices are added to the shost-__devices list from inside
scsi_alloc_sdev(). If something goes wrong during LUN scanning,
e.g. a transport layer failure occurs, then __scsi_remove_device()
can get invoked by the LUN scanning code for a SCSI device in
state SDEV_CREATED_BLOCK or SDEV_BLOCKED. If this happens then
the SCSI device has not yet been added to sysfs (is_visible == 0).
Make sure that if this happens these devices are transitioned
into state SDEV_DEL. This avoids that __scsi_remove_device()
gets invoked a second time by scsi_forget_host().
Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: James Bottomley jbottom...@parallels.com
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: Tejun Heo t...@kernel.org
---
drivers/scsi/scsi_lib.c |2 ++
drivers/scsi/scsi_sysfs.c |7 ---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d84cbd0..34cdcbc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2176,6 +2176,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum
scsi_device_state state)
case SDEV_OFFLINE:
case SDEV_TRANSPORT_OFFLINE:
case SDEV_CANCEL:
+ case SDEV_BLOCK:
+ case SDEV_CREATED_BLOCK:
break;
default:
goto illegal;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 34f1b39..f869ef85 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -956,8 +956,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
unsigned long flags;
if (sdev-is_visible) {
- if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
- return;
+ WARN(scsi_device_set_state(sdev, SDEV_CANCEL) != 0,
+%s: unexpected state %d\n, dev_name(sdev-sdev_gendev),
+sdev-sdev_state);
bsg_unregister_queue(sdev-request_queue);
device_unregister(sdev-sdev_dev);
@@ -971,7 +972,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
* scsi_run_queue() invocations have finished before tearing down the
* device.
*/
- scsi_device_set_state(sdev, SDEV_DEL);
+ WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_DEL) != 0);
blk_cleanup_queue(sdev-request_queue);
cancel_work_sync(sdev-requeue_work);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 4/9] Disallow changing the device state via sysfs into deleted
Changing the state of a SCSI device via sysfs into cancel or
deleted prevents removal of these devices by scsi_remove_host().
Hence do not allow this. Also, introduce the symbolic name
INVALID_SDEV_STATE, representing a value different from any valid
SCSI device state. Update scsi_device_set_state() such that gcc
does not issue a warning about an enumeration value not being
handled inside a switch statement.
Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Tejun Heo t...@kernel.org
Cc: James Bottomley jbottom...@parallels.com
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: David Milburn dmilb...@redhat.com
---
drivers/scsi/scsi_lib.c|2 ++
drivers/scsi/scsi_sysfs.c |5 +++--
include/scsi/scsi_device.h |6 +-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 34cdcbc..67d09a7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2184,6 +2184,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum
scsi_device_state state)
}
break;
+ case INVALID_SDEV_STATE:
+ goto illegal;
}
sdev-sdev_state = state;
return 0;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index f869ef85..fe3ea686 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -594,7 +594,7 @@ store_state_field(struct device *dev, struct
device_attribute *attr,
{
int i;
struct scsi_device *sdev = to_scsi_device(dev);
- enum scsi_device_state state = 0;
+ enum scsi_device_state state = INVALID_SDEV_STATE;
for (i = 0; i ARRAY_SIZE(sdev_states); i++) {
const int len = strlen(sdev_states[i].name);
@@ -604,7 +604,8 @@ store_state_field(struct device *dev, struct
device_attribute *attr,
break;
}
}
- if (!state)
+ if (state == INVALID_SDEV_STATE || state == SDEV_CANCEL ||
+ state == SDEV_DEL)
return -EINVAL;
if (scsi_device_set_state(sdev, state))
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a7f9cba..8539ce6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -29,7 +29,11 @@ struct scsi_mode_data {
* scsi_lib:scsi_device_set_state().
*/
enum scsi_device_state {
- SDEV_CREATED = 1, /* device created but not added to sysfs
+ INVALID_SDEV_STATE, /* Not a valid SCSI device state but a
+* symbolic name that can be used wherever
+* a value is needed that is different from
+* any valid SCSI device state. */
+ SDEV_CREATED, /* device created but not added to sysfs
* Only internal commands allowed (for inq) */
SDEV_RUNNING, /* device properly configured
* All commands allowed */
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 5/9] Avoid saving/restoring interrupt state inside scsi_remove_host()
Since it is not allowed to invoke scsi_remove_host() with interrupts
disabled, avoid saving and restoring the interrupt state inside
scsi_remove_host(). This patch does not change the functionality of
the function scsi_remove_host().
Signed-off-by: Bart Van Assche bvanass...@acm.org
Acked-by: Tejun Heo t...@kernel.org
Acked-by: Hannes Reinecke h...@suse.de
Cc: Mike Christie micha...@cs.wisc.edu
Cc: James Bottomley jbottom...@parallels.com
---
drivers/scsi/hosts.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index df0c3c7..034a567 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -156,27 +156,25 @@ EXPORT_SYMBOL(scsi_host_set_state);
**/
void scsi_remove_host(struct Scsi_Host *shost)
{
- unsigned long flags;
-
mutex_lock(shost-scan_mutex);
- spin_lock_irqsave(shost-host_lock, flags);
+ spin_lock_irq(shost-host_lock);
if (scsi_host_set_state(shost, SHOST_CANCEL))
if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
- spin_unlock_irqrestore(shost-host_lock, flags);
+ spin_unlock_irq(shost-host_lock);
mutex_unlock(shost-scan_mutex);
return;
}
- spin_unlock_irqrestore(shost-host_lock, flags);
+ spin_unlock_irq(shost-host_lock);
scsi_autopm_get_host(shost);
scsi_forget_host(shost);
mutex_unlock(shost-scan_mutex);
scsi_proc_host_rm(shost);
- spin_lock_irqsave(shost-host_lock, flags);
+ spin_lock_irq(shost-host_lock);
if (scsi_host_set_state(shost, SHOST_DEL))
BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
- spin_unlock_irqrestore(shost-host_lock, flags);
+ spin_unlock_irq(shost-host_lock);
transport_unregister_device(shost-shost_gendev);
device_unregister(shost-shost_dev);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 6/9] Make scsi_remove_host() wait until error handling finished
A SCSI LLD may start cleaning up host resources as soon as
scsi_remove_host() returns. These host resources may be needed by
the LLD in an implementation of one of the eh_* functions. So if
one of the eh_* functions is in progress when scsi_remove_host()
is invoked, wait until the eh_* function has finished. Also, do
not invoke any of the eh_* functions after scsi_remove_host() has
started. Remove Scsi_Host.tmf_in_progress because it is now
superfluous.
Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Hannes Reinecke h...@suse.de
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Tejun Heo t...@kernel.org
---
drivers/scsi/hosts.c |6
drivers/scsi/scsi_error.c | 86 ++---
include/scsi/scsi_host.h |6 ++--
3 files changed, 81 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 034a567..17e2ccb 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -176,6 +176,12 @@ void scsi_remove_host(struct Scsi_Host *shost)
BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
spin_unlock_irq(shost-host_lock);
+ /*
+* Wait until the error handler has finished invoking LLD callbacks
+* before allowing the LLD to proceed.
+*/
+ wait_event(shost-host_wait, shost-eh_active == 0);
+
transport_unregister_device(shost-shost_gendev);
device_unregister(shost-shost_dev);
device_del(shost-shost_gendev);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..b739afe 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -536,8 +536,53 @@ static void scsi_eh_done(struct scsi_cmnd *scmd)
}
/**
+ * scsi_begin_eh - start host-related error handling
+ *
+ * Must be called before invoking an LLD callback function to avoid that
+ * scsi_remove_host() returns while one of these callback functions is in
+ * progress.
+ *
+ * Returns 0 if invoking an eh_* function is allowed and a negative value if
+ * not. If this function returns 0 then scsi_end_eh() must be called
+ * eventually.
+ */
+static int scsi_begin_eh(struct Scsi_Host *host)
+{
+ int res;
+
+ spin_lock_irq(host-host_lock);
+ switch (host-shost_state) {
+ case SHOST_DEL:
+ case SHOST_DEL_RECOVERY:
+ res = -ENODEV;
+ break;
+ default:
+ WARN_ON_ONCE(host-eh_active 0);
+ host-eh_active++;
+ res = 0;
+ break;
+ }
+ spin_unlock_irq(host-host_lock);
+
+ return res;
+}
+
+/**
+ * scsi_end_eh - finish host-related error handling
+ */
+static void scsi_end_eh(struct Scsi_Host *host)
+{
+ spin_lock_irq(host-host_lock);
+ host-eh_active--;
+ WARN_ON_ONCE(host-eh_active 0);
+ if (host-eh_active == 0)
+ wake_up(host-host_wait);
+ spin_unlock_irq(host-host_lock);
+}
+
+/**
* scsi_try_host_reset - ask host adapter to reset itself
- * @scmd: SCSI cmd to send hsot reset.
+ * @scmd: SCSI cmd to send host reset.
*/
static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
@@ -552,6 +597,9 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
if (!hostt-eh_host_reset_handler)
return FAILED;
+ if (scsi_begin_eh(host))
+ return FAST_IO_FAIL;
+
rtn = hostt-eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
@@ -561,6 +609,7 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
scsi_report_bus_reset(host, scmd_channel(scmd));
spin_unlock_irqrestore(host-host_lock, flags);
}
+ scsi_end_eh(host);
return rtn;
}
@@ -582,6 +631,9 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
if (!hostt-eh_bus_reset_handler)
return FAILED;
+ if (scsi_begin_eh(host))
+ return FAST_IO_FAIL;
+
rtn = hostt-eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
@@ -591,6 +643,7 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
scsi_report_bus_reset(host, scmd_channel(scmd));
spin_unlock_irqrestore(host-host_lock, flags);
}
+ scsi_end_eh(host);
return rtn;
}
@@ -621,6 +674,9 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
if (!hostt-eh_target_reset_handler)
return FAILED;
+ if (scsi_begin_eh(host))
+ return FAST_IO_FAIL;
+
rtn = hostt-eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
spin_lock_irqsave(host-host_lock, flags);
@@ -628,6 +684,7 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
__scsi_report_device_reset);
spin_unlock_irqrestore(host-host_lock, flags);
}
+ scsi_end_eh(host);
return rtn;
}
@@ -645,14 +702,20 @@ static int
[PATCH v10 7/9] Avoid that scsi_device_set_state() triggers a race
Make concurrent invocations of scsi_device_set_state() safe.
Signed-off-by: Bart Van Assche bvanass...@acm.org
Acked-by: Hannes Reinecke h...@suse.de
Cc: James Bottomley jbottom...@parallels.com
Cc: Tejun Heo t...@kernel.org
Cc: Mike Christie micha...@cs.wisc.edu
---
drivers/scsi/scsi_error.c |4
drivers/scsi/scsi_lib.c | 43 ++-
drivers/scsi/scsi_scan.c | 15 ---
drivers/scsi/scsi_sysfs.c | 18 ++
4 files changed, 60 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index b739afe..a1b7eff 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1431,7 +1431,11 @@ static void scsi_eh_offline_sdevs(struct list_head
*work_q,
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
sdev_printk(KERN_INFO, scmd-device, Device offlined -
not ready after error recovery\n);
+
+ spin_lock_irq(scmd-device-host-host_lock);
scsi_device_set_state(scmd-device, SDEV_OFFLINE);
+ spin_unlock_irq(scmd-device-host-host_lock);
+
if (scmd-eh_eflags SCSI_EH_CANCEL_CMD) {
/*
* FIXME: Handle lost cmds.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 67d09a7..c8698a6 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2079,7 +2079,9 @@ EXPORT_SYMBOL(scsi_test_unit_ready);
* @state: state to change to.
*
* Returns zero if unsuccessful or an error if the requested
- * transition is illegal.
+ * transition is illegal. It is the responsibility of the caller to make
+ * sure that a call of this function does not race with other code that
+ * accesses the device state, e.g. by holding the host lock.
*/
int
scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
@@ -2360,7 +2362,13 @@ EXPORT_SYMBOL_GPL(sdev_evt_send_simple);
int
scsi_device_quiesce(struct scsi_device *sdev)
{
- int err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+ struct Scsi_Host *host = sdev-host;
+ int err;
+
+ spin_lock_irq(host-host_lock);
+ err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+ spin_unlock_irq(host-host_lock);
+
if (err)
return err;
@@ -2384,13 +2392,21 @@ EXPORT_SYMBOL(scsi_device_quiesce);
*/
void scsi_device_resume(struct scsi_device *sdev)
{
+ struct Scsi_Host *host = sdev-host;
+ int err;
+
/* check if the device state was mutated prior to resume, and if
* so assume the state is being managed elsewhere (for example
* device deleted during suspend)
*/
- if (sdev-sdev_state != SDEV_QUIESCE ||
- scsi_device_set_state(sdev, SDEV_RUNNING))
+ spin_lock_irq(host-host_lock);
+ err = sdev-sdev_state == SDEV_QUIESCE ?
+ scsi_device_set_state(sdev, SDEV_RUNNING) : -EINVAL;
+ spin_unlock_irq(host-host_lock);
+
+ if (err)
return;
+
scsi_run_queue(sdev-request_queue);
}
EXPORT_SYMBOL(scsi_device_resume);
@@ -2440,17 +2456,19 @@ EXPORT_SYMBOL(scsi_target_resume);
int
scsi_internal_device_block(struct scsi_device *sdev)
{
+ struct Scsi_Host *host = sdev-host;
struct request_queue *q = sdev-request_queue;
unsigned long flags;
int err = 0;
+ spin_lock_irqsave(host-host_lock, flags);
err = scsi_device_set_state(sdev, SDEV_BLOCK);
- if (err) {
+ if (err)
err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
+ spin_unlock_irqrestore(host-host_lock, flags);
- if (err)
- return err;
- }
+ if (err)
+ return err;
/*
* The device has transitioned to SDEV_BLOCK. Stop the
@@ -2485,13 +2503,16 @@ int
scsi_internal_device_unblock(struct scsi_device *sdev,
enum scsi_device_state new_state)
{
+ struct Scsi_Host *host = sdev-host;
struct request_queue *q = sdev-request_queue;
unsigned long flags;
+ int ret = 0;
/*
* Try to transition the scsi device to SDEV_RUNNING or one of the
* offlined states and goose the device queue if successful.
*/
+ spin_lock_irqsave(host-host_lock, flags);
if ((sdev-sdev_state == SDEV_BLOCK) ||
(sdev-sdev_state == SDEV_TRANSPORT_OFFLINE))
sdev-sdev_state = new_state;
@@ -2503,7 +2524,11 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
sdev-sdev_state = SDEV_CREATED;
} else if (sdev-sdev_state != SDEV_CANCEL
sdev-sdev_state != SDEV_OFFLINE)
- return -EINVAL;
+ ret = -EINVAL;
+ spin_unlock_irqrestore(host-host_lock, flags);
+
+ if (ret)
+
[PATCH v10 8/9] Save and restore host_scribble during error handling
A SCSI LLD may overwrite host_scribble in its queuecommand()
implementation. Several drivers need that field to process
requests and aborts correctly. Hence this field must be saved
by scsi_eh_prep_cmnd() and must be restored by
scsi_eh_restore_cmnd().
Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: James Bottomley jbottom...@parallels.com
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: Tejun Heo t...@kernel.org
---
drivers/scsi/scsi_error.c |2 ++
include/scsi/scsi_eh.h|1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a1b7eff..33dcce3 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -770,6 +770,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct
scsi_eh_save *ses,
ses-result = scmd-result;
ses-underflow = scmd-underflow;
ses-prot_op = scmd-prot_op;
+ ses-host_scribble = scmd-host_scribble;
scmd-prot_op = SCSI_PROT_NORMAL;
scmd-cmnd = ses-eh_cmnd;
@@ -831,6 +832,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct
scsi_eh_save *ses)
scmd-result = ses-result;
scmd-underflow = ses-underflow;
scmd-prot_op = ses-prot_op;
+ scmd-host_scribble = ses-host_scribble;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 06a8790..7bdb02f 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -83,6 +83,7 @@ struct scsi_eh_save {
/* new command support */
unsigned char eh_cmnd[BLK_MAX_CDB];
struct scatterlist sense_sgl;
+ unsigned char *host_scribble;
};
extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 9/9] Avoid reenabling I/O after the transport became offline
Disallow the SDEV_TRANSPORT_OFFLINE to SDEV_CANCEL transition such
that no I/O is sent to devices for which the transport is offline.
Notes:
- Functions like sd_shutdown() use scsi_execute_req() and hence
set the REQ_PREEMPT flag. Such requests are passed to the LLD
queuecommand callback in the SDEV_CANCEL state.
- This patch does not affect Fibre Channel LLD drivers since these
drivers invoke fc_remote_port_chkready() before submitting a SCSI
request to the HBA. That prevents a timeout to occur in state
SDEV_CANCEL if the transport is offline.
Signed-off-by: Bart Van Assche bvanass...@acm.org
Reviewed-by: Mike Christie micha...@cs.wisc.edu
Cc: James Bottomley jbottom...@parallels.com
Cc: Hannes Reinecke h...@suse.de
Cc: Tejun Heo t...@kernel.org
---
drivers/scsi/scsi_lib.c |1 -
drivers/scsi/scsi_sysfs.c |3 ++-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c8698a6..c6aeafd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2163,7 +2163,6 @@ scsi_device_set_state(struct scsi_device *sdev, enum
scsi_device_state state)
case SDEV_RUNNING:
case SDEV_QUIESCE:
case SDEV_OFFLINE:
- case SDEV_TRANSPORT_OFFLINE:
case SDEV_BLOCK:
break;
default:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 11318cc..dd2005c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -963,7 +963,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
if (sdev-is_visible) {
spin_lock_irqsave(shost-host_lock, flags);
- WARN(scsi_device_set_state(sdev, SDEV_CANCEL) != 0,
+ WARN(scsi_device_set_state(sdev, SDEV_CANCEL) != 0
+sdev-sdev_state != SDEV_TRANSPORT_OFFLINE,
%s: unexpected state %d\n, dev_name(sdev-sdev_gendev),
sdev-sdev_state);
spin_unlock_irqrestore(shost-host_lock, flags);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On 13-04-07 10:49 AM, James Bottomley wrote: On Sun, 2013-04-07 at 15:31 +0200, Hannes Reinecke wrote: On 04/06/2013 11:08 AM, James Bottomley wrote: On Fri, 2013-03-15 at 10:46 +0100, Hannes Reinecke wrote: SAM advertises the use of a Well-known LUN (W_LUN) for scanning. As this avoids exposing LUN 0 (which might be a valid LUN) for all initiators it is the preferred method for LUN scanning on some arrays. So we should be using W_LUN for scanning, too. If the W_LUN is not supported we'll fall back to use LUN 0. For broken W_LUN implementations a new blacklist flag 'BLIST_NO_WLUN' is added. Well, we could do this, but I don't really see the point. By the time we get into the report lun code, we've already probed LUN 0, so it's as goeod as any for a REPORT LUN scan. Did we? I thought I had avoided that and directly went for probing W_LUN _first_. Will be cross-checking. What worries me slightly about the W-LUN is that for the first time we'll be assuming a device supports a particular address method (Extended Logical Unit addressing) rather than treating LUNs as opaque handles we keep and pass back to the target. I appreciate you now have a blacklist for failures, but if we didn't use W-LUNs we wouldn't need that blacklist. So could you answer two questions clearly: 1. What does this buy us over the current LUN0 method? I don't see LUN0 might be a valid LUN being a convincing reason. LUN masking. Some HBAs / virtualised devices use LUN masking to forward LUNs to the virtual machines. So for LUN0 you have the choice of exposing it to every virtual machine, meaning you cannot assign a device to LUN0, or have LUN0 as a no-device LUN which then can be exposed to every virtual machine. That shouldn't matter, should it? The spec says that even a masked LUN must respond to an inquiry (with PQ indicating appropriate inaccessibility). Which spec? I haven't seen a mention of LUN masking in any SCSI spec and I just rechecked SAM-2,3,4 and 5. Looked at FCP-4 as well. At which point you run into hardware limitations, as not every storage array allow for the first option. And not every LUN masking implementation allows you to expose a single LUN to several virtual machines. So you might be screwed either way. This was the main reason why zfcp could not use the standard LUN scanning method like every other HBA LLDD and had to resort to manual LUN activation. So this is an out of spec implementation of LUN masking ... as in it doesn't respond correctly to an INQUIRY? No specs apply that I can see. 2. What devices have you actually tested this on? Netapp FAS, HP EVA, HP P2000 / MSA, EMC Clariion. But as mentioned, I'll be rechecking the patch. We should _not_ try to probe LUN0 first, but rather send REPORT_LUNS to the W_LUN directly. If it responds, good. If not, we'll fall back to LUN0. I don't think we can ever do that ... what about SCSI 2 devices that don't support REPORT LUNS or USB devices that will crash on it? We might be able to try a host type whitelist, where if we were a USB or traditional bus host (SPI) we never try this, but if we're a modern one (SAS, FC) we do. The VERSION field (byte 2) of an INQUIRY response is always available, even on USB storage devices which usually claim SCSI-2 compliance: 2 == (rsp_buff[2] 0x7) No need to try REPORT LUNS on such devices. Are there any SCSI-1 devices still out there? Doug Gilbert -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On Sun, 2013-04-07 at 11:59 -0400, Douglas Gilbert wrote: On 13-04-07 10:49 AM, James Bottomley wrote: On Sun, 2013-04-07 at 15:31 +0200, Hannes Reinecke wrote: On 04/06/2013 11:08 AM, James Bottomley wrote: On Fri, 2013-03-15 at 10:46 +0100, Hannes Reinecke wrote: SAM advertises the use of a Well-known LUN (W_LUN) for scanning. As this avoids exposing LUN 0 (which might be a valid LUN) for all initiators it is the preferred method for LUN scanning on some arrays. So we should be using W_LUN for scanning, too. If the W_LUN is not supported we'll fall back to use LUN 0. For broken W_LUN implementations a new blacklist flag 'BLIST_NO_WLUN' is added. Well, we could do this, but I don't really see the point. By the time we get into the report lun code, we've already probed LUN 0, so it's as goeod as any for a REPORT LUN scan. Did we? I thought I had avoided that and directly went for probing W_LUN _first_. Will be cross-checking. What worries me slightly about the W-LUN is that for the first time we'll be assuming a device supports a particular address method (Extended Logical Unit addressing) rather than treating LUNs as opaque handles we keep and pass back to the target. I appreciate you now have a blacklist for failures, but if we didn't use W-LUNs we wouldn't need that blacklist. So could you answer two questions clearly: 1. What does this buy us over the current LUN0 method? I don't see LUN0 might be a valid LUN being a convincing reason. LUN masking. Some HBAs / virtualised devices use LUN masking to forward LUNs to the virtual machines. So for LUN0 you have the choice of exposing it to every virtual machine, meaning you cannot assign a device to LUN0, or have LUN0 as a no-device LUN which then can be exposed to every virtual machine. That shouldn't matter, should it? The spec says that even a masked LUN must respond to an inquiry (with PQ indicating appropriate inaccessibility). Which spec? I haven't seen a mention of LUN masking in any SCSI spec and I just rechecked SAM-2,3,4 and 5. Looked at FCP-4 as well. It's not called LUN Masking; it's called access control, although the ACLs are referred to as LUN masks. At which point you run into hardware limitations, as not every storage array allow for the first option. And not every LUN masking implementation allows you to expose a single LUN to several virtual machines. So you might be screwed either way. This was the main reason why zfcp could not use the standard LUN scanning method like every other HBA LLDD and had to resort to manual LUN activation. So this is an out of spec implementation of LUN masking ... as in it doesn't respond correctly to an INQUIRY? No specs apply that I can see. SPC-3 Section 8.3 Access Controls 2. What devices have you actually tested this on? Netapp FAS, HP EVA, HP P2000 / MSA, EMC Clariion. But as mentioned, I'll be rechecking the patch. We should _not_ try to probe LUN0 first, but rather send REPORT_LUNS to the W_LUN directly. If it responds, good. If not, we'll fall back to LUN0. I don't think we can ever do that ... what about SCSI 2 devices that don't support REPORT LUNS or USB devices that will crash on it? We might be able to try a host type whitelist, where if we were a USB or traditional bus host (SPI) we never try this, but if we're a modern one (SAS, FC) we do. The VERSION field (byte 2) of an INQUIRY response is always available, even on USB storage devices which usually claim SCSI-2 compliance: 2 == (rsp_buff[2] 0x7) So this is the chicken and egg problem: if we haven't probed the target at all, how do we know it's SCSI-2? If we do an initial probe, we have to do it as an INQUIRY to LUN0 and we end up in the same situation we are now. That's why I suggested a host bus type parameter if we want to do REPORT LUNS probes. No need to try REPORT LUNS on such devices. Are there any SCSI-1 devices still out there? I don't have any (last one burned out a while ago). James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On 13-04-07 12:15 PM, James Bottomley wrote: On Sun, 2013-04-07 at 11:59 -0400, Douglas Gilbert wrote: On 13-04-07 10:49 AM, James Bottomley wrote: On Sun, 2013-04-07 at 15:31 +0200, Hannes Reinecke wrote: On 04/06/2013 11:08 AM, James Bottomley wrote: On Fri, 2013-03-15 at 10:46 +0100, Hannes Reinecke wrote: SAM advertises the use of a Well-known LUN (W_LUN) for scanning. As this avoids exposing LUN 0 (which might be a valid LUN) for all initiators it is the preferred method for LUN scanning on some arrays. So we should be using W_LUN for scanning, too. If the W_LUN is not supported we'll fall back to use LUN 0. For broken W_LUN implementations a new blacklist flag 'BLIST_NO_WLUN' is added. Well, we could do this, but I don't really see the point. By the time we get into the report lun code, we've already probed LUN 0, so it's as goeod as any for a REPORT LUN scan. Did we? I thought I had avoided that and directly went for probing W_LUN _first_. Will be cross-checking. What worries me slightly about the W-LUN is that for the first time we'll be assuming a device supports a particular address method (Extended Logical Unit addressing) rather than treating LUNs as opaque handles we keep and pass back to the target. I appreciate you now have a blacklist for failures, but if we didn't use W-LUNs we wouldn't need that blacklist. So could you answer two questions clearly: 1. What does this buy us over the current LUN0 method? I don't see LUN0 might be a valid LUN being a convincing reason. LUN masking. Some HBAs / virtualised devices use LUN masking to forward LUNs to the virtual machines. So for LUN0 you have the choice of exposing it to every virtual machine, meaning you cannot assign a device to LUN0, or have LUN0 as a no-device LUN which then can be exposed to every virtual machine. That shouldn't matter, should it? The spec says that even a masked LUN must respond to an inquiry (with PQ indicating appropriate inaccessibility). Which spec? I haven't seen a mention of LUN masking in any SCSI spec and I just rechecked SAM-2,3,4 and 5. Looked at FCP-4 as well. It's not called LUN Masking; it's called access control, although the ACLs are referred to as LUN masks. At which point you run into hardware limitations, as not every storage array allow for the first option. And not every LUN masking implementation allows you to expose a single LUN to several virtual machines. So you might be screwed either way. This was the main reason why zfcp could not use the standard LUN scanning method like every other HBA LLDD and had to resort to manual LUN activation. So this is an out of spec implementation of LUN masking ... as in it doesn't respond correctly to an INQUIRY? No specs apply that I can see. SPC-3 Section 8.3 Access Controls spc4r36f.pdf section 8.3.1.2 [Overview] Access controls are handled in the SCSI target device by an access controls coordinator located at the ACCESS CONTROLS well known logical unit. The access controls coordinator also may be accessible via LUN 0. The access controls coordinator associates a specific LUN to a specific logical unit depending on which initiator port accesses the SCSI target device and whether the initiator port has access rights to the logical unit. That seems to strengthen the argument for going the W_LUN route. Doug Gilbert 2. What devices have you actually tested this on? Netapp FAS, HP EVA, HP P2000 / MSA, EMC Clariion. But as mentioned, I'll be rechecking the patch. We should _not_ try to probe LUN0 first, but rather send REPORT_LUNS to the W_LUN directly. If it responds, good. If not, we'll fall back to LUN0. I don't think we can ever do that ... what about SCSI 2 devices that don't support REPORT LUNS or USB devices that will crash on it? We might be able to try a host type whitelist, where if we were a USB or traditional bus host (SPI) we never try this, but if we're a modern one (SAS, FC) we do. The VERSION field (byte 2) of an INQUIRY response is always available, even on USB storage devices which usually claim SCSI-2 compliance: 2 == (rsp_buff[2] 0x7) So this is the chicken and egg problem: if we haven't probed the target at all, how do we know it's SCSI-2? If we do an initial probe, we have to do it as an INQUIRY to LUN0 and we end up in the same situation we are now. That's why I suggested a host bus type parameter if we want to do REPORT LUNS probes. No need to try REPORT LUNS on such devices. Are there any SCSI-1 devices still out there? I don't have any (last one burned out a while ago). James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH][RFC] scsi: Use W_LUN for scanning
On Sun, 2013-04-07 at 12:34 -0400, Douglas Gilbert wrote: On 13-04-07 12:15 PM, James Bottomley wrote: No specs apply that I can see. SPC-3 Section 8.3 Access Controls spc4r36f.pdf section 8.3.1.2 [Overview] Access controls are handled in the SCSI target device by an access controls coordinator located at the ACCESS CONTROLS well known logical unit. The access controls coordinator also may be accessible via LUN 0. The access controls coordinator associates a specific LUN to a specific logical unit depending on which initiator port accesses the SCSI target device and whether the initiator port has access rights to the logical unit. That seems to strengthen the argument for going the W_LUN route. The W-LUN route is only viable if we can find a way of making it work for everything (and that includes USB). The spec also says in 8.3.1.7 [Verifying access rights] If an INQUIRY command is addressed to a LUN for which there is no matching LUN value in any LUACD in any ACE allowing the initiator port logical unit access rights, the standard INQUIRY data (see 6.4.2) PERIPHERAL DEVICE TYPE field shall be set to 1Fh and the PERIPHERAL QUALIFIER field shall be set to 011b (i.e., the device server is not capable of supporting a device at this logical unit). That means our current LUN0 probe should work just fine. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
