Re: [PATCH] scsi_transport_fc: Make 'port_state' writeable

[Bart Van Assche]

On 03/15/13 12:55, Hannes Reinecke wrote:

And the LLDD is forced into error recovery which'll take _ages_ as each
and every command send during error recovery will time out.


Hello Hannes,

I'm analyzing a related but not identical issue with SRP. It would help 
if you could tell with which LLDD you ran into this issue and with which 
values of fast_io_fail_tmo and dev_loss_tmo.


Bart.

--
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] scsi_transport_fc: Make 'port_state' writeable

[Bart Van Assche]

On 03/15/13 13:37, Bryn M. Reeves wrote:

On 03/15/2013 12:24 PM, Bart Van Assche wrote:

On 03/15/13 12:55, Hannes Reinecke wrote:

And the LLDD is forced into error recovery which'll take _ages_ as each
and every command send during error recovery will time out.


Hello Hannes,

I'm analyzing a related but not identical issue with SRP. It would help
if you could tell with which LLDD you ran into this issue and with which
values of fast_io_fail_tmo and dev_loss_tmo.


Most of the cases I've seen have involved lpfc (although I don't think
it's in any way exclusive to that LLDD). Even with very low
fast_io_fail_timeout/dev_loss_timeout (5/10) the eh is busy for 10m or
longer before IO fails and multipath is able to react to the problem.


The SCSI EH keeps trying until all outstanding request have been 
finished. Does lpfc_host_reset_handler() invoke scsi_done() for 
outstanding requests ? If not, how about modifying 
lpfc_host_reset_handler() such that it finishes all outstanding requests 
if the remote port is not reachable ?


Bart.
--
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] scsi_transport_fc: Make 'port_state' writeable

[Bart Van Assche]

On 03/15/13 14:28, Bryn M. Reeves wrote:

On 03/15/2013 12:46 PM, Bart Van Assche wrote:

The SCSI EH keeps trying until all outstanding request have been
finished. Does lpfc_host_reset_handler() invoke scsi_done() for


I don't think so (ends up calling lpfc_sli_cancel_iocbs() via
lpfc_hba_down_post() after shutting down the mailbox) but I've not seen
the EH escalate all the way to host reset in most of my testing -
usually some time after reaching the bus reset remaining IOs timeout and
the error bubbles up to device-mapper (all the cases I'm looking at are
devices managed by a dm-multipath target).

The problem is that getting to this stage can take a very long time -
much longer than most cluster's node eviction timer for e.g. which is
the source of much of the complaint about this behaviour.


outstanding requests ? If not, how about modifying
lpfc_host_reset_handler() such that it finishes all outstanding requests
if the remote port is not reachable ?


I'm not sure how safe that is in this situation - James mentioned in the
I_T nexus reset thread concerns about frames that could be delayed etc.
in the fabric if the host unilaterally abandons IOs (not sure of the
details for lpfc at this level).


How about using the value of scsi_cmnd.jiffies_at_alloc to finish only 
those SCSI commands in the host reset handler that exceeded a certain 
processing time ?


Bart.

--
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] scsi_transport_fc: Make 'port_state' writeable

[Bart Van Assche]

On 03/15/13 19:51, Mike Christie wrote:

On 03/15/2013 08:41 AM, Bart Van Assche wrote:

How about using the value of scsi_cmnd.jiffies_at_alloc to finish only
those SCSI commands in the host reset handler that exceeded a certain
processing time ?


We basically do this now. When a scsi command times out the scsi layer
blocks the host from processing new commands and waits for all
outstanding commands to either finish normally or timeout. When all
commands have finished or timedout, then we start the scsi eh code. So,
by the time we have go to the scsi eh callbacks we are in a state where
all the commands being processed by the eh have exceeded a certain
processing time.

If you mean you want to drop the block and wait part, then I think it
could speed things up to do the abort callbacks while other IO is
running (as long as the driver can support it). However if the abort
fails and you need to escalate to operations like resets which interfere
with multiple commands, then the driver/scsi-ml does not have much
choice in what it does cleanup wise. There would be no point in checking
the jiffies_at_alloc. The commands that are going to be affected by the
tmf or host reset operation must be returned to the scsi-ml for retries
or failure upwards.


Hello Mike,

It seems like there is a misunderstanding. With my comment I was not 
referring to the SCSI ML but to the SCSI LLD. LLD drivers like ib_srp 
keep track of outstanding SCSI requests. With the SRP protocol it is 
possible to tell the InfiniBand HCA not to deliver completions for 
outstanding requests by closing the connection used for SRP 
communication. Hence my suggestion to finish SCSI commands that were 
queued longer than a certain time ago from inside the LLD host reset 
handler. I'm not sure though whether all types of FC HBA's allow 
something equivalent.


Bart.


--
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] block,scsi: verify return pointer from blk_get_request

[Bart Van Assche]

On 03/17/13 18:32, Joe Lawrence wrote:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..8f009c4 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1635,6 +1635,8 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 * request becomes available
 */
req = blk_get_request(sdev-request_queue, READ, GFP_KERNEL);
+   if (!req)
+   return -ENOMEM;

req-cmd[0] = ALLOW_MEDIUM_REMOVAL;
req-cmd[1] = 0;


scsi_eh_lock_door() doesn't return a value to it's caller so I think a 
plain return statement is sufficient here.


Bart.
--
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 V7 4/5] virtio-scsi: introduce multiqueue support

[Bart Van Assche]

On 03/23/13 12:28, Wanlong Gao wrote:

+static struct virtio_scsi_vq *virtscsi_pick_vq(struct virtio_scsi *vscsi,
+  struct virtio_scsi_target_state 
*tgt)
+{
+   struct virtio_scsi_vq *vq;
+   unsigned long flags;
+   u32 queue_num;
+
+   spin_lock_irqsave(tgt-tgt_lock, flags);
+
+   /*
+* The memory barrier after atomic_inc_return matches
+* the smp_read_barrier_depends() in virtscsi_req_done.
+*/
+   if (atomic_inc_return(tgt-reqs)  1)
+   vq = ACCESS_ONCE(tgt-req_vq);
+   else {
+   queue_num = smp_processor_id();
+   while (unlikely(queue_num = vscsi-num_queues))
+   queue_num -= vscsi-num_queues;
+
+   tgt-req_vq = vq = vscsi-req_vqs[queue_num];
+   }
+
+   spin_unlock_irqrestore(tgt-tgt_lock, flags);
+   return vq;
+}


Is there any reason why the smp_processor_id() % vscsi-num_queues 
computation in virtscsi_pick_vq() has been implemented as a loop instead 
of as an arithmetic operation ? If so, I would appreciate it if that 
could be explained in a comment.


Thanks,

Bart.
--
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

[Bart Van Assche]

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

[Bart Van Assche]

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()

[Bart Van Assche]

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

[Bart Van Assche]

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

[Bart Van Assche]

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()

[Bart Van Assche]

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

[Bart Van Assche]

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

[Bart Van Assche]

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

[Bart Van Assche]

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

[Bart Van Assche]

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] scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

[Bart Van Assche]

On 05/03/13 20:23, James Bottomley wrote:

+   const unsigned long stall_for = min(msecs_to_jiffies(10), 1UL);


Hello James,

Can you please clarify what the intention of this statement is ? Is the 
purpose of this statement to avoid that stall_for would be zero in case 
HZ  100 ? If that is the case, maybe you meant max() instead of min() ? 
Also, are you aware that msecs_to_jiffies() already rounds up the result 
of the division ?


Bart.

--
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] scsi: Allow error handling timeout to be specified

[Bart Van Assche]

On 05/10/13 05:11, Martin K. Petersen wrote:

Introduce eh_timeout which can be used for error handling purposes. This
was previously hardcoded to 10 seconds in the SCSI error handling
code. However, for some fast-fail scenarios it is necessary to be able
to tune this as it can take several iterations (bus device, target, bus,
controller) before we give up.


Hello Martin,

sdev-max_queue_depth = sdev-queue_depth;
+   sdev-eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;

Have you considered to move the eh_timeout assignment statement to just 
before the transport_configure_device() and slave_configure() calls ? 
That would allow transport drivers and LLD drivers to override the 
default eh_timeout value.


Bart.
--
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] qla2x00t: Fix a memory leak in an error path

[Bart Van Assche]

Avoid that the fcport structure gets leaked if
bsg_job-request-msgcode == FC_BSG_HST_ELS_NOLOGIN, the fcport
allocation succeeds and the !vha-flags.online branch is taken.
Detected by Coverity.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_bsg.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 39719f8..af35707 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -329,7 +329,7 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
if (!vha-flags.online) {
ql_log(ql_log_warn, vha, 0x7005, Host not online.\n);
rval = -EIO;
-   goto done;
+   goto done_free_fcport;
}
 
req_sg_cnt =
-- 
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] qla2x00t: Fix a memory leak in an error path

[Bart Van Assche]

On 05/17/13 07:00, Saurav Kashyap wrote:

Instead of doing this I would move this check to the top of the function,
something like this
-8--
diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
b/drivers/scsi/qla2xxx/qla_bsg.c
index 371bb86..b5f84ae 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -255,6 +255,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 int rval =  (DRIVER_ERROR  16);
 uint16_t nextlid = 0;


+   if (!vha-flags.online) {
+   ql_log(ql_log_warn, vha, 0x7005, Host not online.\n);
+   rval = -EIO;
+   goto done;
+   }
+
 if (bsg_job-request-msgcode == FC_BSG_RPT_ELS) {
 rport = bsg_job-rport;
 fcport = *(fc_port_t **) rport-dd_data;
@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 NPH_FABRIC_CONTROLLER : NPH_F_PORT;
 }


-   if (!vha-flags.online) {
-   ql_log(ql_log_warn, vha, 0x7005, Host not online.\n);
-   rval = -EIO;
-   goto done;
-   }
-
 req_sg_cnt =
 dma_map_sg(ha-pdev-dev,
bsg_job-request_payload.sg_list,
 bsg_job-request_payload.sg_cnt, DMA_TO_DEVICE);

8-


Hello Saurav,

I will update the patch, retest and repost it. Thanks for the feedback.

Bart.
--
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: SCSI testing/USB devices are amazing

[Bart Van Assche]

On 04/28/13 18:19, ronnie sahlberg wrote:

Hi List,

Interested in SCSI tests?

I have a reasonable SCSI (mainly SBC) testsuite at
https://github.com/sahlberg/libiscsi
while I am mainly interested in testing of iSCSI targets, most of my
tests so far are for the SCSI protocol
so they work quite well on devices connected to any kind of transport,
as long as you re-export them via iSCSI.
(For example using a simple passthrough device with TGT.)
At some stage I will redo some of the framework so that it can
optionally talk directly to a /dev/sg* device, but reexporting via TGT
works well enough for now.


Hello Ronnie,

Thanks for making this software publicly available. I have a question 
about one particular test though. In the output of the test suite I 
found the following: quoteTest READ16 with non-zero RDPROTECT. Device 
does not support/use protection information. All commands should 
fail./quote


However, in SBC-3 I found the following: quoteIf type 0 protection is 
enabled and the RDPROTECT field, the WRPROTECT field, the VRPROTECT 
field, or the ORPROTECT field is set to a non-zero value, then medium 
access commands are invalid and may be terminated by the device server 
with CHECK CONDITION status with the sense key set to ILLEGAL REQUEST

and the additional sense code set to INVALID FIELD IN CDB./quote

Apparently your test suite considers not failing READ16 with non-zero 
RDPROTECT field as an error. However, if I interpret the above paragraph 
from SBC-3 correctly that behavior seems compliant to me ?


Bart.

--
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] make dev_loss_tmo checks uniform across drivers

[Bart Van Assche]

On 06/03/13 20:25, gurinder.sherg...@hp.com wrote:

The dev_loss_tmo parameter controls how long the FC transport layer
insulates a loss of a remote port. It is defined to be between 1 and
SCSI_DEVICE_BLOCK_MAX_TIMEOUT (600). Different drivers do slightly different
checks when passed this parameter. This patch makes the dev_loss_tmo setting
uniform across all the drivers by pulling the common logic into the
transport layer function which is responsible for invoking the driver
provided callouts.

In addition, this patch also fixes a issue (see below), where dev_loss_tmo
could end up with a value more than SCSI_DEVICE_BLOCK_MAX_TIMEOUT when
fast_io_fail_tmo is enabled and then disabled.  The change is to never
allow dev_loss_tmo to be out of its defined range. And, once that is a
given, then fast_io_fail_tmo can be capped by dev_loss_tmo.


Hello Sunny,

As far as I know some users set fast_io_fail_tmo such that dev_loss_tmo 
can be set to a very large value in order to prevent that the SCSI host 
number changes. Sorry but I'm not sure it's a good idea to remove that 
functionality.


Bart.

--
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 01/10] qla2xxx: Clean up qla24xx_iidma()

[Bart Van Assche]

Remove dead code and simplify a pointer computation.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_bsg.c |9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 39719f8..7221521 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1282,14 +1282,7 @@ qla24xx_iidma(struct fc_bsg_job *bsg_job)
return -EINVAL;
}
 
-   port_param = (struct qla_port_param *)((char *)bsg_job-request +
-   sizeof(struct fc_bsg_request));
-   if (!port_param) {
-   ql_log(ql_log_warn, vha, 0x7047,
-   port_param header not provided.\n);
-   return -EINVAL;
-   }
-
+   port_param = (void *)bsg_job-request + sizeof(struct fc_bsg_request);
if (port_param-fc_scsi_addr.dest_type != EXT_DEF_TYPE_WWPN) {
ql_log(ql_log_warn, vha, 0x7048,
Invalid destination type.\n);
-- 
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 03/10] qla2xxx: Remove dead code in qla2x00_configure_hba()

[Bart Van Assche]

At the end of qla2x00_configure_hba() we know that rval == QLA_SUCCESS.
Hence remove the code that depends on rval != QLA_SUCCESS.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_init.c |8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 3565dfd..c68bb01 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2309,13 +2309,7 @@ qla2x00_configure_hba(scsi_qla_host_t *vha)
Topology - %s, Host Loop address 0x%x.\n,
connect_type, vha-loop_id);
 
-   if (rval) {
-   ql_log(ql_log_warn, vha, 0x2011,
-   %s FAILED\n, __func__);
-   } else {
-   ql_dbg(ql_dbg_disc, vha, 0x2012,
-   %s success\n, __func__);
-   }
+   ql_dbg(ql_dbg_disc, vha, 0x2012, %s success\n, __func__);
 
return(rval);
 }
-- 
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 02/10] qla2xxx: Clean up qla84xx_mgmt_cmd()

[Bart Van Assche]

Remove dead code, simplify a pointer computation and move the
ql84_mgmt assignment to just before its first use.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_bsg.c |9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 7221521..cf07491 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1084,14 +1084,6 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
return -EINVAL;
}
 
-   ql84_mgmt = (struct qla_bsg_a84_mgmt *)((char *)bsg_job-request +
-   sizeof(struct fc_bsg_request));
-   if (!ql84_mgmt) {
-   ql_log(ql_log_warn, vha, 0x703b,
-   MGMT header not provided, exiting.\n);
-   return -EINVAL;
-   }
-
mn = dma_pool_alloc(ha-s_dma_pool, GFP_KERNEL, mn_dma);
if (!mn) {
ql_log(ql_log_warn, vha, 0x703c,
@@ -1103,6 +1095,7 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
mn-entry_type = ACCESS_CHIP_IOCB_TYPE;
mn-entry_count = 1;
 
+   ql84_mgmt = (void *)bsg_job-request + sizeof(struct fc_bsg_request);
switch (ql84_mgmt-mgmt.cmd) {
case QLA84_MGMT_READ_MEM:
case QLA84_MGMT_GET_INFO:
-- 
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 04/10] qla2xxx: Remove two superfluous tests

[Bart Van Assche]

Since ha-model_desc is an array comparing it against NULL is
superfluous. Hence remove these tests.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_gs.c |3 +--
 drivers/scsi/qla2xxx/qla_os.c |3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index d0ea8b9..f26442a 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -1370,8 +1370,7 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha)
/* Model description. */
eiter = (struct ct_fdmi_hba_attr *) (entries + size);
eiter-type = __constant_cpu_to_be16(FDMI_HBA_MODEL_DESCRIPTION);
-   if (ha-model_desc)
-   strncpy(eiter-a.model_desc, ha-model_desc, 80);
+   strncpy(eiter-a.model_desc, ha-model_desc, 80);
alen = strlen(eiter-a.model_desc);
alen += (alen  3) ? (4 - (alen  3)) : 4;
eiter-len = cpu_to_be16(4 + alen);
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index ad72c1d..1d97626 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -2840,8 +2840,7 @@ skip_dpc:
qla2x00_dfs_setup(base_vha);
 
ql_log(ql_log_info, base_vha, 0x00fb,
-   QLogic %s - %s.\n,
-   ha-model_number, ha-model_desc ? ha-model_desc : );
+   QLogic %s - %s.\n, ha-model_number, ha-model_desc);
ql_log(ql_log_info, base_vha, 0x00fc,
ISP%04X: %s @ %s hdma%c host#=%ld fw=%s.\n,
pdev-device, ha-isp_ops-pci_info_str(base_vha, pci_info),
-- 
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 05/10] qla2xxx: Remove a dead assignment in qla24xx_build_scsi_crc_2_iocbs()

