[PATCH 1/2] IB/srp: Fix SCSI scanning

[PATCH 1/2] IB/srp: Fix SCSI scanning

[Bart Van Assche]

Avoid that SCSI scanning triggers creation of targets with non-zero
channel or non-zero id.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Roland Dreier <roland@purestorage.com>
Cc: David Dillow <dillowda@ornl.gov>
Cc: <stable@kernel.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index ee165fd..de4fd32 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -110,6 +110,11 @@ static const char *srp_target_info(struct Scsi_Host *host)
 	return host_to_target(host)->target_name;
 }
 
+static int srp_target_alloc(struct scsi_target *starget)
+{
+	return starget->channel == 0 && starget->id == 0 ? 0 : -ENODEV;
+}
+
 static int srp_target_is_topspin(struct srp_target_port *target)
 {
 	static const u8 topspin_oui[3] = { 0x00, 0x05, 0xad };
@@ -1836,6 +1841,7 @@ static struct scsi_host_template srp_template = {
 	.name				= "InfiniBand SRP initiator",
 	.proc_name			= DRV_NAME,
 	.info				= srp_target_info,
+	.target_alloc			= srp_target_alloc,
 	.queuecommand			= srp_queuecommand,
 	.eh_abort_handler		= srp_abort,
 	.eh_device_reset_handler	= srp_reset_device,
-- 
1.7.3.4

Re: [PATCH 1/2] IB/srp: Fix SCSI scanning

[David Dillow]

On Sun, 2011-06-19 at 13:48 +0200, Bart Van Assche wrote:
> Avoid that SCSI scanning triggers creation of targets with non-zero
> channel or non-zero id.

Good catch, Bart. How did you find this?

I would like to have a little more meat in the description. What real
problem are we avoiding -- what happens when we get non-zero id/channel?

Based on a quick look at the code -- I don't have ready access to a test
system today -- I'm guessing that we create duplicate devices as a
result of a manual scan?

Signed-off-by: David Dillow <dillowda@ornl.gov>

Re: [PATCH 1/2] IB/srp: Fix SCSI scanning

[Bart Van Assche]

On Sun, Jun 19, 2011 at 7:27 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> On Sun, 2011-06-19 at 13:48 +0200, Bart Van Assche wrote:
>> Avoid that SCSI scanning triggers creation of targets with non-zero
>> channel or non-zero id.
>
> Good catch, Bart. How did you find this?
>
> I would like to have a little more meat in the description. What real
> problem are we avoiding -- what happens when we get non-zero id/channel?
>
> Based on a quick look at the code -- I don't have ready access to a test
> system today -- I'm guessing that we create duplicate devices as a
> result of a manual scan?

Entirely correct. Some SRP target implementations allow to add and
remove LUNs after an SRP initiator has logged in. In order to update
the initiator a SCSI scan has to be initiated. Invoking the
rescan-scsi-bus.sh script on an initiator learned me that many
unwanted duplicate SCSI devices were created on the initiator.

Regarding the implementation: a SCSI scan is initiated by writing
directly or indirectly to /sys/class/scsi_host/host<number>/scan.
If the transport associated with the involved host provides a custom
scanning method, that method will be invoked. Otherwise
scsi_scan_host_selected() will be invoked. Since the SRP transport
does not provide a custom scanning method, for SRP the latter is
invoked. It is that functin that creates duplicate SCSI device nodes
if not prevented by target_alloc. Since which C:I:L values are valid
depends on the LUN addressing method used by the initiator I opted to
implement the filtering in the initiator driver instead of the
transport. As you might have noticed, the filter proposed for the
ibmvscsi driver differs slightly from the filter for the ib_srp
driver. That is because the ibmvscsi and the ib_srp drivers use
different LUN  addressing methods (see also paragraph 4.7, Logical
Unit Number (LUN) in the T10 SCSI Architecture Model standard).

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/srp: Fix SCSI scanning

[Christoph Hellwig]

We really shouldn't require workaround for this in every SCSI driver.

scsi_scan_host_selected already makes sure never to scan above
shost->max_channel.  Could it be that you don't have that one set
properly in the srp driver?  It not we'll need to debug why it happens
instead of adding hacks like this.

Re: [PATCH 1/2] IB/srp: Fix SCSI scanning

[Bart Van Assche]

On Mon, Jun 20, 2011 at 1:37 PM, Christoph Hellwig <hch@infradead.org> wrote:
> We really shouldn't require workaround for this in every SCSI driver.
>
> scsi_scan_host_selected already makes sure never to scan above
> shost->max_channel.  Could it be that you don't have that one set
> properly in the srp driver?  It not we'll need to debug why it happens
> instead of adding hacks like this.

As far as I can see the default value for max_channel is zero. The patch
below also avoids creation of duplicate SCSI targets on my setup.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>

---
 drivers/infiniband/ulp/srp/ib_srp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index ee165fd..4ab1271 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2127,6 +2127,7 @@ static ssize_t srp_create_target(struct device *dev,
 		return -ENOMEM;

 	target_host->transportt  = ib_srp_transport_template;
+	target_host->max_id      = 1;
 	target_host->max_lun     = SRP_MAX_LUN;
 	target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb;

-- 
1.7.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html