[Bart Van Assche]

Since the value of cur_seg is not used and since scsi_prot_sglist()
has no side effects it is safe to remove the statement
cur_seg = scsi_prot_sglist(cmd). Detected by Coverity.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_iocb.c |2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 15e4080..b589d24 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -1189,7 +1189,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct 
cmd_type_crc_2 *cmd_pkt,
uint32_t*cur_dsd, *fcp_dl;
scsi_qla_host_t *vha;
struct scsi_cmnd*cmd;
-   struct scatterlist  *cur_seg;
int sgc;
uint32_ttotal_bytes = 0;
uint32_tdata_bytes;
@@ -1396,7 +1395,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct 
cmd_type_crc_2 *cmd_pkt,
 
if (bundling  tot_prot_dsds) {
/* Walks dif segments */
-   cur_seg = scsi_prot_sglist(cmd);
cmd_pkt-control_flags |=
__constant_cpu_to_le16(CF_DIF_SEG_DESCR_ENABLE);
cur_dsd = (uint32_t *) crc_ctx_pkt-u.bundling.dif_address;
-- 
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 06/10] qla2xxx: Remove redundant assignments

[Bart Van Assche]

The value of the pointer called nxt is not used after the
nxt = qla24xx_copy_eft(ha, nxt) statement. Hence keep the function
call but remove the assignment.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_dbg.c |6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index cfa2a20..c612785 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1468,7 +1468,7 @@ qla25xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 
nxt = qla2xxx_copy_queues(ha, nxt);
 
-   nxt = qla24xx_copy_eft(ha, nxt);
+   qla24xx_copy_eft(ha, nxt);
 
/* Chain entries -- started with MQ. */
nxt_chain = qla25xx_copy_fce(ha, nxt_chain, last_chain);
@@ -1787,7 +1787,7 @@ qla81xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 
nxt = qla2xxx_copy_queues(ha, nxt);
 
-   nxt = qla24xx_copy_eft(ha, nxt);
+   qla24xx_copy_eft(ha, nxt);
 
/* Chain entries -- started with MQ. */
nxt_chain = qla25xx_copy_fce(ha, nxt_chain, last_chain);
@@ -2289,7 +2289,7 @@ qla83xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 copy_queue:
nxt = qla2xxx_copy_queues(ha, nxt);
 
-   nxt = qla24xx_copy_eft(ha, nxt);
+   qla24xx_copy_eft(ha, nxt);
 
/* Chain entries -- started with MQ. */
nxt_chain = qla25xx_copy_fce(ha, nxt_chain, last_chain);
-- 
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 07/10] qla2xxx: Help Coverity with analyzing ct_sns_pkt initialization

[Bart Van Assche]

Coverity reports Overrunning struct type ct_sns_req of 1228 bytes
by passing it to a function which accesses it at byte offset 8207
for each qla2x00_prep_ct_req(), qla2x00_prep_ct_fdmi_req() and
qla24xx_prep_ct_fm_req() call. Help Coverity to recognize that
these calls do not trigger a buffer overflow by making it explicit
that these three functions initializes both the request and reply
structures. This patch does not change any functionality.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_gs.c |   86 ++---
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index f26442a..1ad361b 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -99,17 +99,17 @@ qla24xx_prep_ms_iocb(scsi_qla_host_t *vha, uint32_t 
req_size, uint32_t rsp_size)
  * Returns a pointer to the intitialized @ct_req.
  */
 static inline struct ct_sns_req *
-qla2x00_prep_ct_req(struct ct_sns_req *ct_req, uint16_t cmd, uint16_t rsp_size)
+qla2x00_prep_ct_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t rsp_size)
 {
-   memset(ct_req, 0, sizeof(struct ct_sns_pkt));
+   memset(p, 0, sizeof(struct ct_sns_pkt));
 
-   ct_req-header.revision = 0x01;
-   ct_req-header.gs_type = 0xFC;
-   ct_req-header.gs_subtype = 0x02;
-   ct_req-command = cpu_to_be16(cmd);
-   ct_req-max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
+   p-p.req.header.revision = 0x01;
+   p-p.req.header.gs_type = 0xFC;
+   p-p.req.header.gs_subtype = 0x02;
+   p-p.req.command = cpu_to_be16(cmd);
+   p-p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
 
-   return (ct_req);
+   return p-p.req;
 }
 
 static int
@@ -188,8 +188,7 @@ qla2x00_ga_nxt(scsi_qla_host_t *vha, fc_port_t *fcport)
GA_NXT_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, GA_NXT_CMD,
-   GA_NXT_RSP_SIZE);
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, GA_NXT_CMD, GA_NXT_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
/* Prepare CT arguments -- port_id */
@@ -284,8 +283,7 @@ qla2x00_gid_pt(scsi_qla_host_t *vha, sw_info_t *list)
gid_pt_rsp_size);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, GID_PT_CMD,
-   gid_pt_rsp_size);
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, GID_PT_CMD, gid_pt_rsp_size);
ct_rsp = ha-ct_sns-p.rsp;
 
/* Prepare CT arguments -- port_type */
@@ -359,7 +357,7 @@ qla2x00_gpn_id(scsi_qla_host_t *vha, sw_info_t *list)
GPN_ID_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, GPN_ID_CMD,
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, GPN_ID_CMD,
GPN_ID_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
@@ -421,7 +419,7 @@ qla2x00_gnn_id(scsi_qla_host_t *vha, sw_info_t *list)
GNN_ID_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, GNN_ID_CMD,
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, GNN_ID_CMD,
GNN_ID_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
@@ -495,7 +493,7 @@ qla2x00_rft_id(scsi_qla_host_t *vha)
RFT_ID_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, RFT_ID_CMD,
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, RFT_ID_CMD,
RFT_ID_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
@@ -551,8 +549,7 @@ qla2x00_rff_id(scsi_qla_host_t *vha)
RFF_ID_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, RFF_ID_CMD,
-   RFF_ID_RSP_SIZE);
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, RFF_ID_CMD, RFF_ID_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
/* Prepare CT arguments -- port_id, FC-4 feature, FC-4 type */
@@ -606,8 +603,7 @@ qla2x00_rnn_id(scsi_qla_host_t *vha)
RNN_ID_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, RNN_ID_CMD,
-   RNN_ID_RSP_SIZE);
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, RNN_ID_CMD, RNN_ID_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
/* Prepare CT arguments -- port_id, node_name */
@@ -676,8 +672,7 @@ qla2x00_rsnn_nn(scsi_qla_host_t *vha)
ms_pkt = ha-isp_ops-prep_ms_iocb(vha, 0, RSNN_NN_RSP_SIZE);
 
/* Prepare CT request */
-   ct_req = qla2x00_prep_ct_req(ha-ct_sns-p.req, RSNN_NN_CMD,
-   RSNN_NN_RSP_SIZE);
+   ct_req = qla2x00_prep_ct_req(ha-ct_sns, RSNN_NN_CMD, RSNN_NN_RSP_SIZE);
ct_rsp = ha-ct_sns-p.rsp;
 
/* Prepare CT arguments -- node_name, symbolic node_name, size */
@@ -1262,18

[PATCH 08/10] qla2xxx: Fix qla2xxx_check_risc_status()

[Bart Van Assche]

Change the 'rval' variable from QLA_FUNCTION_TIMEOUT into QLA_SUCCESS
before starting a loop that is only executed if rval is initialized
to QLA_SUCCESS. Coverity reported that loop as dead code.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_isr.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 259d920..bd0e2fa 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2495,6 +2495,7 @@ qla2xxx_check_risc_status(scsi_qla_host_t *vha)
if (rval == QLA_SUCCESS)
goto next_test;
 
+   rval = QLA_SUCCESS;
WRT_REG_DWORD(reg-iobase_window, 0x0003);
for (cnt = 100; (RD_REG_DWORD(reg-iobase_window)  BIT_0) == 0 
rval == QLA_SUCCESS; cnt--) {
-- 
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 09/10] qla2xxx: Remove an unused variable from qla2x00_remove_one()

[Bart Van Assche]

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_os.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 1d97626..2176cc9 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -2980,14 +2980,12 @@ qla2x00_remove_one(struct pci_dev *pdev)
set_bit(UNLOADING, base_vha-dpc_flags);
mutex_lock(ha-vport_lock);
while (ha-cur_vport_count) {
-   struct Scsi_Host *scsi_host;
-
spin_lock_irqsave(ha-vport_slock, flags);
 
BUG_ON(base_vha-list.next == ha-vp_list);
/* This assumes first entry in ha-vp_list is always base vha */
vha = list_first_entry(base_vha-list, scsi_qla_host_t, list);
-   scsi_host = scsi_host_get(vha-host);
+   scsi_host_get(vha-host);
 
spin_unlock_irqrestore(ha-vport_slock, flags);
mutex_unlock(ha-vport_lock);
-- 
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 10/10] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

[Bart Van Assche]

Avoid that the fcport structure gets leaked if
bsg_job-request-msgcode == FC_BSG_HST_ELS_NOLOGIN, the fcport
allocation succeeds and the !vha-flags.online branch is taken.
This was detected by Coverity. However, Coverity does not recognize
that all qla2x00_process_els() callers specify either
FC_BSG_RPT_ELS or FC_BSG_HST_ELS_NOLOGIN in the field
bsg_job-request-msgcode and that the value of that field is not
modified inside that function. This results in a false positive
report about a possible memory leak in an error path for
bsg_job-request-msgcode values other than the two mentioned
values.  Make it easy for Coverity (and for humans) to recognize
that there is no fcport leak in the error path by changing the
bsg_job-request-msgcode == FC_BSG_HST_ELS_NOLOGIN test into
bsg_job-request-msgcode != FC_BSG_RPT_ELS.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Saurav Kashyap saurav.kash...@qlogic.com
---
 drivers/scsi/qla2xxx/qla_bsg.c |   14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index cf07491..f8a2634 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -255,6 +255,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
int rval =  (DRIVER_ERROR  16);
uint16_t nextlid = 0;
 
+   if (!vha-flags.online) {
+   ql_log(ql_log_warn, vha, 0x7005, Host not online.\n);
+   rval = -EIO;
+   goto done;
+   }
+
if (bsg_job-request-msgcode == FC_BSG_RPT_ELS) {
rport = bsg_job-rport;
fcport = *(fc_port_t **) rport-dd_data;
@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
NPH_FABRIC_CONTROLLER : NPH_F_PORT;
}
 
-   if (!vha-flags.online) {
-   ql_log(ql_log_warn, vha, 0x7005, Host not online.\n);
-   rval = -EIO;
-   goto done;
-   }
-
req_sg_cnt =
dma_map_sg(ha-pdev-dev, bsg_job-request_payload.sg_list,
bsg_job-request_payload.sg_cnt, DMA_TO_DEVICE);
@@ -399,7 +399,7 @@ done_unmap_sg:
goto done_free_fcport;
 
 done_free_fcport:
-   if (bsg_job-request-msgcode == FC_BSG_HST_ELS_NOLOGIN)
+   if (bsg_job-request-msgcode != FC_BSG_RPT_ELS)
kfree(fcport);
 done:
return rval;
-- 
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 0/10] qla2xxx: Reduce the number of Coverity warnings

[Bart Van Assche]

On 06/07/13 21:06, Saurav Kashyap wrote:

Thanks for the patches. Please share the warnings reported by Coverity, so
that its easy for us to review the patches.


Hello Saurav,

The Coverity warnings that led me to developing this patch series are as 
follows:
- For patches 1, 2, 3, 8: Logically dead code - execution cannot reach 
this statement.
- For patch 4: Array compared against 0 (NO_EFFECT) array_null: 
Comparing an array to null is not useful: ha-model_desc.

- For patch 5, 6 and 9: Unused pointer value.
- For patch 7: Overrunning struct type ct_sns_req of 1228 bytes by 
passing it to a function which accesses it at byte offset 8207.
- For patch 10: Resource leak (RESOURCE_LEAK) leaked_storage: Variable 
fcport going out of scope leaks the storage it points to.


Hope this helps,

Bart.
--
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 3/9] scsi: improved eh timeout handler

[Bart Van Assche]

On 06/11/13 20:57, James Bottomley wrote:

Actually, I think we can dump the workqueue altogether.  The only reason
we need it is because the current abort handlers wait for the command
and return the completion state.  However, all LLDs are capable of
emitting TMFs at interrupt level, so if we separated the emit from the
wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having
an abort issued (possibly by adding a pointer to the abort task), return
BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsuccessful and
escalate to LUN reset.

This is fully asynchronous, fully tracked and doesn't rely on work
queues.

The necessary additions for something like this are the from interrupt
issue abort and LUN reset, which could just be additional callbacks in
the host template.


Do we really need a new callback in the host template for a command 
abort that does not wait ? Several LLDs already have their own internal 
data structures for keeping track of the request state and can use these 
data structures to set a flag command has been aborted. If aborting a 
command fails and the command completes that flag can then be used to 
avoid a second invocation of scsi_done(). At least, that's what the SRP 
initiator already does today in srp_abort().


Bart.

--
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 v11 0/9] More device removal fixes

[Bart Van Assche]

Fix a few issues that can be triggered by removing a SCSI device:
- Fix a race between starved list processing and device removal that
  can trigger a kernel oops.
- 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.

Changes compared to v10:
- Rebased and retested on top of Linux kernel v3.10-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

--
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 v11 1/9] Fix race between starved list and device removal

[Bart Van Assche]

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 86d5220..d6ca072 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -456,11 +456,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 v11 2/9] Remove get_device() / put_device() pair from scsi_request_fn()

[Bart Van Assche]

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 d6ca072..03d4165 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1533,16 +1533,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.
@@ -1631,7 +1629,7 @@ static void scsi_request_fn(struct request_queue *q)
goto out_delay;
}
 
-   goto out;
+   return;
 
  not_ready:
spin_unlock_irq(shost-host_lock);
@@ -1650,12 +1648,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 v11 3/9] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

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 03d4165..d57b5e1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2175,6 +2175,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 v11 4/9] Disallow changing the device state via sysfs into deleted

[Bart Van Assche]

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 d57b5e1..e1cc1d1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2183,6 +2183,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 cc64587..cc1b52a 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 v11 5/9] Avoid saving/restoring interrupt state inside scsi_remove_host()

[Bart Van Assche]

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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

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 f43de1e..1f2f593 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -537,8 +537,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)
 {
@@ -553,6 +598,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) {
@@ -562,6 +610,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;
 }
@@ -583,6 +632,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) {
@@ -592,6 +644,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;
 }
@@ -622,6 +675,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);
@@ -629,6 +685,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;
 }
@@ -646,14 +703,20 @@ static int

PATCH v11 7/9] Avoid that scsi_device_set_state() triggers a race

[Bart Van Assche]

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 1f2f593..2c99eab 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1448,7 +1448,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 e1cc1d1..10c054f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2078,7 +2078,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)
@@ -2359,7 +2361,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;
 
@@ -2383,13 +2391,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);
@@ -2439,17 +2455,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
@@ -2484,13 +2502,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;
@@ -2502,7 +2523,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 v11 9/9] Avoid reenabling I/O after the transport became offline

[Bart Van Assche]

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 10c054f..024e768 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2162,7 +2162,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

[PATCH 0/14] IB SRP initiator patches for kernel 3.11

[Bart Van Assche]

The purpose of this InfiniBand SRP initiator patch series is as follows:
- Make the SRP initiator driver better suited for use in a H.A. setup.
  Speed up failover by reducing the IB RC retry count and by notifying
  multipathd faster about transport layer failures by adding
  fast_io_fail_tmo and dev_loss_tmo parameters.
- Make the SRP initiator better suited for use on NUMA systems by
  making the HCA completion vector configurable.

Some of the patches in this patch series were posted previously as Make 
ib_srp better suited for H.A. purposes. The changes compared to version 
five of that patch series are:

- Left out patches that are already upstream.
- Made it possible to set dev_loss_tmo to off. This is useful in a
  setup using initiator side mirroring to avoid that new /dev/sd* names
  are reassigned after a failover or cable pull and reinsert.
- Added kernel module parameters to ib_srp for configuring default
  values of the fast_io_fail_tmo and dev_loss_tmo parameters.
- Added a patch from Dotan Barak that fixes a kernel oops during rmmod
  triggered by resource allocation failure at module load time.
- Avoid duplicate connections by refusing relogins instead of dropping
  duplicate connections, as proposed by Sebastian Riemer.
- Added a patch from Sebastian Riemer for failing SCSI commands
  silently.
- Added a patch from Vu Pham to make the transport layer (IB RC) retry
  count configurable.
- Made HCA completion vector configurable.

Changes since v4:
- Added a patch for removing SCSI devices upon a port down event

Changes since v3:
- Restored the dev_loss_tmo and fast_io_fail_tmo sysfs attributes.
- Included a patch to fix an ib_srp crash that could be triggered by
  cable pulling.

Changes since v2:
- Addressed the v2 review comments.
- Dropped the patches that have already been merged.
- Dropped the patches for integration with multipathd.
- Dropped the micro-optimization of the IB completion handlers.

The individual patches in this series are as follows:
0001-IB-srp-Fix-remove_one-crash-due-to-resource-exhausti.patch
0002-IB-srp-Fix-race-between-srp_queuecommand-and-srp_cla.patch
0003-IB-srp-Avoid-that-srp_reset_host-is-skipped-after-a-.patch
0004-IB-srp-Skip-host-settle-delay.patch
0005-IB-srp-Maintain-a-single-connection-per-I_T-nexus.patch
0006-IB-srp-Keep-rport-as-long-as-the-IB-transport-layer.patch
0007-scsi_transport_srp-Add-transport-layer-error-handlin.patch
0008-IB-srp-Add-srp_terminate_io.patch
0009-IB-srp-Use-SRP-transport-layer-error-recovery.patch
0010-IB-srp-Start-timers-if-a-transport-layer-error-occur.patch
0011-IB-srp-Fail-SCSI-commands-silently.patch
0012-IB-srp-Make-HCA-completion-vector-configurable.patch
0013-IB-srp-Make-transport-layer-retry-count-configurable.patch
0014-IB-srp-Bump-driver-version-and-release-date.patch

--
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 06/14] IB/srp: Keep rport as long as the IB transport layer

[Bart Van Assche]

Keep the rport data structure around after srp_remove_host() has
finished until cleanup of the IB transport layer has finished
completely. This is necessary because later patches use the rport
pointer inside the queuecommand callback. Without this patch
accessing the rport from inside a queuecommand callback is racy
because srp_remove_host() must be invoked before scsi_remove_host()
and because the queuecommand callback may get invoked after
srp_remove_host() has finished.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 drivers/infiniband/ulp/srp/ib_srp.c |3 +++
 drivers/infiniband/ulp/srp/ib_srp.h |1 +
 drivers/scsi/scsi_transport_srp.c   |   18 ++
 include/scsi/scsi_transport_srp.h   |2 ++
 4 files changed, 24 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 1a73b24..2bd0587 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -526,11 +526,13 @@ static void srp_remove_target(struct srp_target_port 
*target)
WARN_ON_ONCE(target-state != SRP_TARGET_REMOVED);
 
srp_del_scsi_host_attr(target-scsi_host);
+   srp_rport_get(target-rport);
srp_remove_host(target-scsi_host);
scsi_remove_host(target-scsi_host);
srp_disconnect_target(target);
ib_destroy_cm_id(target-cm_id);
srp_free_target_ib(target);
+   srp_rport_put(target-rport);
srp_free_req_data(target);
scsi_host_put(target-scsi_host);
 }
@@ -2009,6 +2011,7 @@ static int srp_add_target(struct srp_host *host, struct 
srp_target_port *target)
}
 
rport-lld_data = target;
+   target-rport = rport;
 
spin_lock(host-target_lock);
list_add_tail(target-list, host-target_list);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h 
b/drivers/infiniband/ulp/srp/ib_srp.h
index 66fbedd..1817ed5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -153,6 +153,7 @@ struct srp_target_port {
u16 io_class;
struct srp_host*srp_host;
struct Scsi_Host   *scsi_host;
+   struct srp_rport   *rport;
chartarget_name[32];
unsigned intscsi_id;
unsigned intsg_tablesize;
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f379c7f..f7ba94a 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -185,6 +185,24 @@ static int srp_host_match(struct attribute_container 
*cont, struct device *dev)
 }
 
 /**
+ * srp_rport_get() - increment rport reference count
+ */
+void srp_rport_get(struct srp_rport *rport)
+{
+   get_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_get);
+
+/**
+ * srp_rport_put() - decrement rport reference count
+ */
+void srp_rport_put(struct srp_rport *rport)
+{
+   put_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_put);
+
+/**
  * srp_rport_add - add a SRP remote port to the device hierarchy
  * @shost: scsi host the remote port is connected to.
  * @ids:   The port id for the remote port.
diff --git a/include/scsi/scsi_transport_srp.h 
b/include/scsi/scsi_transport_srp.h
index ff0f04a..5a2d2d1 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -38,6 +38,8 @@ extern struct scsi_transport_template *
 srp_attach_transport(struct srp_function_template *);
 extern void srp_release_transport(struct scsi_transport_template *);
 
+extern void srp_rport_get(struct srp_rport *rport);
+extern void srp_rport_put(struct srp_rport *rport);
 extern struct srp_rport *srp_rport_add(struct Scsi_Host *,
   struct srp_rport_identifiers *);
 extern void srp_rport_del(struct srp_rport *);
-- 
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

Add the necessary functions in the SRP transport module to allow
an SRP initiator driver to implement transport layer error handling
similar to the functionality already provided by the FC transport
layer. This includes:
- Support for implementing fast_io_fail_tmo, the time that should
  elapse after having detected a transport layer problem and
  before failing I/O.
- Support for implementing dev_loss_tmo, the time that should
  elapse after having detected a transport layer problem and
  before removing a remote port.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 Documentation/ABI/stable/sysfs-transport-srp |   36 ++
 drivers/scsi/scsi_transport_srp.c|  489 +-
 include/scsi/scsi_transport_srp.h|   54 ++-
 3 files changed, 576 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp 
b/Documentation/ABI/stable/sysfs-transport-srp
index b36fb0d..b7759b1 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -5,6 +5,23 @@ Contact:   linux-scsi@vger.kernel.org, 
linux-r...@vger.kernel.org
 Description:   Instructs an SRP initiator to disconnect from a target and to
remove all LUNs imported from that target.
 
+What:  /sys/class/srp_remote_ports/port-h:n/dev_loss_tmo
+Date:  September 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before removing a target port.
+   Zero means immediate removal.
+
+What:  /sys/class/srp_remote_ports/port-h:n/fast_io_fail_tmo
+Date:  September 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before failing I/O. Zero means
+   immediate removal. A negative value will disable this
+   behavior.
+
 What:  /sys/class/srp_remote_ports/port-h:n/port_id
 Date:  June 27, 2007
 KernelVersion: 2.6.24
@@ -12,8 +29,27 @@ Contact: linux-scsi@vger.kernel.org
 Description:   16-byte local SRP port identifier in hexadecimal format. An
example: 4c:49:4e:55:58:20:56:49:4f:00:00:00:00:00:00:00.
 
+What:  /sys/class/srp_remote_ports/port-h:n/reconnect_delay
+Date:  September 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a reconnect
+   attempt failed before retrying.
+
 What:  /sys/class/srp_remote_ports/port-h:n/roles
 Date:  June 27, 2007
 KernelVersion: 2.6.24
 Contact:   linux-scsi@vger.kernel.org
 Description:   Role of the remote port. Either SRP Initiator or SRP Target.
+
+What:  /sys/class/srp_remote_ports/port-h:n/state
+Date:  September 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   State of the transport layer to the remote port. running if
+   the transport layer is operational; blocked if a transport
+   layer error has been encountered but the fail_io_fast_tmo
+   timer has not yet fired; fail-fast after the
+   fail_io_fast_tmo timer has fired and before the dev_loss_tmo
+   timer has fired; lost after the dev_loss_tmo timer has
+   fired and before the port is finally removed.
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f7ba94a..53b34b3 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -24,12 +24,15 @@
 #include linux/err.h
 #include linux/slab.h
 #include linux/string.h
+#include linux/delay.h
 
 #include scsi/scsi.h
 #include scsi/scsi_device.h
 #include scsi/scsi_host.h
 #include scsi/scsi_transport.h
+#include scsi/scsi_cmnd.h
 #include scsi/scsi_transport_srp.h
+#include scsi_priv.h
 #include scsi_transport_srp_internal.h
 
 struct srp_host_attrs {
@@ -38,7 +41,7 @@ struct srp_host_attrs {
 #define to_srp_host_attrs(host)((struct srp_host_attrs 
*)(host)-shost_data)
 
 #define SRP_HOST_ATTRS 0
-#define SRP_RPORT_ATTRS 3
+#define SRP_RPORT_ATTRS 8
 
 struct srp_internal {
struct scsi_transport_template t;
@@ -54,6 +57,28 @@ struct srp_internal {
 
 #definedev_to_rport(d) container_of(d, struct srp_rport, dev)
 #define transport_class_to_srp_rport(dev) dev_to_rport((dev)-parent)
+static inline struct Scsi_Host *rport_to_shost(struct

Re: [PATCH 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/13/13 21:43, Vu Pham wrote:
 Hello Bart,
 

 +What:/sys/class/srp_remote_ports/port-h:n/dev_loss_tmo
 +Date:September 1, 2013
 +KernelVersion:3.11
 +Contact:linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
 +Description:Number of seconds the SCSI layer will wait after a 
 transport
 +layer error has been observed before removing a target port.
 +Zero means immediate removal.
 A negative value will disable the target port removal.

 snip
 +
 +/**
 + * srp_tmo_valid() - check timeout combination validity
 + *
 + * If no fast I/O fail timeout has been configured then the device 
 loss timeout
 + * must be below SCSI_DEVICE_BLOCK_MAX_TIMEOUT. If a fast I/O fail 
 timeout has
 + * been configured then it must be below the device loss timeout.
 + */
 +int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
 +{
 +return (fast_io_fail_tmo  0  1 = dev_loss_tmo 
 +dev_loss_tmo = SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
 +|| (0 = fast_io_fail_tmo 
 +(dev_loss_tmo  0 ||
 + (fast_io_fail_tmo  dev_loss_tmo 
 +  dev_loss_tmo  LONG_MAX / HZ))) ? 0 : -EINVAL;
 +}
 +EXPORT_SYMBOL_GPL(srp_tmo_valid);
 fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative 
 value
 dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative 
 value

OK, will update the documentation such that it correctly refers to off 
instead of a negative value and I will also mention that dev_loss_tmo can now 
be disabled.

 snip

 + * srp_reconnect_rport - reconnect by invoking 
 srp_function_template.reconnect()
 + *
 + * Blocks SCSI command queueing before invoking reconnect() such that 
 the
 + * scsi_host_template.queuecommand() won't be invoked concurrently with
 + * reconnect(). This is important since a reconnect() implementation may
 + * reallocate resources needed by queuecommand(). Please note that this
 + * function neither waits until outstanding requests have finished 
 nor tries
 + * to abort these. It is the responsibility of the reconnect() 
 function to
 + * finish outstanding commands before reconnecting to the target port.
 + */
 +int srp_reconnect_rport(struct srp_rport *rport)
 +{
 +struct Scsi_Host *shost = rport_to_shost(rport);
 +struct srp_internal *i = to_srp_internal(shost-transportt);
 +struct scsi_device *sdev;
 +int res;
 +
 +pr_debug(SCSI host %s\n, dev_name(shost-shost_gendev));
 +
 +res = mutex_lock_interruptible(rport-mutex);
 +if (res) {
 +pr_debug(%s: mutex_lock_interruptible() returned %d\n,
 + dev_name(shost-shost_gendev), res);
 +goto out;
 +}
 +
 +spin_lock_irq(shost-host_lock);
 +scsi_block_requests(shost);
 +spin_unlock_irq(shost-host_lock);
 +
 In scsi_block_requests() definition, no locks are assumed held.

Good catch :-) However, if you look around in drivers/scsi you will see that 
several SCSI LLD drivers invoke scsi_block_requests() with the host lock held. 
I'm not sure whether these LLDs or the scsi_block_requests() documentation is 
incorrect. Anyway, I'll leave the locking statements out since these are not 
necessary around this call of scsi_block_requests().

 If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to 
 do extra block with scsi_block_requests()

Please keep in mind that srp_reconnect_rport() can be called from two different 
contexts: that function can not only be called from inside the SRP transport 
layer but also from inside the SCSI error handler (see also the 
srp_reset_device() modifications in a later patch in this series). If this 
function is invoked from the context of the SCSI error handler the chance is 
high that the SCSI device will have another state than SDEV_BLOCK. Hence the 
scsi_block_requests() call in this function.

 Shouldn't we avoid using both scsi_block/unblock_requests() and 
 scsi_target_block/unblock()?
 ie. in srp_start_tl_fail_timers()  call scsi_block_requests(), in 
 __rport_fast_io_fail_timedout() and rport_dev_loss_timedout() call 
 scsi_unblock_requests()
 and using scsi_block/unblock_requests in this function.

I agree that using only one of these two mechanisms would be more elegant. 
However, the advantage of using scsi_target_block() as long as fast_io_fail_tmo 
has not expired is that this functions set the SDEV_BLOCK state which is 
recognized by multipathd. The reason scsi_block_requests() is used in the above 
function is to prevent that a user can reenable requests via sysfs while a new 
InfiniBand RC connection is being established. A call of srp_queuecommand() 
concurrently with srp_create_target_ib() can namely result in a kernel crash.

 +while (scsi_request_fn_active(shost))
 +msleep(20);
 +
 +res = i-f-reconnect(rport);
 +scsi_unblock_requests(shost);
 +pr_debug(%s (state %d): transport.reconnect() returned %d\n,
 + dev_name(shost-shost_gendev), rport-state, res);
 +if (res == 0) {
 + 

Re: [PATCH 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/14/13 19:59, Vu Pham wrote:

On 06/13/13 21:43, Vu Pham wrote:

+/**
+ * srp_tmo_valid() - check timeout combination validity
+ *
+ * If no fast I/O fail timeout has been configured then the device
loss timeout
+ * must be below SCSI_DEVICE_BLOCK_MAX_TIMEOUT. If a fast I/O fail
timeout has
+ * been configured then it must be below the device loss timeout.
+ */
+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+return (fast_io_fail_tmo  0  1 = dev_loss_tmo 
+dev_loss_tmo = SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+|| (0 = fast_io_fail_tmo 
+(dev_loss_tmo  0 ||
+ (fast_io_fail_tmo  dev_loss_tmo 
+  dev_loss_tmo  LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with
negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with
negative value


OK, will update the documentation such that it correctly refers to
off instead of a negative value and I will also mention that
dev_loss_tmo can now be disabled.


It's not only the documentation but also the code logic, you cannot turn
dev_loss_tmo off if fast_io_fail_tmo already turned off and vice versa
with the return statement above.


Does this mean that you think it would be useful to disable both the 
fast_io_fail and the dev_loss mechanisms, and hence rely on the user to 
remove remote ports that have disappeared and on the SCSI command 
timeout to detect path failures ? I'll start testing this to see whether 
that combination does not trigger any adverse behavior.



If rport's state is already SRP_RPORT_BLOCKED, I don't think we need
to do extra block with scsi_block_requests()


Please keep in mind that srp_reconnect_rport() can be called from two
different contexts: that function can not only be called from inside
the SRP transport layer but also from inside the SCSI error handler
(see also the srp_reset_device() modifications in a later patch in
this series). If this function is invoked from the context of the SCSI
error handler the chance is high that the SCSI device will have
another state than SDEV_BLOCK. Hence the scsi_block_requests() call in
this function.

Yes, srp_reconnect_rport() can be called from two contexts; however, it
deals with same rport  rport's state.
I'm thinking something like this:

if (rport-state != SRP_RPORT_BLOCKED) {
 scsi_block_requests(shost);


Sorry but I'm afraid that that approach would still allow the user to 
unblock one or more SCSI devices via sysfs during the 
i-f-reconnect(rport) call, something we do not want.



I think that we can use only the pair
scsi_block_requests()/scsi_unblock_requests() unless the advantage of
multipathd recognizing the SDEV_BLOCK is noticeable.


I think the advantage of multipathd recognizing the SDEV_BLOCK state 
before the fast_io_fail_tmo timer has expired is important. Multipathd 
does not queue I/O to paths that are in the SDEV_BLOCK state so setting 
that state helps I/O to fail over more quickly, especially for large 
values of fast_io_fail_tmo.


Hope this helps,

Bart.
--
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/17/13 08:18, Hannes Reinecke wrote:

On 06/15/2013 11:52 AM, Bart Van Assche wrote:

On 06/14/13 19:59, Vu Pham wrote:

On 06/13/13 21:43, Vu Pham wrote:

+/**
+ * srp_tmo_valid() - check timeout combination validity
+ *
+ * If no fast I/O fail timeout has been configured then the
device
loss timeout
+ * must be below SCSI_DEVICE_BLOCK_MAX_TIMEOUT. If a fast I/O
fail
timeout has
+ * been configured then it must be below the device loss timeout.
+ */
+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+return (fast_io_fail_tmo  0  1 = dev_loss_tmo 
+dev_loss_tmo = SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+|| (0 = fast_io_fail_tmo 
+(dev_loss_tmo  0 ||
+ (fast_io_fail_tmo  dev_loss_tmo 
+  dev_loss_tmo  LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with
negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with
negative value


OK, will update the documentation such that it correctly refers to
off instead of a negative value and I will also mention that
dev_loss_tmo can now be disabled.


It's not only the documentation but also the code logic, you
cannot turn
dev_loss_tmo off if fast_io_fail_tmo already turned off and vice
versa
with the return statement above.


Does this mean that you think it would be useful to disable both the
fast_io_fail and the dev_loss mechanisms, and hence rely on the user
to remove remote ports that have disappeared and on the SCSI command
timeout to detect path failures ? I'll start testing this to see
whether that combination does not trigger any adverse behavior.


If rport's state is already SRP_RPORT_BLOCKED, I don't think we
need
to do extra block with scsi_block_requests()


Please keep in mind that srp_reconnect_rport() can be called from
two
different contexts: that function can not only be called from inside
the SRP transport layer but also from inside the SCSI error handler
(see also the srp_reset_device() modifications in a later patch in
this series). If this function is invoked from the context of the
SCSI
error handler the chance is high that the SCSI device will have
another state than SDEV_BLOCK. Hence the scsi_block_requests()
call in
this function.

Yes, srp_reconnect_rport() can be called from two contexts;
however, it
deals with same rport  rport's state.
I'm thinking something like this:

 if (rport-state != SRP_RPORT_BLOCKED) {
  scsi_block_requests(shost);


Sorry but I'm afraid that that approach would still allow the user
to unblock one or more SCSI devices via sysfs during the
i-f-reconnect(rport) call, something we do not want.


I think that we can use only the pair
scsi_block_requests()/scsi_unblock_requests() unless the advantage of
multipathd recognizing the SDEV_BLOCK is noticeable.


I think the advantage of multipathd recognizing the SDEV_BLOCK state
before the fast_io_fail_tmo timer has expired is important.
Multipathd does not queue I/O to paths that are in the SDEV_BLOCK
state so setting that state helps I/O to fail over more quickly,
especially for large values of fast_io_fail_tmo.


Sadly it doesn't work that way.

SDEV_BLOCK will instruct multipath to not queue _new_ I/Os to the
path, but there still will be I/O queued on that path.
For these multipath _has_ to wait for I/O completion.
And as it turns out, in most cases the application itself will wait
for completion on these I/O before continue sending more I/O.
So in effect multipath would queue new I/O to other paths, but won't
_receive_ new I/O as the upper layers are still waiting for
completion of the queued I/O.

The only way to excite fast failover with multipathing is to set
fast_io_fail to a _LOW_ value (eg 5 seconds), as this will terminate
the outstanding I/Os.

Large values of fast_io_fail will almost guarantee sluggish I/O
failover.


Hello Hannes,

I agree that the value of fast_io_fail_tmo should be kept small. 
Although as you explained changing the SCSI device state into SDEV_BLOCK 
doesn't help for I/O that has already been queued on a failed path, I 
think it's still useful for I/O that is queued after the fast_io_fail 
timer has been started and before that timer has expired.


Bart.

--
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/17/13 09:14, Hannes Reinecke wrote:

On 06/17/2013 09:04 AM, Bart Van Assche wrote:

I agree that the value of fast_io_fail_tmo should be kept small.
Although as you explained changing the SCSI device state into
SDEV_BLOCK doesn't help for I/O that has already been queued on a
failed path, I think it's still useful for I/O that is queued after
the fast_io_fail timer has been started and before that timer has
expired.


Why, but of course.

The typical scenario would be:
- detect link-loss
- call scsi_block_request()
- start dev_loss_tmo and fast_io_fail_tmo

- When fast_io_fail_tmo triggers:
- Abort all outstanding requests

- When dev_loss_tmo triggers:
- Abort all outstanding requests
- Remove/disable the I_T nexus
- call scsi_unblock_request()

However, if and whether multipath detects SDEV_BLOCK doesn't
guarantee a fast failover; in fact is was only added rather recently
as it's not a big win in most cases.


Even if setting the state SDEV_BLOCK doesn't help much with improving 
failover time, it still has the advantage over using 
scsi_block_requests() that it can be overridden by a user via sysfs.


Bart.

--
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/19/13 15:44, Jack Wang wrote:

+   /*
+* It can occur that after fast_io_fail_tmo expired and before
+* dev_loss_tmo expired that the SCSI error handler has
+* offlined one or more devices. scsi_target_unblock() doesn't
+* change the state of these devices into running, so do that
+* explicitly.
+*/
+   spin_lock_irq(shost-host_lock);
+   __shost_for_each_device(sdev, shost)
+   if (sdev-sdev_state == SDEV_OFFLINE)
+   sdev-sdev_state = SDEV_RUNNING;
+   spin_unlock_irq(shost-host_lock);


Do you have test case to verify this behaviour?


Hello Jack,

This is what I came up with after analyzing why a so-called port 
flapping test failed. The concept of that test is simple: use 
ibportstate to disable and reenable the proper IB port on the switch 
with random intervals and check whether I/O starts running again if the 
path remains operational long enough. When running such a test for a few 
days with random intervals between a few seconds and a few minutes 
sooner or later it will occur that scsi_try_host_reset() succeeds and 
that scsi_eh_test_devices() fails. That will cause the SCSI error 
handler to offline devices. Hence the above code to change the offline 
state into running after a reconnect succeeds. I'm not proud of that 
code but I couldn't find a better solution. Maybe the above code won't 
be necessary anymore once we switch to Hannes' new SCSI error handler.


Bart.
--
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 v3 3/6] [SCSI] Add support for scsi_target events

[Bart Van Assche]

On 06/19/13 19:42, Ewan D. Milne wrote:

+static void starget_evt_emit(struct scsi_target *starget,
+struct starget_event *evt)
+{
+   int idx = 0;
+   char *envp[3];
+
+   switch (evt-evt_type) {
+   case STARGET_EVT_LUN_CHANGE_REPORTED:
+   envp[idx++] = STARGET_LUN_CHANGE_REPORTED=1;
+   break;
+   default:
+   /* do nothing */
+   break;
+   }
+
+   envp[idx++] = NULL;
+
+   kobject_uevent_env(starget-dev.kobj, KOBJ_CHANGE, envp);
+}


Sorry but it's not clear to me why the envp[] array has size three while 
at most two entries are used ? And shouldn't that array be declared as 
const char*[] instead of char *[] since string constants have type const 
char[] ?



+   list_for_each_safe(this, tmp, event_list) {
+   evt = list_entry(this, struct starget_event, node);


Any reason why list_for_each_entry_safe() has not been used here ?


+void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(starget-list_lock, flags);
+   list_add_tail(evt-node, starget-event_list);
+   schedule_work(starget-event_work);
+   spin_unlock_irqrestore(starget-list_lock, flags);
+}


Is it necessary here to invoke schedule_work() under protection of 
list_lock, or would it be safe to invoke schedule_work() after the 
spin_unlock_irqrestore() ?



+#ifdef CONFIG_SCSI_ENHANCED_UA
+   struct list_head *this, *tmp;
+
+   cancel_work_sync(starget-event_work);
+
+   list_for_each_safe(this, tmp, starget-event_list) {
+   struct starget_event *evt;

+   evt = list_entry(this, struct starget_event, node);
+   list_del(evt-node);
+   kfree(evt);
+   }
+#endif


Same question here: any reason why list_for_each_entry_safe() has not 
been used ?


Thanks,

Bart.
--
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 v11 3/9] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

On 06/23/13 23:35, Mike Christie wrote:

On 06/12/2013 07:52 AM, Bart Van Assche wrote:

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 03d4165..d57b5e1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2175,6 +2175,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);


The Addition to scsi_device_set_state looks ok. I was not sure why we
need the new WARNs though. Do we still think there are other cases?


Hello Mike,

The new WARN statements helped me while testing and debugging this code. 
These statements might also be helpful if anyone would ever add more 
SCSI device states. I can leave these out if you want.


Bart.

--
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 v11 4/9] Disallow changing the device state via sysfs into deleted

[Bart Van Assche]

On 06/24/13 03:05, Mike Christie wrote:

On 6/12/13 7:53 AM, Bart Van Assche wrote:

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


Maybe it should be named SDEV_INVALID_STATE to match the current naming
scheme? Other than that, it seems ok.


If nobody objects I will rename that new state into SDEV_INVALID_STATE.

Bart.

--
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

On 06/24/13 03:15, Mike Christie wrote:
 On 6/12/13 7:55 AM, Bart Van Assche wrote:
 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.
 
 I think the patch looks ok for drivers that do not implement their own
 eh_strategy_handler, but what about SAS? If you added a scsi_begin_eh in 
 scsi_error_handler before the eh_strategy_handler is called and then add 
 a scsi_end_eh after it is called, I think it would cover them too.

I will start testing the modification below for the patch at the start of
this thread:

--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1950,10 +1950,14 @@ int scsi_error_handler(void *data)
continue;
}
 
-   if (shost-transportt-eh_strategy_handler)
-   shost-transportt-eh_strategy_handler(shost);
-   else
+   if (shost-transportt-eh_strategy_handler) {
+   if (scsi_begin_eh(shost) == 0) {
+   shost-transportt-eh_strategy_handler(shost);
+   scsi_end_eh(shost);
+   }
+   } else {
scsi_unjam_host(shost);
+   }
 
/*
 * Note - if the above fails completely, the action is to take


 @@ -1894,6 +1962,9 @@ int scsi_error_handler(void *data)
   }
   __set_current_state(TASK_RUNNING);

 +WARN_ONCE(shost-eh_active, scsi_eh_%d: eh_active = %d\n,
 +  shost-host_no, shost-eh_active);
 +
   SCSI_LOG_ERROR_RECOVERY(1,
   printk(Error handler scsi_eh_%d exiting\n, shost-host_no));
   shost-ehandler = NULL;
 
 What is the warn for? Is there a chance this can happen with some non 
 upstream driver or are you just adding it just in case?

This is code that helped me to test this patch. I can leave it out if
you prefer so.

Bart.

--
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 v11 2/9] Remove get_device() / put_device() pair from scsi_request_fn()

[Bart Van Assche]

On 06/24/13 04:36, James Bottomley wrote:

On Wed, 2013-06-12 at 14:51 +0200, Bart Van Assche wrote:

Now that all scsi_request_fn() callers hold a reference on the
SCSI device that function is invoked for


What makes you think that this is a true statement?  The usual caller is
the block layer, which doesn't really know anything about the
sdev-sdev_gendev.


The reasoning behind that comment is as follows:
* The block layer guarantees that the reference count of a request
  queue is = 1 as long as a request_fn() call is in progress (see also
  blk_cleanup_queue(), the __blk_drain_queue() call in that function
  and the loop in __blk_drain_queue() that waits until
  request_fn_active == 0).
* The SCSI core guarantees that blk_cleanup_queue() is invoked before
  the final put on sdev-sdev_gendev.

Bart.

--
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/23/13 23:13, Mike Christie wrote:
 On 06/12/2013 08:28 AM, Bart Van Assche wrote:
 +/*
 + * It can occur that after fast_io_fail_tmo expired and before
 + * dev_loss_tmo expired that the SCSI error handler has
 + * offlined one or more devices. scsi_target_unblock() doesn't
 + * change the state of these devices into running, so do that
 + * explicitly.
 + */
 +spin_lock_irq(shost-host_lock);
 +__shost_for_each_device(sdev, shost)
 +if (sdev-sdev_state == SDEV_OFFLINE)
 +sdev-sdev_state = SDEV_RUNNING;
 +spin_unlock_irq(shost-host_lock);
 
 Is it possible for this to race with scsi_eh_offline_sdevs? Can it be
 looping over cmds offlining devices while this is looping over devices
 onlining them?
 
 It seems this can also happen for all transports/drivers. Maybe a a scsi
 eh/lib helper function that syncrhonizes with the scsi eh completion
 would be better.

I'm not sure it's possible to avoid such a race without introducing
a new mutex. How about something like the (untested) SCSI core patch
below, and invoking scsi_block_eh() and scsi_unblock_eh() around any
reconnect activity not initiated from the SCSI EH thread ?

[PATCH] Add scsi_block_eh() and scsi_unblock_eh()

---
 drivers/scsi/hosts.c  |1 +
 drivers/scsi/scsi_error.c |   10 ++
 include/scsi/scsi_host.h  |1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 17e2ccb..0df3ec8 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -360,6 +360,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
init_waitqueue_head(shost-host_wait);
 
mutex_init(shost-scan_mutex);
+   mutex_init(shost-block_eh_mutex);
 
/*
 * subtract one because we increment first then return, but we need to
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ab16930..566daaa 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -551,6 +551,10 @@ static int scsi_begin_eh(struct Scsi_Host *host)
 {
int res;
 
+   res = mutex_lock_interruptible(host-block_eh_mutex);
+   if (res)
+   goto out;
+
spin_lock_irq(host-host_lock);
switch (host-shost_state) {
case SHOST_DEL:
@@ -565,6 +569,10 @@ static int scsi_begin_eh(struct Scsi_Host *host)
}
spin_unlock_irq(host-host_lock);
 
+   if (res)
+   mutex_unlock(host-block_eh_mutex);
+
+out:
return res;
 }
 
@@ -579,6 +587,8 @@ static void scsi_end_eh(struct Scsi_Host *host)
if (host-eh_active == 0)
wake_up(host-host_wait);
spin_unlock_irq(host-host_lock);
+
+   mutex_unlock(host-block_eh_mutex);
 }
 
 /**
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 9785e51..d7ce065 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -573,6 +573,7 @@ struct Scsi_Host {
spinlock_t  *host_lock;
 
struct mutexscan_mutex;/* serialize scanning activity */
+   struct mutexblock_eh_mutex; /* block ML LLD EH calls */
 
struct list_headeh_cmd_q;
struct task_struct* ehandler;  /* Error recovery thread. */
-- 
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

On 06/24/13 12:17, Jack Wang wrote:

@@ -646,14 +703,20 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
  static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
  {
 int rtn;
-   struct scsi_host_template *hostt = scmd-device-host-hostt;
+   struct Scsi_Host *host = scmd-device-host;
+   struct scsi_host_template *hostt = host-hostt;

 if (!hostt-eh_device_reset_handler)
 return FAILED;

+   if (scsi_begin_eh(host))
+   return FAST_IO_FAIL;
+
 rtn = hostt-eh_device_reset_handler(scmd);
 if (rtn == SUCCESS)
 __scsi_report_device_reset(scmd-device, NULL);
+   scsi_end_eh(host);
+
 return rtn;
  }


As the new eh from Hannes haven't make it into mainline, maybe we still
need also check scsi_try_to_abort_cmd?


I don't think such checks are necessary in scsi_try_to_abort_cmd(). 
scsi_remove_host() already waits until outstanding SCSI commands have 
finished. scsi_try_to_abort_cmd() only gets invoked if there are still 
one or more unfinished commands so scsi_try_to_abort_cmd() won't be 
invoked anymore after scsi_remove_host() has finished.


Bart.

--
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 v11 2/9] Remove get_device() / put_device() pair from scsi_request_fn()

[Bart Van Assche]

On 06/24/13 15:34, James Bottomley wrote:

On Mon, 2013-06-24 at 09:13 +0200, Bart Van Assche wrote:

On 06/24/13 04:36, James Bottomley wrote:

On Wed, 2013-06-12 at 14:51 +0200, Bart Van Assche wrote:

Now that all scsi_request_fn() callers hold a reference on the
SCSI device that function is invoked for


What makes you think that this is a true statement?  The usual caller is
the block layer, which doesn't really know anything about the
sdev-sdev_gendev.


The reasoning behind that comment is as follows:
* The block layer guarantees that the reference count of a request
queue is = 1 as long as a request_fn() call is in progress (see also
blk_cleanup_queue(), the __blk_drain_queue() call in that function
and the loop in __blk_drain_queue() that waits until
request_fn_active == 0).
* The SCSI core guarantees that blk_cleanup_queue() is invoked before
the final put on sdev-sdev_gendev.


That's still unclear.

I think a clear explanation is something like:

 scsi_devices may only be removed by by scsi_remove_device()
 which has a call to blk_cleanup_queue() before the final put of
 sdev-sdev_gendev.  Since blk_cleanup_queue() waits for the
 block queue to drain and then tears it down, scsi_request_fn
 cannot be active once blk_cleanup_queue() returns and hence the
 get_device/put_device pairs in scsi_request_fn are unnecessary
 because the final put will always be done by
 scsi_remove_device().

This, by the way, is an optimisation not a fix, so it shouldn't really
be part of a series labelled device removal fixes


Thanks for the feedback. I will update the patch description and take 
this patch out of the device removal fixes patch series.


Bart.

--
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 07/14] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/24/13 15:48, Jack Wang wrote:

I'm not sure it's possible to avoid such a race without introducing
a new mutex. How about something like the (untested) SCSI core patch
below, and invoking scsi_block_eh() and scsi_unblock_eh() around any
reconnect activity not initiated from the SCSI EH thread ?

[PATCH] Add scsi_block_eh() and scsi_unblock_eh()


Hi Bart,

The description doesn't match the code at all, do you mean try to
serialize the reconnect activity with this new block_eh_mutex?


In case it wasn't clear, the actual scsi_block_eh() and 
scsi_unblock_eh() calls aren't present in the patch I posted, only their 
implementation.


P.S.: please fix your e-mail client such that it does not break e-mail 
threading. There are no In-Reply-To: tags in the header of your e-mails.


Thanks,

Bart.
--
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 v11 1/9] Fix race between starved list and device removal

[Bart Van Assche]

On 06/24/13 17:38, James Bottomley wrote:

I really don't like this because it's shuffling potentially fragile
lifetime rules since you now have to have the sdev deleted from the
starved list before final put.  That becomes an unstated assumption
within the code.

The theory is that the starved list processing may be racing with a
scsi_remove_device, so when we unlock the host lock, the device (and the
queue) may be destroyed.  OK, so I agree with this, remote a possibility
though it may be.  The easy way of fixing it without making assumptions
is this, isn't it?  All it requires is that the queue be destroyed after
the starved list entry is deleted in the sdev release code.

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..f294cc6 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -434,6 +434,7 @@ static void scsi_run_queue(struct request_queue *q)
list_splice_init(shost-starved_list, starved_list);

while (!list_empty(starved_list)) {
+   struct request_queue *slq;
/*
 * As long as shost is accepting commands and we have
 * starved queues, call blk_run_queue. scsi_request_fn
@@ -456,10 +457,21 @@ static void scsi_run_queue(struct request_queue *q)
continue;
}

+   /*
+* once we drop the host lock, a racing scsi_remove_device may
+* remove the sdev from the starved list and destroy it and
+* the queue.  Mitigate by taking a reference to the queue and
+* never touching the sdev again after we drop the host lock.
+*/
+   slq = sdev-request_queue;
+   if (!blk_get_queue(slq))
+   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);
+
+   blk_run_queue(slq);
+   blk_put_queue(slq);
+
spin_lock(shost-host_lock);
}
/* put any unprocessed entries back */


Since the above patch invokes blk_put_queue() with interrupts disabled 
it may cause blk_release_queue() to be invoked with interrupts disabled. 
Sorry but I'm not sure whether that will work fine.


Bart.
--
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: RFC: Allow block drivers to poll for I/O instead of sleeping

[Bart Van Assche]

On 06/25/13 05:18, Matthew Wilcox wrote:

On Mon, Jun 24, 2013 at 10:07:51AM +0200, Ingo Molnar wrote:

I'm wondering, how will this scheme work if the IO completion latency is a
lot more than the 5 usecs in the testcase? What if it takes 20 usecs or
100 usecs or more?


There's clearly a threshold at which it stops making sense, and our
current NAND-based SSDs are almost certainly on the wrong side of that
threshold!  I can't wait for one of the post-NAND technologies to make
it to market in some form that makes it economical to use in an SSD.

The problem is that some of the people who are looking at those
technologies are crazy.  They want to bypass the kernel and do user
space I/O because the kernel is too slow.  This patch is part of an
effort to show them how crazy they are.  And even if it doesn't convince
them, at least users who refuse to rewrite their applications to take
advantage of magical userspace I/O libraries will see real performance
benefits.


Recently I attended an interesting talk about this subject in which it 
was proposed not only to bypass the kernel for access to high-IOPS 
devices but also to allow byte-addressability for block devices. The 
slides that accompanied that talk can be found here (includes a 
performance comparison with the traditional block driver API):


Bernard Metzler, On Suitability of High-Performance Networking API for 
Storage, OFA Int'l Developer Workshop, April 24, 2013 
(http://www.openfabrics.org/ofa-documents/presentations/doc_download/559-on-suitability-of-high-performance-networking-api-for-storage.html).


This approach leaves the choice of whether to use polling or an 
interrupt-based completion notification to the user of the new API, 
something the Linux InfiniBand RDMA verbs API already allows today.


Bart.
--
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

On 06/25/13 00:27, James Bottomley wrote:

For a variety of reasons this patch set is incredibly hard to review:
Almost every patch touches pieces in the mid layer where you have to be
sure in minute detail you know what's going on (and what should be going
on), so usually it's a couple of hours with the source code just making
sure you do know this.  Plus it's code where the underlying usage model
has evolved over the years meaning the original guarantees might have
been violated silently somewhere and the ramifications spread out like
tentacles everywhere.  Finally, it's not clear from the change logs why
the changes are actually being made: for instance sentence one of this
change log says A SCSI LLD may start cleaning up host resources as soon
as scsi_remove_host() returns. which causes my sanity checker to go off
immediately because in a refcounted model, like we use for dev, target
and host, nothing essential is supposed to be freed until the last put
which may or may not happen in the remove function.


If the invocations of the eh_* callback functions would be visible to 
the block layer then blk_cleanup_queue() would wait until any such eh_* 
invocations have finished. Such an approach could simplify device 
removal in the SCSI mid-layer significantly. It also would avoid that an 
eh_* callback can be invoked via an ioctl after scsi_remove_device() has 
finished.


Bart.

--
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 v11 3/9] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

On 06/25/13 15:44, James Bottomley wrote:

On Tue, 2013-06-25 at 10:37 +0200, Bart Van Assche wrote:

On 06/24/13 19:38, James Bottomley wrote:

On Wed, 2013-06-12 at 14:52 +0200, Bart Van Assche wrote:

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().


The current principle is that scsi_remove_device can fail, so the
condition you're avoiding is expected.  If you want to make it always
succeed, we have to worry about any device state racing with an
asynchronous remove, which looks like a whole nasty can of worms.

The change log makes it sound like what you actually want to enable is
the ability to remove devices which fail probing but which are in the
blocked state, so why not just respin with only that, which is just
adding the blocked states to the -SDEV_DEL state transitions?


If what you had in mind is the patch below, I think we agree:

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e3d6276..eaea242 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2185,6 +2185,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:


Something like this, yes.  For the probe lun case, we have to be in
CREATED, so any block action transitions only to CREATED_BLOCK.  The
BLOCK-DEL transition can only be a result of an async remove racing
with bringup, can't it?  Which is something I think we still want to
forbid.


OK, I will leave the BLOCK-DEL transition out.

Bart.

--
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

On 06/25/13 15:45, James Bottomley wrote:

On Tue, 2013-06-25 at 11:01 +0200, Bart Van Assche wrote:

There is a difference though between moving the EH kthread_stop() call
and the patch at the start of this thread: moving the EH kthread_stop()
call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_*
callback after scsi_remove_host() has finished. However, the
scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can
cause an eh_* callback to be invoked after scsi_remove_device() finished.


OK, but this doesn't tell me what you're trying to achieve.

An eh function is allowable as long as the host hadn't had the release
callback executed.  That means you must have to have a reference to the
device/host to execute the eh function, which is currently guaranteed
for all invocations.


That raises a new question: how is an LLD expected to clean up resources 
without triggering a race condition ? What you wrote means that it's not 
safe for an LLD to start cleaning up the resources needed by the eh_* 
callbacks immediately after scsi_remove_device() returns since it it not 
guaranteed that at that time all references to the device have already 
been dropped.


Bart.

--
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 v11 6/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

On 06/25/13 19:40, James Bottomley wrote:

If I look at what we actually do: all the HBAs treat scsi_remove_host as
a waited for transition.  The reason this works is the loop over
__scsi_remove_device() in scsi_forget_host().  By the time that loop
returns, every scsi_device is gone (and so is every target).  Because
blk_cleanup_queue() induces a synchronous wait for the queue to die in
__scsi_remove_device(), there can be no outstanding I/O and no eh
activity for the device when it returns (and no possibility of starting
any).  Thus at the end of scsi_forget_host, we have no devices to start
I/O and no eh activity, so the final put will be the last.


With the patch at the start of this thread everything works like you 
described above. However, without that patch EH activity can continue 
after scsi_remove_device() has returned. I have seen that occurring 
several times while testing SCSI LLD patches.


Bart.

--
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] scsi_prep_fn() check for empty queue

[Bart Van Assche]

On 06/26/13 11:02, Maxim Uvarov wrote:

This fix:
end_request: I/O error, dev sdc, sector 976576
  rport-0:0-3: blocked FC remote port time out: removing target and saving
binding
BUG: unable to handle kernel NULL pointer dereference at 0400
IP: [812f0cc2] scsi_prep_state_check+0xe/0x99
  [812f1f9d] scsi_setup_blk_pc_cmnd+0x1b/0x115
  [812f20c0] scsi_prep_fn+0x29/0x3b
  [8121cfb9] blk_peek_request+0xe1/0x1b3
  [812f1400] scsi_request_fn+0x3a/0x4d2
  [8121d916] __generic_unplug_device+0x32/0x36
  [81220f4b] blk_execute_rq_nowait+0x77/0x9e
  [81221018] blk_execute_rq+0xa6/0xde
  [8144f24b] ? printk+0x41/0x46
  [a00a21c5] ? get_rdac_req+0x81/0xe8 [scsi_dh_rdac]
  [a00a273a] send_mode_select+0x29f/0x489 [scsi_dh_rdac]
  [810c5d9b] ? probe_workqueue_execution+0xb1/0xce
  [81071e38] worker_thread+0x1a9/0x237
  [a00a249b] ? send_mode_select+0x0/0x489 [scsi_dh_rdac]
  [8107651b] ? autoremove_wake_function+0x0/0x39
  [81071c8f] ? worker_thread+0x0/0x237
  [81076222] kthread+0x7f/0x87
  [81012d2a] child_rip+0xa/0x20
  [810761a3] ? kthread+0x0/0x87
  [81012d20] ? child_rip+0x0/0x20
Signed-off-by: Maxim Uvarov maxim.uva...@oracle.com
---
  drivers/scsi/scsi_lib.c |3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..8e89ed9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1295,6 +1295,9 @@ int scsi_prep_fn(struct request_queue *q, struct request 
*req)
struct scsi_device *sdev = q-queuedata;
int ret = BLKPREP_KILL;

+   if (!sdev)
+   return ret;
+
if (req-cmd_type == REQ_TYPE_BLOCK_PC)
ret = scsi_setup_blk_pc_cmnd(sdev, req);
return scsi_prep_return(q, req, ret);


Sorry but this patch does not look like a proper fix to me. What you 
probably need is a scsi_device_get() call in scsi_dh_rdac.c somewhere 
before the queue_work(kmpath_rdacd, ctlr-ms_work) call and a 
scsi_device_put() call once send_mode_select() has finished using the sdev.


Bart.
--
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 v12 0/6] SCSI device removal fixes

[Bart Van Assche]

Fix a few issues related to SCSI device removal:
- Fix a race between starved list processing and device removal that
  can trigger a kernel oops.
- Avoid that __scsi_remove_device() is called twice for the same SCSI
  device, which also can cause a kernel oops.
- Restrict the SCSI device state changes allowed via sysfs.
- Avoid that invoking scsi_device_set_state() triggers a race.
- Avoid re-enabling I/O after the transport layer became offline.

Changes compared to v11:
- Left out a patch that was not a device removal bug fix.
- Left out the patches about which there is not yet an agreement.

Changes compared to v10:
- Rebased and retested on top of Linux kernel v3.10-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 v12 1/6] Fix race between starved list and device removal

[Bart Van Assche]

From: James Bottomley jbottom...@parallels.com

scsi_run_queue() examines all SCSI devices that are present on
the starved list. Since scsi_run_queue() unlocks the SCSI host
lock 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 holding a reference on the
queue while running it.

Signed-off-by: James Bottomley jbottom...@parallels.com
Signed-off-by: Bart Van Assche bvanass...@acm.org
Reported-by: Chanho Min chanho@lge.com
Reference: http://lkml.org/lkml/2012/8/2/96
Cc: Tejun Heo t...@kernel.org
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: sta...@vger.kernel.org
---
 drivers/scsi/scsi_lib.c |   26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..df8bd5a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -434,6 +434,8 @@ static void scsi_run_queue(struct request_queue *q)
list_splice_init(shost-starved_list, starved_list);
 
while (!list_empty(starved_list)) {
+   struct request_queue *slq;
+
/*
 * As long as shost is accepting commands and we have
 * starved queues, call blk_run_queue. scsi_request_fn
@@ -456,11 +458,25 @@ 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);
+   /*
+* Once we drop the host lock, a racing scsi_remove_device()
+* call may remove the sdev from the starved list and destroy
+* it and the queue.  Mitigate by taking a reference to the
+* queue and never touching the sdev again after we drop the
+* host lock.  Note: if __scsi_remove_device() invokes
+* blk_cleanup_queue() before the queue is run from this
+* function then blk_run_queue() will return immediately since
+* blk_cleanup_queue() marks the queue with QUEUE_FLAG_DYING.
+*/
+   slq = sdev-request_queue;
+   if (!blk_get_queue(slq))
+   continue;
+   spin_unlock_irqrestore(shost-host_lock, flags);
+
+   blk_run_queue(slq);
+   blk_put_queue(slq);
+
+   spin_lock_irqsave(shost-host_lock, flags);
}
/* put any unprocessed entries back */
list_splice(starved_list, shost-starved_list);
-- 
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 v12 2/6] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

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. If
this happens then the SCSI device has not yet been added to sysfs
(is_visible == 0).  Make sure that in that case the transition into
state SDEV_DEL occurs. 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 |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index df8bd5a..124392f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2193,6 +2193,7 @@ 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_CREATED_BLOCK:
break;
default:
goto illegal;
-- 
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 v12 3/6] Restrict device state changes allowed via sysfs

[Bart Van Assche]

Restrict the SCSI device state changes allowd via sysfs to the
OFFLINERUNNING transitions. Other transitions may confuse
the SCSI mid-layer. As an example, changing the state of a SCSI
device via sysfs into cancel or deleted prevents removal of
a SCSI device by scsi_remove_host().

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_sysfs.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 931a7d9..013c6de 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -605,7 +605,7 @@ store_state_field(struct device *dev, struct 
device_attribute *attr,
break;
}
}
-   if (!state)
+   if (state != SDEV_OFFLINE  state != SDEV_RUNNING)
return -EINVAL;
 
if (scsi_device_set_state(sdev, state))
-- 
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 v12 4/6] Avoid saving/restoring interrupt state inside scsi_remove_host()

[Bart Van Assche]

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
Reviewed-by: 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 v12 5/6] Avoid that scsi_device_set_state() triggers a race

[Bart Van Assche]

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 |   24 +++-
 4 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f43de1e..7006359 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1380,7 +1380,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 124392f..6a4fde7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2096,7 +2096,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)
@@ -2374,7 +2376,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;
 
@@ -2398,13 +2406,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);
@@ -2454,17 +2470,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
@@ -2499,13 +2517,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;
@@ -2517,7 +2538,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 v12 6/6] Avoid re-enabling I/O after the transport became offline

[Bart Van Assche]

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
Cc: 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 |4 +++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6a4fde7..63875c3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2180,7 +2180,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 dfbaa34..666b741 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -959,14 +959,16 @@ void __scsi_remove_device(struct scsi_device *sdev)
 {
struct Scsi_Host *shost = sdev-host;
struct device *dev = sdev-sdev_gendev;
+   enum scsi_device_state sdev_state;
int res;
 
if (sdev-is_visible) {
spin_lock_irq(shost-host_lock);
+   sdev_state = sdev-sdev_state;
res = scsi_device_set_state(sdev, SDEV_CANCEL);
spin_unlock_irq(shost-host_lock);
 
-   if (res != 0)
+   if (res != 0  sdev_state != SDEV_TRANSPORT_OFFLINE)
return;
 
bsg_unregister_queue(sdev-request_queue);
-- 
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 v2 0/15] IB SRP initiator patches for kernel 3.11

[Bart Van Assche]

The purpose of this InfiniBand SRP initiator patch series is as follows:
- Make the SRP initiator driver better suited for use in a H.A. setup.
  Speed up failover by reducing the IB RC retry count and by notifying
  multipathd faster about transport layer failures by adding
  fast_io_fail_tmo and dev_loss_tmo parameters.
- Make the SRP initiator better suited for use on NUMA systems by
  making the HCA completion vector configurable.

Changes since the v1 of the IB SRP initiator patches for kernel 3.11 
patch series:

- scsi_transport_srp: Allowed both fast_io_fail and dev_loss timeouts
  to be disabled.
- scsi_transport_srp, srp_reconnect_rport(): switched from
  scsi_block_requests() to scsi_target_block() for blocking SCSI command
  processing temporarily.
- scsi_transport_srp, srp_start_tl_fail_timers(): only block SCSI device
  command processing if the fast_io_fail timer is enabled.
- Changed srp_abort() such that upon transport offline the value
  FAST_IO_FAIL is returned instead of SUCCESS.
- Fixed a race condition in the maintain single connection patch: a
  new login after removal had started but before removal had finished
  still could create a duplicate connection. Fixed this by deferring
  removal from the target list until removal has finished.
- Modified the error message in the same patch for reporting that a
  duplicate connection has been rejected.
- Modified patch 2/15 such that all possible race conditions with
  srp_claim_req() are addressed.
- Documented the comp_vector and tl_retry_count login string parameters.
- Updated dev_loss_tmo and fast_io_fail_tmo documentation - mentioned
  off is a valid choice.

Changes compared to v5 of the Make ib_srp better suited for H.A. 
purposes patch series:

- Left out patches that are already upstream.
- Made it possible to set dev_loss_tmo to off. This is useful in a
  setup using initiator side mirroring to avoid that new /dev/sd* names
  are reassigned after a failover or cable pull and reinsert.
- Added kernel module parameters to ib_srp for configuring default
  values of the fast_io_fail_tmo and dev_loss_tmo parameters.
- Added a patch from Dotan Barak that fixes a kernel oops during rmmod
  triggered by resource allocation failure at module load time.
- Avoid duplicate connections by refusing relogins instead of dropping
  duplicate connections, as proposed by Sebastian Riemer.
- Added a patch from Sebastian Riemer for failing SCSI commands
  silently.
- Added a patch from Vu Pham to make the transport layer (IB RC) retry
  count configurable.
- Made HCA completion vector configurable.

Changes since v4:
- Added a patch for removing SCSI devices upon a port down event

Changes since v3:
- Restored the dev_loss_tmo and fast_io_fail_tmo sysfs attributes.
- Included a patch to fix an ib_srp crash that could be triggered by
  cable pulling.

Changes since v2:
- Addressed the v2 review comments.
- Dropped the patches that have already been merged.
- Dropped the patches for integration with multipathd.
- Dropped the micro-optimization of the IB completion handlers.

The individual patches in this series are as follows:
0001-IB-srp-Fix-remove_one-crash-due-to-resource-exhausti.patch
0002-IB-srp-Fix-race-between-srp_queuecommand-and-srp_cla.patch
0003-IB-srp-Avoid-that-srp_reset_host-is-skipped-after-a-.patch
0004-IB-srp-Fail-I-O-fast-if-target-offline.patch
0005-IB-srp-Skip-host-settle-delay.patch
0006-IB-srp-Maintain-a-single-connection-per-I_T-nexus.patch
0007-IB-srp-Keep-rport-as-long-as-the-IB-transport-layer.patch
0008-scsi_transport_srp-Add-transport-layer-error-handlin.patch
0009-IB-srp-Add-srp_terminate_io.patch
0010-IB-srp-Use-SRP-transport-layer-error-recovery.patch
0011-IB-srp-Start-timers-if-a-transport-layer-error-occur.patch
0012-IB-srp-Fail-SCSI-commands-silently.patch
0013-IB-srp-Make-HCA-completion-vector-configurable.patch
0014-IB-srp-Make-transport-layer-retry-count-configurable.patch
0015-IB-srp-Bump-driver-version-and-release-date.patch
--
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 v2 06/15] IB/srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

An SRP target is required to maintain a single connection between
initiator and target. This means that if the 'add_target' attribute
is used to create a second connection to a target that the first
connection will be logged out and that the SCSI error handler will
kick in. The SCSI error handler will cause the SRP initiator to
reconnect, which will cause I/O over the second connection to fail.
Avoid such ping-pong behavior by disabling relogins. Note: if
reconnecting manually is necessary, that is possible by deleting
and recreating an rport via sysfs.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Cc: Roland Dreier rol...@kernel.org
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
---
 drivers/infiniband/ulp/srp/ib_srp.c |   44 +--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 01212c9..4eba1f7 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -542,11 +542,11 @@ static void srp_remove_work(struct work_struct *work)
 
WARN_ON_ONCE(target-state != SRP_TARGET_REMOVED);
 
+   srp_remove_target(target);
+
spin_lock(target-srp_host-target_lock);
list_del(target-list);
spin_unlock(target-srp_host-target_lock);
-
-   srp_remove_target(target);
 }
 
 static void srp_rport_delete(struct srp_rport *rport)
@@ -2010,6 +2010,36 @@ static struct class srp_class = {
.dev_release = srp_release_dev
 };
 
+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */
+static bool srp_conn_unique(struct srp_host *host,
+   struct srp_target_port *target)
+{
+   struct srp_target_port *t;
+   bool ret = false;
+
+   if (target-state == SRP_TARGET_REMOVED)
+   goto out;
+
+   ret = true;
+
+   spin_lock(host-target_lock);
+   list_for_each_entry(t, host-target_list, list) {
+   if (t != target 
+   target-id_ext == t-id_ext 
+   target-ioc_guid == t-ioc_guid 
+   target-initiator_ext == t-initiator_ext) {
+   ret = false;
+   break;
+   }
+   }
+   spin_unlock(host-target_lock);
+
+out:
+   return ret;
+}
+
 /*
  * Target ports are added by writing
  *
@@ -2266,6 +2296,16 @@ static ssize_t srp_create_target(struct device *dev,
if (ret)
goto err;
 
+   if (!srp_conn_unique(target-srp_host, target)) {
+   shost_printk(KERN_INFO, target-scsi_host,
+PFX Already connected to target port with 
id_ext=%016llx;ioc_guid=%016llx;initiator_ext=%016llx\n,
+be64_to_cpu(target-id_ext),
+be64_to_cpu(target-ioc_guid),
+be64_to_cpu(target-initiator_ext));
+   ret = -EEXIST;
+   goto err;
+   }
+
if (!host-srp_dev-fmr_pool  !target-allow_ext_sg 
target-cmd_sg_cnt  target-sg_tablesize) {
pr_warn(No FMR pool and no external indirect descriptors, 
limiting sg_tablesize to cmd_sg_cnt\n);
-- 
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 v2 07/15] IB/srp: Keep rport as long as the IB transport layer

[Bart Van Assche]

Keep the rport data structure around after srp_remove_host() has
finished until cleanup of the IB transport layer has finished
completely. This is necessary because later patches use the rport
pointer inside the queuecommand callback. Without this patch
accessing the rport from inside a queuecommand callback is racy
because srp_remove_host() must be invoked before scsi_remove_host()
and because the queuecommand callback may get invoked after
srp_remove_host() has finished.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 drivers/infiniband/ulp/srp/ib_srp.c |3 +++
 drivers/infiniband/ulp/srp/ib_srp.h |1 +
 drivers/scsi/scsi_transport_srp.c   |   18 ++
 include/scsi/scsi_transport_srp.h   |2 ++
 4 files changed, 24 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 4eba1f7..cb86be5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -526,11 +526,13 @@ static void srp_remove_target(struct srp_target_port 
*target)
WARN_ON_ONCE(target-state != SRP_TARGET_REMOVED);
 
srp_del_scsi_host_attr(target-scsi_host);
+   srp_rport_get(target-rport);
srp_remove_host(target-scsi_host);
scsi_remove_host(target-scsi_host);
srp_disconnect_target(target);
ib_destroy_cm_id(target-cm_id);
srp_free_target_ib(target);
+   srp_rport_put(target-rport);
srp_free_req_data(target);
scsi_host_put(target-scsi_host);
 }
@@ -1984,6 +1986,7 @@ static int srp_add_target(struct srp_host *host, struct 
srp_target_port *target)
}
 
rport-lld_data = target;
+   target-rport = rport;
 
spin_lock(host-target_lock);
list_add_tail(target-list, host-target_list);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h 
b/drivers/infiniband/ulp/srp/ib_srp.h
index 66fbedd..1817ed5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -153,6 +153,7 @@ struct srp_target_port {
u16 io_class;
struct srp_host*srp_host;
struct Scsi_Host   *scsi_host;
+   struct srp_rport   *rport;
chartarget_name[32];
unsigned intscsi_id;
unsigned intsg_tablesize;
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f379c7f..f7ba94a 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -185,6 +185,24 @@ static int srp_host_match(struct attribute_container 
*cont, struct device *dev)
 }
 
 /**
+ * srp_rport_get() - increment rport reference count
+ */
+void srp_rport_get(struct srp_rport *rport)
+{
+   get_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_get);
+
+/**
+ * srp_rport_put() - decrement rport reference count
+ */
+void srp_rport_put(struct srp_rport *rport)
+{
+   put_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_put);
+
+/**
  * srp_rport_add - add a SRP remote port to the device hierarchy
  * @shost: scsi host the remote port is connected to.
  * @ids:   The port id for the remote port.
diff --git a/include/scsi/scsi_transport_srp.h 
b/include/scsi/scsi_transport_srp.h
index ff0f04a..5a2d2d1 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -38,6 +38,8 @@ extern struct scsi_transport_template *
 srp_attach_transport(struct srp_function_template *);
 extern void srp_release_transport(struct scsi_transport_template *);
 
+extern void srp_rport_get(struct srp_rport *rport);
+extern void srp_rport_put(struct srp_rport *rport);
 extern struct srp_rport *srp_rport_add(struct Scsi_Host *,
   struct srp_rport_identifiers *);
 extern void srp_rport_del(struct srp_rport *);
-- 
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 v2 08/15] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

Add the necessary functions in the SRP transport module to allow
an SRP initiator driver to implement transport layer error handling
similar to the functionality already provided by the FC transport
layer. This includes:
- Support for implementing fast_io_fail_tmo, the time that should
  elapse after having detected a transport layer problem and
  before failing I/O.
- Support for implementing dev_loss_tmo, the time that should
  elapse after having detected a transport layer problem and
  before removing a remote port.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 Documentation/ABI/stable/sysfs-transport-srp |   37 ++
 drivers/scsi/scsi_transport_srp.c|  477 +-
 include/scsi/scsi_transport_srp.h|   62 +++-
 3 files changed, 573 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp 
b/Documentation/ABI/stable/sysfs-transport-srp
index b36fb0d..6a4d651 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -5,6 +5,24 @@ Contact:   linux-scsi@vger.kernel.org, 
linux-r...@vger.kernel.org
 Description:   Instructs an SRP initiator to disconnect from a target and to
remove all LUNs imported from that target.
 
+What:  /sys/class/srp_remote_ports/port-h:n/dev_loss_tmo
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before removing a target port.
+   Zero means immediate removal. Setting this attribute to off
+   will disable this behavior.
+
+What:  /sys/class/srp_remote_ports/port-h:n/fast_io_fail_tmo
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before failing I/O. Zero means
+   immediate removal. Setting this attribute to off will
+   disable this behavior.
+
 What:  /sys/class/srp_remote_ports/port-h:n/port_id
 Date:  June 27, 2007
 KernelVersion: 2.6.24
@@ -12,8 +30,27 @@ Contact: linux-scsi@vger.kernel.org
 Description:   16-byte local SRP port identifier in hexadecimal format. An
example: 4c:49:4e:55:58:20:56:49:4f:00:00:00:00:00:00:00.
 
+What:  /sys/class/srp_remote_ports/port-h:n/reconnect_delay
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a reconnect
+   attempt failed before retrying.
+
 What:  /sys/class/srp_remote_ports/port-h:n/roles
 Date:  June 27, 2007
 KernelVersion: 2.6.24
 Contact:   linux-scsi@vger.kernel.org
 Description:   Role of the remote port. Either SRP Initiator or SRP Target.
+
+What:  /sys/class/srp_remote_ports/port-h:n/state
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   State of the transport layer to the remote port. running if
+   the transport layer is operational; blocked if a transport
+   layer error has been encountered but the fail_io_fast_tmo
+   timer has not yet fired; fail-fast after the
+   fail_io_fast_tmo timer has fired and before the dev_loss_tmo
+   timer has fired; lost after the dev_loss_tmo timer has
+   fired and before the port is finally removed.
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f7ba94a..44b27dd 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -24,11 +24,13 @@
 #include linux/err.h
 #include linux/slab.h
 #include linux/string.h
+#include linux/delay.h
 
 #include scsi/scsi.h
 #include scsi/scsi_device.h
 #include scsi/scsi_host.h
 #include scsi/scsi_transport.h
+#include scsi/scsi_cmnd.h
 #include scsi/scsi_transport_srp.h
 #include scsi_transport_srp_internal.h
 
@@ -38,7 +40,7 @@ struct srp_host_attrs {
 #define to_srp_host_attrs(host)((struct srp_host_attrs 
*)(host)-shost_data)
 
 #define SRP_HOST_ATTRS 0
-#define SRP_RPORT_ATTRS 3
+#define SRP_RPORT_ATTRS 8
 
 struct srp_internal {
struct scsi_transport_template t;
@@ -54,6 +56,25 @@ struct srp_internal {
 
 #definedev_to_rport(d) container_of(d, struct srp_rport, dev)
 #define transport_class_to_srp_rport(dev) dev_to_rport((dev)-parent)
+static inline struct

[PATCH v2 12/15] IB/srp: Fail SCSI commands silently

[Bart Van Assche]

From: Sebastian Riemer sebastian.rie...@profitbricks.com

Avoid that path failover in a multipath setup causes the SCSI layer
to generate kernel messages about SCSI command failures. This patch
speeds up SRP initiator operation significantly when monitoring
kernel messages over a serial port.

[bvanassche: Changed patch description]
Signed-off-by: Sebastian Riemer sebastian.rie...@profitbricks.com
Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
---
 drivers/infiniband/ulp/srp/ib_srp.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index a8cc427..e77176e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -757,6 +757,7 @@ static void srp_finish_req(struct srp_target_port *target,
 
if (scmnd) {
srp_free_req(target, req, scmnd, 0);
+   scmnd-request-cmd_flags |= REQ_QUIET;
scmnd-result = result;
scmnd-scsi_done(scmnd);
}
@@ -1438,6 +1439,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, 
struct scsi_cmnd *scmnd)
 
result = srp_chkready(target-rport);
if (unlikely(result)) {
+   scmnd-request-cmd_flags |= REQ_QUIET;
scmnd-result = result;
scmnd-scsi_done(scmnd);
return 0;
@@ -1843,6 +1845,7 @@ static int srp_abort(struct scsi_cmnd *scmnd)
else
ret = FAILED;
srp_free_req(target, req, scmnd, 0);
+   scmnd-request-cmd_flags |= REQ_QUIET;
scmnd-result = DID_ABORT  16;
scmnd-scsi_done(scmnd);
 
-- 
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 v2 08/15] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 06/30/13 23:05, David Dillow wrote:

On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+   return (fast_io_fail_tmo  0 || dev_loss_tmo  0 ||
+   fast_io_fail_tmo  dev_loss_tmo) 
+   fast_io_fail_tmo  LONG_MAX / HZ 
+   dev_loss_tmo  LONG_MAX / HZ ? 0 : -EINVAL;
+}


They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
LONG_MAX / HZ, I think.


The fast_io_fail_tmo should indeed be capped by that value. However, I'm 
not sure about dev_loss_tmo. I think there are several use cases (e.g. 
initiator-side mirroring) where it's useful to set dev_loss_tmo to a 
larger value than ten minutes.



+static ssize_t store_srp_rport_fast_io_fail_tmo(struct device *dev,
+   struct device_attribute *attr,
+   const char *buf, size_t count)
+{
+   struct srp_rport *rport = transport_class_to_srp_rport(dev);
+   char ch[16], *p;
+   int res;
+   int fast_io_fail_tmo;
+
+   sprintf(ch, %.*s, min_t(int, sizeof(ch) - 1, count), buf);
+   p = strchr(ch, '\n');
+   if (p)
+   *p = '\0';


Again, no need for the sprintf if you don't modify the buffer? Instead
of using strchr() to make the strcmp() work with newlines, just do

if (!strcmp(buf, off) || !strcmp(buf, off\n)) {
fast_io_fail_tmo = 1;
} else {
res = kstrtoint(buf, 0, fast_io_fail_tmo);
...

instead?

Same comment applies for store_srp_rport_dev_loss_tmo().


OK, will remove the temporary char arrays.

Bart.

--
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 v12 2/6] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

On 07/01/13 09:05, James Bottomley wrote:


On Thu, 2013-06-27 at 16:53 +0200, Bart Van Assche wrote:

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. If
this happens then the SCSI device has not yet been added to sysfs
(is_visible == 0).  Make sure that in that case the transition into
state SDEV_DEL occurs. This avoids that __scsi_remove_device() gets
invoked a second time by scsi_forget_host().


The patch summary of this one isn't true.  How about enable destruction
of blocked devices which fail LUN scanning


Hello James,

Do you want me to repost the patch series or is this something you can 
fix up ?


Thanks,

Bart.

--
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 v12 6/6] Avoid re-enabling I/O after the transport became offline

[Bart Van Assche]

On 07/01/13 10:27, Hannes Reinecke wrote:
 On 06/27/2013 04:57 PM, Bart Van Assche wrote:
 diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
 index dfbaa34..666b741 100644
 --- a/drivers/scsi/scsi_sysfs.c
 +++ b/drivers/scsi/scsi_sysfs.c
 @@ -959,14 +959,16 @@ void __scsi_remove_device(struct scsi_device *sdev)
   {
  struct Scsi_Host *shost = sdev-host;
  struct device *dev = sdev-sdev_gendev;
 +enum scsi_device_state sdev_state;
  int res;
   
  if (sdev-is_visible) {
  spin_lock_irq(shost-host_lock);
 +sdev_state = sdev-sdev_state;
  res = scsi_device_set_state(sdev, SDEV_CANCEL);
  spin_unlock_irq(shost-host_lock);
   
 -if (res != 0)
 +if (res != 0  sdev_state != SDEV_TRANSPORT_OFFLINE)
  return;
   
  bsg_unregister_queue(sdev-request_queue);

 Hmm. This is really subtle. Do you mind adding inserting a comment
 here on why this is required?

How about inserting the following comment just above the last if-statement
in the code cited above ?

/*
 * The transition from SDEV_TRANSPORT_OFFLINE into SDEV_CANCEL
 * is not allowed since this transition would re-enable I/O. If
 * the device state was already SDEV_TRANSPORT_OFFLINE,
 * proceed with device removal.
 */

Bart.
--
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 v12 5/6] Avoid that scsi_device_set_state() triggers a race

[Bart Van Assche]

On 07/01/13 16:49, James Bottomley wrote:

On Thu, 2013-06-27 at 16:56 +0200, Bart Van Assche wrote:

Make concurrent invocations of scsi_device_set_state() safe.


Firstly, I don't understand from this where you think the races are.
Secondly, shouldn't this be the device lock? and thirdly, if we accept
that locking is required, encapsulate it in the function: Having the
callers manage locking is asking for trouble.  The latter may require a
new lock for the state to avoid entanglement.


Today there is no guarantee that scsi_device_set_state() calls are 
serialized, so two scsi_device_set_state() invocations may be in 
progress concurrently. It is e.g. possible that both calls report 
device state has been changed successfully to their callers although 
only one of these two state changes will be effective due to the race.


At the time I wrote this patch I think there was a caller that invoked 
scsi_device_set_state() with the host lock held. Hence my choice for the 
host lock. However, I can't find that caller anymore. So the suggestion 
to use the device lock instead makes sense to me. I'll double check 
whether there are no callers of that function that already hold the 
device lock.


Bart.
--
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 v13 0/4] SCSI device removal fixes

[Bart Van Assche]

Fix a few issues related to SCSI device removal:
- Fix a race between starved list processing and device removal that
  can trigger a kernel oops.
- Avoid that __scsi_remove_device() is called twice for the same SCSI
  device, which also can cause a kernel oops.
- Restrict the SCSI device state changes allowed via sysfs.
- Avoid re-enabling I/O after the transport layer became offline.

Changes compared to v12:
- Clarified the description of the patch for handling a transport
  layer failure during LUN scanning: mentioned that this patch was
  developed after analyzing the cause of a kernel oops triggered by
  asynchronous LUN scanning.
- Restored the previous version of the patch for restricting sysfs
  SCSI device state changes, namely the version that only disallows
  changing the device state into cancel or deleted.
- Added a comment in patch 4/4.
- Left out a patch that was the result of source reading and also
  an intermediate patch that is no longer needed in this series.

Changes compared to v11:
- Left out a patch that was not a device removal bug fix.
- Left out the patches about which there is not yet an agreement.

Changes compared to v10:
- Rebased and retested on top of Linux kernel v3.10-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 1/4] Fix race between starved list and device removal

[Bart Van Assche]

From: James Bottomley jbottom...@parallels.com

scsi_run_queue() examines all SCSI devices that are present on
the starved list. Since scsi_run_queue() unlocks the SCSI host
lock 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 holding a reference on the
queue while running it.

Signed-off-by: James Bottomley jbottom...@parallels.com
Signed-off-by: Bart Van Assche bvanass...@acm.org
Reported-by: Chanho Min chanho@lge.com
Reference: http://lkml.org/lkml/2012/8/2/96
Cc: Tejun Heo t...@kernel.org
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: sta...@vger.kernel.org
---
 drivers/scsi/scsi_lib.c |   26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..df8bd5a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -434,6 +434,8 @@ static void scsi_run_queue(struct request_queue *q)
list_splice_init(shost-starved_list, starved_list);
 
while (!list_empty(starved_list)) {
+   struct request_queue *slq;
+
/*
 * As long as shost is accepting commands and we have
 * starved queues, call blk_run_queue. scsi_request_fn
@@ -456,11 +458,25 @@ 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);
+   /*
+* Once we drop the host lock, a racing scsi_remove_device()
+* call may remove the sdev from the starved list and destroy
+* it and the queue.  Mitigate by taking a reference to the
+* queue and never touching the sdev again after we drop the
+* host lock.  Note: if __scsi_remove_device() invokes
+* blk_cleanup_queue() before the queue is run from this
+* function then blk_run_queue() will return immediately since
+* blk_cleanup_queue() marks the queue with QUEUE_FLAG_DYING.
+*/
+   slq = sdev-request_queue;
+   if (!blk_get_queue(slq))
+   continue;
+   spin_unlock_irqrestore(shost-host_lock, flags);
+
+   blk_run_queue(slq);
+   blk_put_queue(slq);
+
+   spin_lock_irqsave(shost-host_lock, flags);
}
/* put any unprocessed entries back */
list_splice(starved_list, shost-starved_list);
-- 
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 2/4] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

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 and
before the SCSI device has been added to sysfs (is_visible == 0).
Make sure that even in this case the transition into state SDEV_DEL
occurs. This avoids that __scsi_remove_device() can get invoked a
second time by scsi_forget_host() if this last function is invoked
from another thread than the thread that performs LUN scanning.

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 |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index df8bd5a..124392f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2193,6 +2193,7 @@ 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_CREATED_BLOCK:
break;
default:
goto illegal;
-- 
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 3/4] Avoid re-enabling I/O after the transport became offline

[Bart Van Assche]

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
Cc: 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 |9 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 124392f..a0fb56b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2178,7 +2178,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 931a7d9..1711617 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -955,7 +955,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
struct device *dev = sdev-sdev_gendev;
 
if (sdev-is_visible) {
-   if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+   /*
+* The transition from SDEV_TRANSPORT_OFFLINE into
+* SDEV_CANCEL is not allowed since this transition would
+* reenable I/O. However, if the device state was already
+* SDEV_TRANSPORT_OFFLINE, proceed with device removal.
+*/
+   if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0 
+   sdev-sdev_state != SDEV_TRANSPORT_OFFLINE)
return;
 
bsg_unregister_queue(sdev-request_queue);
-- 
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 4/4] Disallow changing the device state via sysfs into deleted

[Bart Van Assche]

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.

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_sysfs.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 1711617..292df85 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -605,10 +605,8 @@ store_state_field(struct device *dev, struct 
device_attribute *attr,
break;
}
}
-   if (!state)
-   return -EINVAL;
-
-   if (scsi_device_set_state(sdev, state))
+   if (state == 0 || state == SDEV_CANCEL || state == SDEV_DEL ||
+   scsi_device_set_state(sdev, state))
return -EINVAL;
return count;
 }
-- 
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 v3 0/13] IB SRP initiator patches for kernel 3.11

[Bart Van Assche]

The purpose of this InfiniBand SRP initiator patch series is as follows:
- Make the SRP initiator driver better suited for use in a H.A. setup.
  Add fast_io_fail_tmo and dev_loss_tmo parameters. These can be used
  either to speed up failover or to avoid device removal when e.g. using
  initiator side mirroring.
- Make the SRP initiator better suited for use on NUMA systems by
  making the HCA completion vector configurable.

Changes since the v2 of the IB SRP initiator patches for kernel 3.11 
patch series:

- Improved documentation of the newly added sysfs parameters.
- Limit fast_io_fail_tmo to SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
- Simplified the code for parsing values written into sysfs attributes.
- Fixed a potential deadlock in the code added in scsi_transport_srp
  (invoking cancel_delayed_work() with the rport mutex held for work
   that needs the rport mutex itself).
- Changed the default retry count back from 2 to 7 since there is not
  yet agreement about this change.
- Dropped the patch that silences failed SCSI commands and also the
  patch that fixes a race between srp_queuecommand() and
  srp_claim_req() since there is no agreement about these patches.

Changes since the v1 of the IB SRP initiator patches for kernel 3.11 
patch series:

- scsi_transport_srp: Allowed both fast_io_fail and dev_loss timeouts
  to be disabled.
- scsi_transport_srp, srp_reconnect_rport(): switched from
  scsi_block_requests() to scsi_target_block() for blocking SCSI command
  processing temporarily.
- scsi_transport_srp, srp_start_tl_fail_timers(): only block SCSI device
  command processing if the fast_io_fail timer is enabled.
- Changed srp_abort() such that upon transport offline the value
  FAST_IO_FAIL is returned instead of SUCCESS.
- Fixed a race condition in the maintain single connection patch: a
  new login after removal had started but before removal had finished
  still could create a duplicate connection. Fixed this by deferring
  removal from the target list until removal has finished.
- Modified the error message in the same patch for reporting that a
  duplicate connection has been rejected.
- Modified patch 2/15 such that all possible race conditions with
  srp_claim_req() are addressed.
- Documented the comp_vector and tl_retry_count login string parameters.
- Updated dev_loss_tmo and fast_io_fail_tmo documentation - mentioned
  off is a valid choice.

Changes compared to v5 of the Make ib_srp better suited for H.A. 
purposes patch series:

- Left out patches that are already upstream.
- Made it possible to set dev_loss_tmo to off. This is useful in a
  setup using initiator side mirroring to avoid that new /dev/sd* names
  are reassigned after a failover or cable pull and reinsert.
- Added kernel module parameters to ib_srp for configuring default
  values of the fast_io_fail_tmo and dev_loss_tmo parameters.
- Added a patch from Dotan Barak that fixes a kernel oops during rmmod
  triggered by resource allocation failure at module load time.
- Avoid duplicate connections by refusing relogins instead of dropping
  duplicate connections, as proposed by Sebastian Riemer.
- Added a patch from Sebastian Riemer for failing SCSI commands
  silently.
- Added a patch from Vu Pham to make the transport layer (IB RC) retry
  count configurable.
- Made HCA completion vector configurable.

Changes since v4:
- Added a patch for removing SCSI devices upon a port down event

Changes since v3:
- Restored the dev_loss_tmo and fast_io_fail_tmo sysfs attributes.
- Included a patch to fix an ib_srp crash that could be triggered by
  cable pulling.

Changes since v2:
- Addressed the v2 review comments.
- Dropped the patches that have already been merged.
- Dropped the patches for integration with multipathd.
- Dropped the micro-optimization of the IB completion handlers.

The individual patches in this series are as follows:
0001-IB-srp-Fix-remove_one-crash-due-to-resource-exhausti.patch
0002-IB-srp-Fix-race-between-srp_queuecommand-and-srp_cla.patch
0003-IB-srp-Avoid-that-srp_reset_host-is-skipped-after-a-.patch
0004-IB-srp-Fail-I-O-fast-if-target-offline.patch
0005-IB-srp-Skip-host-settle-delay.patch
0006-IB-srp-Maintain-a-single-connection-per-I_T-nexus.patch
0007-IB-srp-Keep-rport-as-long-as-the-IB-transport-layer.patch
0008-scsi_transport_srp-Add-transport-layer-error-handlin.patch
0009-IB-srp-Add-srp_terminate_io.patch
0010-IB-srp-Use-SRP-transport-layer-error-recovery.patch
0011-IB-srp-Start-timers-if-a-transport-layer-error-occur.patch
0012-IB-srp-Fail-SCSI-commands-silently.patch
0013-IB-srp-Make-HCA-completion-vector-configurable.patch
0014-IB-srp-Make-transport-layer-retry-count-configurable.patch
0015-IB-srp-Bump-driver-version-and-release-date.patch
--
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 v3 06/13] IB/srp: Keep rport as long as the IB transport layer

[Bart Van Assche]

Keep the rport data structure around after srp_remove_host() has
finished until cleanup of the IB transport layer has finished
completely. This is necessary because later patches use the rport
pointer inside the queuecommand callback. Without this patch
accessing the rport from inside a queuecommand callback is racy
because srp_remove_host() must be invoked before scsi_remove_host()
and because the queuecommand callback may get invoked after
srp_remove_host() has finished.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 drivers/infiniband/ulp/srp/ib_srp.c |3 +++
 drivers/infiniband/ulp/srp/ib_srp.h |1 +
 drivers/scsi/scsi_transport_srp.c   |   18 ++
 include/scsi/scsi_transport_srp.h   |2 ++
 4 files changed, 24 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index f046e32..f65701d 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -526,11 +526,13 @@ static void srp_remove_target(struct srp_target_port 
*target)
WARN_ON_ONCE(target-state != SRP_TARGET_REMOVED);
 
srp_del_scsi_host_attr(target-scsi_host);
+   srp_rport_get(target-rport);
srp_remove_host(target-scsi_host);
scsi_remove_host(target-scsi_host);
srp_disconnect_target(target);
ib_destroy_cm_id(target-cm_id);
srp_free_target_ib(target);
+   srp_rport_put(target-rport);
srp_free_req_data(target);
scsi_host_put(target-scsi_host);
 }
@@ -1982,6 +1984,7 @@ static int srp_add_target(struct srp_host *host, struct 
srp_target_port *target)
}
 
rport-lld_data = target;
+   target-rport = rport;
 
spin_lock(host-target_lock);
list_add_tail(target-list, host-target_list);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h 
b/drivers/infiniband/ulp/srp/ib_srp.h
index 66fbedd..1817ed5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -153,6 +153,7 @@ struct srp_target_port {
u16 io_class;
struct srp_host*srp_host;
struct Scsi_Host   *scsi_host;
+   struct srp_rport   *rport;
chartarget_name[32];
unsigned intscsi_id;
unsigned intsg_tablesize;
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f379c7f..f7ba94a 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -185,6 +185,24 @@ static int srp_host_match(struct attribute_container 
*cont, struct device *dev)
 }
 
 /**
+ * srp_rport_get() - increment rport reference count
+ */
+void srp_rport_get(struct srp_rport *rport)
+{
+   get_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_get);
+
+/**
+ * srp_rport_put() - decrement rport reference count
+ */
+void srp_rport_put(struct srp_rport *rport)
+{
+   put_device(rport-dev);
+}
+EXPORT_SYMBOL(srp_rport_put);
+
+/**
  * srp_rport_add - add a SRP remote port to the device hierarchy
  * @shost: scsi host the remote port is connected to.
  * @ids:   The port id for the remote port.
diff --git a/include/scsi/scsi_transport_srp.h 
b/include/scsi/scsi_transport_srp.h
index ff0f04a..5a2d2d1 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -38,6 +38,8 @@ extern struct scsi_transport_template *
 srp_attach_transport(struct srp_function_template *);
 extern void srp_release_transport(struct scsi_transport_template *);
 
+extern void srp_rport_get(struct srp_rport *rport);
+extern void srp_rport_put(struct srp_rport *rport);
 extern struct srp_rport *srp_rport_add(struct Scsi_Host *,
   struct srp_rport_identifiers *);
 extern void srp_rport_del(struct srp_rport *);
-- 
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 v3 07/13] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

Add the necessary functions in the SRP transport module to allow
an SRP initiator driver to implement transport layer error handling
similar to the functionality already provided by the FC transport
layer. This includes:
- Support for implementing fast_io_fail_tmo, the time that should
  elapse after having detected a transport layer problem and
  before failing I/O.
- Support for implementing dev_loss_tmo, the time that should
  elapse after having detected a transport layer problem and
  before removing a remote port.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 Documentation/ABI/stable/sysfs-transport-srp |   38 +++
 drivers/scsi/scsi_transport_srp.c|  468 +-
 include/scsi/scsi_transport_srp.h|   62 +++-
 3 files changed, 565 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp 
b/Documentation/ABI/stable/sysfs-transport-srp
index b36fb0d..52babb9 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -5,6 +5,24 @@ Contact:   linux-scsi@vger.kernel.org, 
linux-r...@vger.kernel.org
 Description:   Instructs an SRP initiator to disconnect from a target and to
remove all LUNs imported from that target.
 
+What:  /sys/class/srp_remote_ports/port-h:n/dev_loss_tmo
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before removing a target port.
+   Zero means immediate removal. Setting this attribute to off
+   will disable this behavior.
+
+What:  /sys/class/srp_remote_ports/port-h:n/fast_io_fail_tmo
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a transport
+   layer error has been observed before failing I/O. Zero means
+   failing I/O immediately. Setting this attribute to off will
+   disable this behavior.
+
 What:  /sys/class/srp_remote_ports/port-h:n/port_id
 Date:  June 27, 2007
 KernelVersion: 2.6.24
@@ -12,8 +30,28 @@ Contact: linux-scsi@vger.kernel.org
 Description:   16-byte local SRP port identifier in hexadecimal format. An
example: 4c:49:4e:55:58:20:56:49:4f:00:00:00:00:00:00:00.
 
+What:  /sys/class/srp_remote_ports/port-h:n/reconnect_delay
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a reconnect
+   attempt failed before retrying.
+
 What:  /sys/class/srp_remote_ports/port-h:n/roles
 Date:  June 27, 2007
 KernelVersion: 2.6.24
 Contact:   linux-scsi@vger.kernel.org
 Description:   Role of the remote port. Either SRP Initiator or SRP Target.
+
+What:  /sys/class/srp_remote_ports/port-h:n/state
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   State of the transport layer used for communication with the
+   remote port. running if the transport layer is operational;
+   blocked if a transport layer error has been encountered but
+   the fail_io_fast_tmo timer has not yet fired; fail-fast
+   after the fail_io_fast_tmo timer has fired and before the
+   dev_loss_tmo timer has fired; lost after the
+   dev_loss_tmo timer has fired and before the port is finally
+   removed.
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f7ba94a..1b9ebd5 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -24,12 +24,15 @@
 #include linux/err.h
 #include linux/slab.h
 #include linux/string.h
+#include linux/delay.h
 
 #include scsi/scsi.h
+#include scsi/scsi_cmnd.h
 #include scsi/scsi_device.h
 #include scsi/scsi_host.h
 #include scsi/scsi_transport.h
 #include scsi/scsi_transport_srp.h
+#include scsi_priv.h
 #include scsi_transport_srp_internal.h
 
 struct srp_host_attrs {
@@ -38,7 +41,7 @@ struct srp_host_attrs {
 #define to_srp_host_attrs(host)((struct srp_host_attrs 
*)(host)-shost_data)
 
 #define SRP_HOST_ATTRS 0
-#define SRP_RPORT_ATTRS 3
+#define SRP_RPORT_ATTRS 8
 
 struct srp_internal {
struct scsi_transport_template t;
@@ -54,6 +57,26 @@ struct srp_internal {
 
 #definedev_to_rport(d) container_of(d, struct srp_rport, dev

Re: [PATCH v3 0/13] IB SRP initiator patches for kernel 3.11

[Bart Van Assche]

On 07/03/13 15:38, Or Gerlitz wrote:

Some of these patches were already picked by Roland (SB), I would
suggest that you post V4 and drop the ones which were accepted.


One of the patches that is already in Roland's tree and that was in v1 
of this series has been split into two patches in v2 and v3 of this 
series. So I'd like to hear from Roland what he prefers himself - that I 
drop the patches that are already in his tree or that Roland updates his 
tree with the most recently posted patches.


Bart.
--
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 v3 07/13] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 07/03/13 19:27, David Dillow wrote:

On Wed, 2013-07-03 at 18:00 +0200, Bart Van Assche wrote:

The combination of dev_loss_tmo off and reconnect_delay  0 worked fine
in my tests. An I/O failure was detected shortly after the cable to the
target was pulled. I/O resumed shortly after the cable to the target was
reinserted.


Perhaps I don't understand your answer -- I'm asking about dev_loss_tmo
 0, and fast_io_fail_tmo = 0. The other transports do not allow this
scenario, and I'm asking if it makes sense for SRP to allow it.

But now that you mention reconnect_delay, what is the meaning of that
when it is negative? That's not in the documentation. And should it be
considered in srp_tmo_valid() -- are there values of reconnect_delay
that cause problems?


None of the combinations that can be configured from user space can 
bring the kernel in trouble. If reconnect_delay = 0 that means that the 
time-based reconnect mechanism is disabled.



I'm starting to get a bit concerned about this patch -- can you, Vu, and
Sebastian comment on the testing you have done?


All combinations of reconnect_delay, fast_io_fail_tmo and dev_loss_tmo 
that result in different behavior have been tested.



Also, FC caps dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT if
fail_io_fast_tmo is off; I agree with your reasoning about leaving it
unlimited if fast fail is on, but does that still hold if it is off?


I think setting dev_loss_tmo to a large value only makes sense if the
value of reconnect_delay is not too large. Setting both to a large value
would result in slow recovery after a transport layer failure has been
corrected.


So you agree it should be capped? I can't tell from your response.


Not all combinations of reconnect_delay / fail_io_fast_tmo / 
dev_loss_tmo result in useful behavior. It is up to the user to choose a 
meaningful combination.


Bart.
--
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 v3 07/13] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 07/03/13 20:57, David Dillow wrote:

And I'm getting the strong sense that the answer to my question about
fast_io_fail_tmo = 0 when dev_loss_tmo is that we should not allow that
combination, even if it doesn't break the kernel. If it doesn't make
sense, there is no reason to create an opportunity for user confusion.


Let's take a step back. I think we agree that the only combinations of 
timeout parameters that are useful are those combinations that guarantee 
that SCSI commands will be finished in a reasonable time and also that 
allow multipath to detect failed paths. The first requirement comes down 
to limiting the value fast_io_fail_tmo can be set to. The second 
requirement means that either reconnect_delay or fast_io_fail_tmo must 
be set (please note that a reconnect failure changes the state of all 
involved SCSI devices into SDEV_TRANSPORT_OFFLINE). So how about 
modifying srp_tmo_valid() as follows:

* Add an argument called reconnect_delay.
* Add the following code in that function:
if (reconnect_delay  0  fast_io_fail_tmo  0  dev_loss_tmo  0)
return -EINVAL;

Bart.
--
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 v3 07/13] scsi_transport_srp: Add transport layer error handling

[Bart Van Assche]

On 07/04/13 10:01, Bart Van Assche wrote:

On 07/03/13 20:57, David Dillow wrote:

And I'm getting the strong sense that the answer to my question about
fast_io_fail_tmo = 0 when dev_loss_tmo is that we should not allow that
combination, even if it doesn't break the kernel. If it doesn't make
sense, there is no reason to create an opportunity for user confusion.


Let's take a step back. I think we agree that the only combinations of
timeout parameters that are useful are those combinations that guarantee
that SCSI commands will be finished in a reasonable time and also that
allow multipath to detect failed paths. The first requirement comes down
to limiting the value fast_io_fail_tmo can be set to. The second
requirement means that either reconnect_delay or fast_io_fail_tmo must
be set (please note that a reconnect failure changes the state of all
involved SCSI devices into SDEV_TRANSPORT_OFFLINE). So how about
modifying srp_tmo_valid() as follows:
* Add an argument called reconnect_delay.
* Add the following code in that function:
 if (reconnect_delay  0  fast_io_fail_tmo  0  dev_loss_tmo  0)
 return -EINVAL;


(replying to my own e-mail)

A small correction to what I wrote above: a reconnect failure only 
causes the SCSI device state to be changed into SDEV_TRANSPORT_OFFLINE 
if the fast_io_fail mechanism has been disabled.


Bart.

--
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 v14 0/6] SCSI device removal fixes

[Bart Van Assche]

Fix a few issues related to SCSI device removal:
- Fix a race between starved list processing and device removal that
  can trigger a kernel oops.
- Avoid that __scsi_remove_device() is called twice for the same SCSI
  device, which also can cause a kernel oops.
- Restrict the SCSI device state changes allowed via sysfs.
- Avoid re-enabling I/O after the transport layer became offline.

Changes compared to v13:
- Reworked the patch for avoiding to re-enable I/O while removing an
  offline device by introducing a new SCSI device state
  (SDEV_CANCEL_OFFLINE).

Changes compared to v12:
- Clarified the description of the patch for handling a transport
  layer failure during LUN scanning: mentioned that this patch was
  developed after analyzing the cause of a kernel oops triggered by
  asynchronous LUN scanning.
- Restored the previous version of the patch for restricting sysfs
  SCSI device state changes, namely the version that only disallows
  changing the device state into cancel or deleted.
- Added a comment in patch 4/4.
- Left out a patch that was the result of source reading and also
  an intermediate patch that is no longer needed in this series.

Changes compared to v11:
- Left out a patch that was not a device removal bug fix.
- Left out the patches about which there is not yet an agreement.

Changes compared to v10:
- Rebased and retested on top of Linux kernel v3.10-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 v14 2/6] Avoid calling __scsi_remove_device() twice

[Bart Van Assche]

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 and
before the SCSI device has been added to sysfs (is_visible == 0).
Make sure that even in this case the transition into state SDEV_DEL
occurs. This avoids that __scsi_remove_device() can get invoked a
second time by scsi_forget_host() if this last function is invoked
from another thread than the thread that performs LUN scanning.

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
---
 drivers/scsi/scsi_lib.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index df8bd5a..124392f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2193,6 +2193,7 @@ 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_CREATED_BLOCK:
break;
default:
goto illegal;
-- 
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 v14 1/6] Fix race between starved list and device removal

[Bart Van Assche]

scsi_run_queue() examines all SCSI devices that are present on
the starved list. Since scsi_run_queue() unlocks the SCSI host
lock 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 holding a reference on the
queue while running it.

Signed-off-by: James Bottomley jbottom...@parallels.com
Signed-off-by: Bart Van Assche bvanass...@acm.org
Reported-by: Chanho Min chanho@lge.com
Reference: http://lkml.org/lkml/2012/8/2/96
Cc: Mike Christie micha...@cs.wisc.edu
Cc: Hannes Reinecke h...@suse.de
Cc: sta...@vger.kernel.org
---
 drivers/scsi/scsi_lib.c |   26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..df8bd5a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -434,6 +434,8 @@ static void scsi_run_queue(struct request_queue *q)
list_splice_init(shost-starved_list, starved_list);
 
while (!list_empty(starved_list)) {
+   struct request_queue *slq;
+
/*
 * As long as shost is accepting commands and we have
 * starved queues, call blk_run_queue. scsi_request_fn
@@ -456,11 +458,25 @@ 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);
+   /*
+* Once we drop the host lock, a racing scsi_remove_device()
+* call may remove the sdev from the starved list and destroy
+* it and the queue.  Mitigate by taking a reference to the
+* queue and never touching the sdev again after we drop the
+* host lock.  Note: if __scsi_remove_device() invokes
+* blk_cleanup_queue() before the queue is run from this
+* function then blk_run_queue() will return immediately since
+* blk_cleanup_queue() marks the queue with QUEUE_FLAG_DYING.
+*/
+   slq = sdev-request_queue;
+   if (!blk_get_queue(slq))
+   continue;
+   spin_unlock_irqrestore(shost-host_lock, flags);
+
+   blk_run_queue(slq);
+   blk_put_queue(slq);
+
+   spin_lock_irqsave(shost-host_lock, flags);
}
/* put any unprocessed entries back */
list_splice(starved_list, shost-starved_list);
-- 
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 v14 3/6] Introduce scsi_device_being_removed()

[Bart Van Assche]

Introduce the helper function scsi_device_being_removed(). This patch
does not change any functionality.

Signed-off-by: Bart Van Assche bvanass...@acm.org
Acked-by: Hannes Reinecke h...@suse.de
Cc: James Bottomley jbottom...@parallels.com
Cc: Mike Christie micha...@cs.wisc.edu
---
 drivers/scsi/device_handler/scsi_dh.c |7 ++-
 include/scsi/scsi_device.h|5 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c 
b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..78b3ddb 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -156,8 +156,7 @@ store_dh_state(struct device *dev, struct device_attribute 
*attr,
struct scsi_device_handler *scsi_dh;
int err = -EINVAL;
 
-   if (sdev-sdev_state == SDEV_CANCEL ||
-   sdev-sdev_state == SDEV_DEL)
+   if (scsi_device_being_removed(sdev))
return -ENODEV;
 
if (!sdev-scsi_dh_data) {
@@ -400,9 +399,7 @@ int scsi_dh_activate(struct request_queue *q, 
activate_complete fn, void *data)
if (sdev-scsi_dh_data)
scsi_dh = sdev-scsi_dh_data-scsi_dh;
dev = get_device(sdev-sdev_gendev);
-   if (!scsi_dh || !dev ||
-   sdev-sdev_state == SDEV_CANCEL ||
-   sdev-sdev_state == SDEV_DEL)
+   if (!scsi_dh || !dev || scsi_device_being_removed(sdev))
err = SCSI_DH_NOSYS;
if (sdev-sdev_state == SDEV_OFFLINE)
err = SCSI_DH_DEV_OFFLINED;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cc64587..fead252 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -454,6 +454,11 @@ static inline int scsi_device_created(struct scsi_device 
*sdev)
return sdev-sdev_state == SDEV_CREATED ||
sdev-sdev_state == SDEV_CREATED_BLOCK;
 }
+static inline int scsi_device_being_removed(struct scsi_device *sdev)
+{
+   return sdev-sdev_state == SDEV_CANCEL ||
+   sdev-sdev_state == SDEV_DEL;
+}
 
 /* accessor functions for the SCSI parameters */
 static inline int scsi_device_sync(struct scsi_device *sdev)
-- 
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 v14 4/6] Rework scsi_internal_device_unblock()

[Bart Van Assche]

Modify scsi_internal_device_unblock() such that it uses
scsi_device_set_state() to change the device state. This is only
possible by changing scsi_device_set_state() such that it allows
the transition from SDEV_CREATED_BLOCK to the SDEV_OFFLINE and
SDEV_TRANSPORT_OFFLINE states. Notes:
- All callers of scsi_internal_device_unblock() ignore the return
  value of this function.
- Since the SDEV_CREATED_BLOCK to SDEV_{TRANSPORT_,}OFFLINE transition
  is now allowed, direct scsi_device_set_state() calls that change the
  device state from SDEV_CREATED_BLOCK into SDEV_*OFFLINE will now
  proceed instead of being rejected.

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
---
 drivers/scsi/scsi_lib.c |   29 +++--
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 124392f..9eb05a7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2147,6 +2147,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum 
scsi_device_state state)
case SDEV_RUNNING:
case SDEV_QUIESCE:
case SDEV_BLOCK:
+   case SDEV_CREATED_BLOCK:
break;
default:
goto illegal;
@@ -2501,29 +2502,21 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 {
struct request_queue *q = sdev-request_queue; 
unsigned long flags;
+   int res;
 
/*
 * Try to transition the scsi device to SDEV_RUNNING or one of the
 * offlined states and goose the device queue if successful.
 */
-   if ((sdev-sdev_state == SDEV_BLOCK) ||
-   (sdev-sdev_state == SDEV_TRANSPORT_OFFLINE))
-   sdev-sdev_state = new_state;
-   else if (sdev-sdev_state == SDEV_CREATED_BLOCK) {
-   if (new_state == SDEV_TRANSPORT_OFFLINE ||
-   new_state == SDEV_OFFLINE)
-   sdev-sdev_state = new_state;
-   else
-   sdev-sdev_state = SDEV_CREATED;
-   } else if (sdev-sdev_state != SDEV_CANCEL 
-sdev-sdev_state != SDEV_OFFLINE)
-   return -EINVAL;
-
-   spin_lock_irqsave(q-queue_lock, flags);
-   blk_start_queue(q);
-   spin_unlock_irqrestore(q-queue_lock, flags);
-
-   return 0;
+   if (sdev-sdev_state == SDEV_CREATED_BLOCK  new_state == SDEV_RUNNING)
+   new_state = SDEV_CREATED;
+   res = scsi_device_set_state(sdev, new_state);
+   if (!scsi_device_blocked(sdev)) {
+   spin_lock_irqsave(q-queue_lock, flags);
+   blk_start_queue(q);
+   spin_unlock_irqrestore(q-queue_lock, flags);
+   }
+   return res;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
 
-- 
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