RE: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

RE: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[James.Smart]

Yep... I heard from some others with the same comments. 

Here's a streamlined version of this patch. Gets rid of all
the list searching, atomic add to the list, etc...

-- James

> I don't like the recursion part of this at all.  I'd suggest to make
> scsi_alloc_target a tiny wrapper around the transport ->alloc_target +
> a default action, the shared body that's in scsi_alloc_target will
> become __scsi_alloc_target.
> 
> > +	spin_lock_irqsave(shost->host_lock, flags);
> > +	list_for_each_entry_safe(rport, next_rport,
> > +			&fc_host_rports(shost), peers) {
> 
> you don't actually need list_for_each_entry_safe here as 
> you're dropping
> out of the loop once the right rport has been found.
> 
> > +		if ((rport->channel == channel) &&
> > +		    (rport->scsi_target_id == id)) {
> > +			spin_unlock_irqrestore(shost->host_lock, flags);
> > +			starget = 
> scsi_alloc_target(&rport->dev, channel, id);
> > +			if (starget)
> > +				return 0;
> > +			return 1;
> 
> this is always under the scan_mutex, right?  else we'd get a 
> race after
> the sin_unlock but before registering the new target.



diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_scan.c linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c
--- linux-2.6.12-rc6/drivers/scsi/scsi_scan.c	2005-06-09 15:58:31.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c	2005-06-10 16:11:04.000000000 -0400
@@ -332,9 +332,23 @@ static struct scsi_target *scsi_alloc_ta
 	unsigned long flags;
 	const int size = sizeof(struct scsi_target)
 		+ shost->transportt->target_size;
-	struct scsi_target *starget = kmalloc(size, GFP_ATOMIC);
+	struct scsi_target *starget;
 	struct scsi_target *found_target;
 
+	/*
+	 * Obtain the real parent from the transport. The transport
+	 * is allowed to fail (no error) if there is nothing at that
+	 * target id.
+	 */
+	if (shost->transportt->target_parent) {
+		spin_lock_irqsave(shost->host_lock, flags);
+		parent = shost->transportt->target_parent(shost, channel, id);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		if (!parent)
+			return NULL;
+	}
+
+	starget = kmalloc(size, GFP_KERNEL);
 	if (!starget) {
 		printk(KERN_ERR "%s: allocation failure\n", __FUNCTION__);
 		return NULL;
diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_transport_fc.c
--- linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c	2005-06-09 15:58:31.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_transport_fc.c	2005-06-10 16:01:20.000000000 -0400
@@ -1022,6 +1022,23 @@ static int fc_rport_match(struct attribu
 	return &i->rport_attr_cont.ac == cont;
 }
 
+
+/*
+ * Must be called with shost->host_lock held
+ */
+static struct device *fc_target_parent(struct Scsi_Host *shost,
+					int channel, uint id)
+{
+	struct fc_rport *rport;
+
+	list_for_each_entry(rport, &fc_host_rports(shost), peers)
+		if ((rport->channel == channel) &&
+		    (rport->scsi_target_id == id))
+			return &rport->dev;
+
+	return NULL;
+}
+
 struct scsi_transport_template *
 fc_attach_transport(struct fc_function_template *ft)
 {
@@ -1057,6 +1074,8 @@ fc_attach_transport(struct fc_function_t
 
 	/* Transport uses the shost workq for scsi scanning */
 	i->t.create_work_queue = 1;
+
+	i->t.target_parent = fc_target_parent;
 	
 	/*
 	 * Setup SCSI Target Attributes.
diff -upNr linux-2.6.12-rc6/include/scsi/scsi_transport.h linux-2.6.12-rc6.PATCHED/include/scsi/scsi_transport.h
--- linux-2.6.12-rc6/include/scsi/scsi_transport.h	2005-06-09 15:58:52.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/include/scsi/scsi_transport.h	2005-06-10 15:58:32.000000000 -0400
@@ -28,6 +28,14 @@ struct scsi_transport_template {
 	struct transport_container target_attrs;
 	struct transport_container device_attrs;
 
+	/*
+	 * If set, call target_parent prior to allocating a scsi_target,
+	 * so we get the appropriate parent for the target. This function
+	 * is required for transports like FC and iSCSI that do not put the
+	 * scsi_target under scsi_host.
+	 */
+	struct device *(*target_parent)(struct Scsi_Host *, int, uint);
+
 	/* The size of the specific transport attribute structure (a
 	 * space of this size will be left at the end of the
 	 * scsi_* structure */
@@ -35,9 +43,7 @@ struct scsi_transport_template {
 	int	target_size;
 	int	host_size;
 
-	/*
-	 * True if the transport wants to use a host-based work-queue
-	 */
+	/* True if the transport wants to use a host-based work-queue */
 	unsigned int create_work_queue : 1;
 };
 

Re: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[Christoph Hellwig]

> diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_scan.c linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c
> --- linux-2.6.12-rc6/drivers/scsi/scsi_scan.c	2005-06-09 15:58:31.000000000 -0400
> +++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c	2005-06-10 16:11:04.000000000 -0400
> @@ -332,9 +332,23 @@ static struct scsi_target *scsi_alloc_ta
>  	unsigned long flags;
>  	const int size = sizeof(struct scsi_target)
>  		+ shost->transportt->target_size;
> -	struct scsi_target *starget = kmalloc(size, GFP_ATOMIC);
> +	struct scsi_target *starget;
>  	struct scsi_target *found_target;
>  
> +	/*
> +	 * Obtain the real parent from the transport. The transport
> +	 * is allowed to fail (no error) if there is nothing at that
> +	 * target id.
> +	 */
> +	if (shost->transportt->target_parent) {
> +		spin_lock_irqsave(shost->host_lock, flags);
> +		parent = shost->transportt->target_parent(shost, channel, id);
> +		spin_unlock_irqrestore(shost->host_lock, flags);
> +		if (!parent)
> +			return NULL;
> +	}

Ok, this idea is a lot better, but I think scsi_alloc_target is still
the wrong place to put this.  It has an explicit parent argument that
all but one of it's callers get right, and we're now totally ignoring
and looking it up again when the transport has a ->target_parent method.

I'm not totally sure where the right place to put it in the
scsi_scan_host_selected path is, though.  Probably scsi_scan_host_selected
shoud just become __scsi_scan_host_selected, scsi_scan_host would call
it directly but the sysfs and legacy procfs scanning would call the method.

Or it might be better to give the transport completely control over the
scan sysfs attribute, that way we could even allow scanning by WWNs or
similar things.

Re: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[Patrick Mansfield]

On Sat, Jun 11, 2005 at 04:37:45PM +0100, Christoph Hellwig wrote:

> > +	if (shost->transportt->target_parent) {
> > +		spin_lock_irqsave(shost->host_lock, flags);
> > +		parent = shost->transportt->target_parent(shost, channel, id);
> > +		spin_unlock_irqrestore(shost->host_lock, flags);
> > +		if (!parent)
> > +			return NULL;
> > +	}
> 
> Ok, this idea is a lot better, but I think scsi_alloc_target is still
> the wrong place to put this.  It has an explicit parent argument that
> all but one of it's callers get right, and we're now totally ignoring
> and looking it up again when the transport has a ->target_parent method.
> 
> I'm not totally sure where the right place to put it in the
> scsi_scan_host_selected path is, though.  Probably scsi_scan_host_selected
> shoud just become __scsi_scan_host_selected, scsi_scan_host would call
> it directly but the sysfs and legacy procfs scanning would call the method.
> 
> Or it might be better to give the transport completely control over the
> scan sysfs attribute, that way we could even allow scanning by WWNs or
> similar things.

What do you think of Mike C's patch, to add a transportt->scan function?

http://marc.theaimsgroup.com/?l=linux-scsi&m=111671146532103&w=2

It needs a bit more code in the transport compared to the above. It also
allows the transport to more efficiently scan (the transport can scan only
devices it knows are attached, versus all id's up to max_id).

If the scsi_target (sysfs targetC:I:L) was always under the scsi_host
(syfs hostH) in the device tree there would no problem. Then the transport
specific target object (like rport for FC) would have to be parallel to or
under the target.

-- Patrick Mansfield

Re: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[Christoph Hellwig]

On Sat, Jun 11, 2005 at 11:15:16AM -0700, Patrick Mansfield wrote:
> What do you think of Mike C's patch, to add a transportt->scan function?
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=111671146532103&w=2
> 
> It needs a bit more code in the transport compared to the above. It also
> allows the transport to more efficiently scan (the transport can scan only
> devices it knows are attached, versus all id's up to max_id).

Looks good except that it's not updating the legacy procfs-based
scanning code.

[Patch] take 4: Correct issue with midlayer scsi target allocation and transports that create the targets

[Patrick Mansfield]

Hi ... yet another patch to fix user space scanning.

Based on code in James Smart patch, but moves the ->target_parent call
outside of scsi_alloc_target.

Adding a transport->scan does not play well with __scsi_add_device and
scsi_get_host_dev.

There appears to be a ref-counting issue with rport->dev, both in this
patch, and in current code calling scsi_scan_target(&rport->dev ...), or I
am misunderstanding the code (rport->dev ref count is not incremented)
hence the FIXME.

Signed off by: Patrick Mansfield <patmans@us.ibm.coM>

diff -uprN -X /home/patman/dontdiff scsi-rc-fixes-2.6/drivers/scsi/scsi_scan.c uscan-scsi-rc-fixes-2.6/drivers/scsi/scsi_scan.c
--- scsi-rc-fixes-2.6/drivers/scsi/scsi_scan.c	Tue Jun 14 10:15:57 2005
+++ uscan-scsi-rc-fixes-2.6/drivers/scsi/scsi_scan.c	Tue Jun 14 11:40:56 2005
@@ -373,6 +373,15 @@ static struct scsi_target *scsi_alloc_ta
 	return found_target;
 }
 
+static struct device *scsi_target_parent(struct Scsi_Host *shost, unsigned
+					int channel, unsigned int id)
+{
+	if (shost->transportt->target_parent)
+		return shost->transportt->target_parent(shost, channel, id);
+	else 
+		return &shost->shost_gendev;
+}
+
 /**
  * scsi_target_reap - check to see if target is in use and destroy if not
  *
@@ -1190,10 +1199,15 @@ struct scsi_device *__scsi_add_device(st
 				      uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
-	struct device *parent = &shost->shost_gendev;
+	struct device *parent;
 	int res;
-	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
+	struct scsi_target *starget;
 
+	parent = scsi_target_parent(shost, channel, id);
+	if (!parent)
+		return ERR_PTR(-ENODEV);
+
+	starget = scsi_alloc_target(parent, channel, id);
 	if (!starget)
 		return ERR_PTR(-ENOMEM);
 
@@ -1314,6 +1328,7 @@ static void scsi_scan_channel(struct Scs
 			      unsigned int id, unsigned int lun, int rescan)
 {
 	uint order_id;
+	struct device *parent;
 
 	if (id == SCAN_WILD_CARD)
 		for (id = 0; id < shost->max_id; ++id) {
@@ -1333,10 +1348,16 @@ static void scsi_scan_channel(struct Scs
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			scsi_scan_target(&shost->shost_gendev, channel, order_id, lun, rescan);
+			parent = scsi_target_parent(shost, channel, id);
+			if (parent)
+				scsi_scan_target(parent, channel,
+						 order_id, lun, rescan);
 		}
-	else
-		scsi_scan_target(&shost->shost_gendev, channel, id, lun, rescan);
+	else {
+		parent = scsi_target_parent(shost, channel, id);
+		if (parent)
+			scsi_scan_target(parent, channel, id, lun, rescan);
+	}
 }
 
 int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
@@ -1432,8 +1453,12 @@ struct scsi_device *scsi_get_host_dev(st
 {
 	struct scsi_device *sdev;
 	struct scsi_target *starget;
+	struct device *parent;
 
-	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
+	parent = scsi_target_parent(shost, 0, shost->this_id);
+	if (!parent)
+		return NULL;
+	starget = scsi_alloc_target(parent, 0, shost->this_id);
 	if (!starget)
 		return NULL;
 
diff -uprN -X /home/patman/dontdiff scsi-rc-fixes-2.6/drivers/scsi/scsi_transport_fc.c uscan-scsi-rc-fixes-2.6/drivers/scsi/scsi_transport_fc.c
--- scsi-rc-fixes-2.6/drivers/scsi/scsi_transport_fc.c	Tue Jun 14 10:15:57 2005
+++ uscan-scsi-rc-fixes-2.6/drivers/scsi/scsi_transport_fc.c	Tue Jun 14 13:25:35 2005
@@ -1022,6 +1022,27 @@ static int fc_rport_match(struct attribu
 	return &i->rport_attr_cont.ac == cont;
 }
 
+static struct device *fc_target_parent(struct Scsi_Host *shost,
+					int channel, uint id)
+{
+	struct fc_rport *rport;
+	struct device *parent = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry(rport, &fc_host_rports(shost), peers)
+		if ((rport->channel == channel) &&
+		    (rport->scsi_target_id == id)) {
+			parent = &rport->dev;
+			/*
+			 * FIXME who holds parent in place?
+			 */
+			break;
+		}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+	return parent;
+}
+
 struct scsi_transport_template *
 fc_attach_transport(struct fc_function_template *ft)
 {
@@ -1057,6 +1078,8 @@ fc_attach_transport(struct fc_function_t
 
 	/* Transport uses the shost workq for scsi scanning */
 	i->t.create_work_queue = 1;
+
+	i->t.target_parent = fc_target_parent;
 	
 	/*
 	 * Setup SCSI Target Attributes.
diff -uprN -X /home/patman/dontdiff scsi-rc-fixes-2.6/include/scsi/scsi_transport.h uscan-scsi-rc-fixes-2.6/include/scsi/scsi_transport.h
--- scsi-rc-fixes-2.6/include/scsi/scsi_transport.h	Tue Jun 14 10:16:26 2005
+++ uscan-scsi-rc-fixes-2.6/include/scsi/scsi_transport.h	Tue Jun 14 11:21:36 2005
@@ -27,6 +27,13 @@ struct scsi_transport_template {
 	struct transport_container host_attrs;
 	struct transport_container target_attrs;
 	struct transport_container device_attrs;
+	/*
+	 * If set, call target_parent prior to allocating or scanning a
+	 * scsi_target, so we get the appropriate parent for the target.
+	 * This function is required for transports like FC and iSCSI that
+	 * do not put the scsi_target under scsi_host.
+	 */
+	struct device *(*target_parent)(struct Scsi_Host *, int, uint);
 
 	/* The size of the specific transport attribute structure (a
 	 * space of this size will be left at the end of the

Re: [Patch] take 4: Correct issue with midlayer scsi target allocation and transports that create the targets

[Christoph Hellwig]

On Tue, Jun 14, 2005 at 03:17:00PM -0700, Patrick Mansfield wrote:
> Hi ... yet another patch to fix user space scanning.
> 
> Based on code in James Smart patch, but moves the ->target_parent call
> outside of scsi_alloc_target.
> 
> Adding a transport->scan does not play well with __scsi_add_device and
> scsi_get_host_dev.
> 
> There appears to be a ref-counting issue with rport->dev, both in this
> patch, and in current code calling scsi_scan_target(&rport->dev ...), or I
> am misunderstanding the code (rport->dev ref count is not incremented)
> hence the FIXME.

> @@ -1190,10 +1199,15 @@ struct scsi_device *__scsi_add_device(st
>  				      uint id, uint lun, void *hostdata)
>  {
>  	struct scsi_device *sdev;
> -	struct device *parent = &shost->shost_gendev;
> +	struct device *parent;
>  	int res;
> -	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
> +	struct scsi_target *starget;
>  
> +	parent = scsi_target_parent(shost, channel, id);
> +	if (!parent)
> +		return ERR_PTR(-ENODEV);
> +

scsi_add_device is a library function for LLDDs, thus it does not need
this fix.  If we want LLDDs with objects below the target to use them
in the future we should probably change the prototype so it takes a parent
object.

>  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> @@ -1432,8 +1453,12 @@ struct scsi_device *scsi_get_host_dev(st
>  {
>  	struct scsi_device *sdev;
>  	struct scsi_target *starget;
> +	struct device *parent;
>  
> -	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
> +	parent = scsi_target_parent(shost, 0, shost->this_id);
> +	if (!parent)
> +		return NULL;
> +	starget = scsi_alloc_target(parent, 0, shost->this_id);

scsi_get_host_dev is a library function for LLDDs and does not need to
care about this.

> +	list_for_each_entry(rport, &fc_host_rports(shost), peers)
> +		if ((rport->channel == channel) &&
> +		    (rport->scsi_target_id == id)) {

lots of superflous braces ;-)

Re: [Patch] take 4: Correct issue with midlayer scsi target allocation and transports that create the targets

[Patrick Mansfield]

On Tue, Jun 14, 2005 at 11:35:35PM +0100, Christoph Hellwig wrote:
> On Tue, Jun 14, 2005 at 03:17:00PM -0700, Patrick Mansfield wrote:

> > @@ -1190,10 +1199,15 @@ struct scsi_device *__scsi_add_device(st
> >  				      uint id, uint lun, void *hostdata)
> >  {
> >  	struct scsi_device *sdev;
> > -	struct device *parent = &shost->shost_gendev;
> > +	struct device *parent;
> >  	int res;
> > -	struct scsi_target *starget = scsi_alloc_target(parent, channel, id);
> > +	struct scsi_target *starget;
> >  
> > +	parent = scsi_target_parent(shost, channel, id);
> > +	if (!parent)
> > +		return ERR_PTR(-ENODEV);
> > +
> 
> scsi_add_device is a library function for LLDDs, thus it does not need
> this fix.  If we want LLDDs with objects below the target to use them
> in the future we should probably change the prototype so it takes a parent
> object.

> >  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> > @@ -1432,8 +1453,12 @@ struct scsi_device *scsi_get_host_dev(st
> >  {
> >  	struct scsi_device *sdev;
> >  	struct scsi_target *starget;
> > +	struct device *parent;
> >  
> > -	starget = scsi_alloc_target(&shost->shost_gendev, 0, shost->this_id);
> > +	parent = scsi_target_parent(shost, 0, shost->this_id);
> > +	if (!parent)
> > +		return NULL;
> > +	starget = scsi_alloc_target(parent, 0, shost->this_id);
> 
> scsi_get_host_dev is a library function for LLDDs and does not need to
> care about this.

But if the transport is like FC or iSCSI, the eventual calls to
scsi_alloc_sdev() will fail (for qlogic, ->slave_alloc returns ENXIO) or
somehow be incorrect, so why not change these?

> > +	list_for_each_entry(rport, &fc_host_rports(shost), peers)
> > +		if ((rport->channel == channel) &&
> > +		    (rport->scsi_target_id == id)) {
> 
> lots of superflous braces ;-)

OK ))

Re: [Patch] take 4: Correct issue with midlayer scsi target allocation and transports that create the targets

[Christoph Hellwig]

On Tue, Jun 14, 2005 at 03:54:33PM -0700, Patrick Mansfield wrote:
> > scsi_get_host_dev is a library function for LLDDs and does not need to
> > care about this.
> 
> But if the transport is like FC or iSCSI, the eventual calls to
> scsi_alloc_sdev() will fail (for qlogic, ->slave_alloc returns ENXIO) or
> somehow be incorrect, so why not change these?

I'd rather see people not using it.  And in case we need any caller
to use scsi_target_parent we should consolidate this.  Anyway we should
have a simple as possible patch for 2.6.12.  Probably the one before
this one.

[Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[James.Smart]

I was pinged by Pat Mansfield on an issue with using the scan sysfs
attribute to rescan FC adapters. Especially if the target and luns
structures have been torn down via the delete sysfs attribute. See:
http://marc.theaimsgroup.com/?l=linux-scsi&m=111671146532103&w=2

We have some nasty issues with 2.6.12-rc6. Any request to scan on
the lpfc or qla2xxx FC adapters will oops. What is happening is the
system is defaulting to non-transport registered targets, which
inherit the parent of the scan. On this second scan, performed by
the attribute, the parent becomes the shost instead of the rport.
The slave functions in the 2 FC adapters use starget_to_rport()
routines, which incorrectly map the shost as an rport pointer.

Additionally, this pointed out other weaknesses:
- If the target structure is torn down outside of the transport,
  we have no method for it to be regenerated at the proper parent.
- We have race conditions on the target being allocated by both
  the midlayer scan (parent=shost) and by the fc transport
  (parent=rport).

Mike Christie proposed adding a transport scan function to resolve
this (see link above). However, I'm concerned that his patch does
not catch all potential scan calls, especially those that may be
within the kernel.

Here's a proposed patch. The real problem in all of this was that
we had 2 places allocating a scsi target (or should I say, that
invoked the scan function that makes the target, but the parents
are different).  This patch adds a transport alloc_target function,
which allows the transport to adjusts the parent, and then calls
back into the midlayer to complete the allocation (small recursive
call). It has one additional optimization in that, if the transport
is doling out targets, it fails target allocations on things that
don't exist. This will make scanning slightly faster. I also like
this patch as it leaves all "scan" logic in the midlayer.

-- James S




diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_priv.h linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_priv.h
--- linux-2.6.12-rc6/drivers/scsi/scsi_priv.h	2005-06-10 10:05:15.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_priv.h	2005-06-10 10:06:21.000000000 -0400
@@ -125,6 +125,7 @@ extern int scsi_scan_host_selected(struc
 				   unsigned int, unsigned int, int);
 extern void scsi_forget_host(struct Scsi_Host *);
 extern void scsi_rescan_device(struct device *);
+extern struct scsi_target *scsi_alloc_target(struct device *, int , uint);
 
 /* scsi_sysctl.c */
 #ifdef CONFIG_SYSCTL
diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_scan.c linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c
--- linux-2.6.12-rc6/drivers/scsi/scsi_scan.c	2005-06-09 15:58:31.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_scan.c	2005-06-10 10:38:27.000000000 -0400
@@ -324,7 +324,7 @@ static struct scsi_target *__scsi_find_t
 	return found_starget;
 }
 
-static struct scsi_target *scsi_alloc_target(struct device *parent,
+struct scsi_target *scsi_alloc_target(struct device *parent,
 					     int channel, uint id)
 {
 	struct Scsi_Host *shost = dev_to_shost(parent);
@@ -332,9 +332,34 @@ static struct scsi_target *scsi_alloc_ta
 	unsigned long flags;
 	const int size = sizeof(struct scsi_target)
 		+ shost->transportt->target_size;
-	struct scsi_target *starget = kmalloc(size, GFP_ATOMIC);
+	struct scsi_target *starget;
 	struct scsi_target *found_target;
 
+	/*
+	 * Allow transport to allocate target struct or reject target.
+	 * Only make this call if the parent is the scsi host. If it
+	 * is not the host, it is the transport calling in to allocate
+	 * the target.
+	 */
+	if ((shost->transportt->alloc_target) &&
+	    (scsi_is_host_device(parent))) {
+		if ((shost->transportt->alloc_target)(shost, channel, id))
+			return NULL;
+	}
+
+	/* Find existing target */
+	spin_lock_irqsave(shost->host_lock, flags);
+	found_target = __scsi_find_target(parent, channel, id);
+	if (found_target) {
+		found_target->reap_ref++;
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		put_device(parent);
+		return found_target;
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	/* allocate and add */
+	starget = kmalloc(size, GFP_ATOMIC);
 	if (!starget) {
 		printk(KERN_ERR "%s: allocation failure\n", __FUNCTION__);
 		return NULL;
@@ -352,26 +377,14 @@ static struct scsi_target *scsi_alloc_ta
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	spin_lock_irqsave(shost->host_lock, flags);
-
-	found_target = __scsi_find_target(parent, channel, id);
-	if (found_target)
-		goto found;
-
 	list_add_tail(&starget->siblings, &shost->__targets);
 	spin_unlock_irqrestore(shost->host_lock, flags);
-	/* allocate and add */
 	transport_setup_device(&starget->dev);
 	device_add(&starget->dev);
 	transport_add_device(&starget->dev);
 	return starget;
-
- found:
-	found_target->reap_ref++;
-	spin_unlock_irqrestore(shost->host_lock, flags);
-	put_device(parent);
-	kfree(starget);
-	return found_target;
 }
+EXPORT_SYMBOL(scsi_alloc_target);
 
 /**
  * scsi_target_reap - check to see if target is in use and destroy if not
diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_transport_fc.c
--- linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c	2005-06-09 15:58:31.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/drivers/scsi/scsi_transport_fc.c	2005-06-10 10:43:36.000000000 -0400
@@ -1022,6 +1022,33 @@ static int fc_rport_match(struct attribu
 	return &i->rport_attr_cont.ac == cont;
 }
 
+/*
+ * Attempts to allocate scsi target for remote port. Invoked typically
+ *   on rescans by the midlayer.
+ * Returns 0 on succes, non-zero on failure
+ */
+static int fc_alloc_target(struct Scsi_Host *shost, int channel, uint id)
+{
+	struct fc_rport *rport, *next_rport;
+	struct scsi_target *starget;
+	unsigned long flags;
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_for_each_entry_safe(rport, next_rport,
+			&fc_host_rports(shost), peers) {
+		if ((rport->channel == channel) &&
+		    (rport->scsi_target_id == id)) {
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			starget = scsi_alloc_target(&rport->dev, channel, id);
+			if (starget)
+				return 0;
+			return 1;
+		}
+	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+	return 1;
+}
+
 struct scsi_transport_template *
 fc_attach_transport(struct fc_function_template *ft)
 {
@@ -1057,6 +1084,9 @@ fc_attach_transport(struct fc_function_t
 
 	/* Transport uses the shost workq for scsi scanning */
 	i->t.create_work_queue = 1;
+
+	/* Transport manages target structure allocation */
+	i->t.alloc_target = fc_alloc_target;
 	
 	/*
 	 * Setup SCSI Target Attributes.
diff -upNr linux-2.6.12-rc6/include/scsi/scsi_transport.h linux-2.6.12-rc6.PATCHED/include/scsi/scsi_transport.h
--- linux-2.6.12-rc6/include/scsi/scsi_transport.h	2005-06-09 15:58:52.000000000 -0400
+++ linux-2.6.12-rc6.PATCHED/include/scsi/scsi_transport.h	2005-06-10 10:37:07.000000000 -0400
@@ -28,6 +28,9 @@ struct scsi_transport_template {
 	struct transport_container target_attrs;
 	struct transport_container device_attrs;
 
+	/* If non-NULL, transport creates targets */
+	int	(* alloc_target)(struct Scsi_Host *, int, uint);
+
 	/* The size of the specific transport attribute structure (a
 	 * space of this size will be left at the end of the
 	 * scsi_* structure */
@@ -35,9 +38,7 @@ struct scsi_transport_template {
 	int	target_size;
 	int	host_size;
 
-	/*
-	 * True if the transport wants to use a host-based work-queue
-	 */
+	/* True if the transport wants to use a host-based work-queue */
 	unsigned int create_work_queue : 1;
 };
 

Re: [Patch] Correct issue with midlayer scsi target allocation and transports that create the targets

[Christoph Hellwig]

> Here's a proposed patch. The real problem in all of this was that
> we had 2 places allocating a scsi target (or should I say, that
> invoked the scan function that makes the target, but the parents
> are different).  This patch adds a transport alloc_target function,
> which allows the transport to adjusts the parent, and then calls
> back into the midlayer to complete the allocation (small recursive
> call). It has one additional optimization in that, if the transport
> is doling out targets, it fails target allocations on things that
> don't exist. This will make scanning slightly faster. I also like
> this patch as it leaves all "scan" logic in the midlayer.

I don't like the recursion part of this at all.  I'd suggest to make
scsi_alloc_target a tiny wrapper around the transport ->alloc_target +
a default action, the shared body that's in scsi_alloc_target will
become __scsi_alloc_target.

> +	spin_lock_irqsave(shost->host_lock, flags);
> +	list_for_each_entry_safe(rport, next_rport,
> +			&fc_host_rports(shost), peers) {

you don't actually need list_for_each_entry_safe here as you're dropping
out of the loop once the right rport has been found.

> +		if ((rport->channel == channel) &&
> +		    (rport->scsi_target_id == id)) {
> +			spin_unlock_irqrestore(shost->host_lock, flags);
> +			starget = scsi_alloc_target(&rport->dev, channel, id);
> +			if (starget)
> +				return 0;
> +			return 1;

this is always under the scan_mutex, right?  else we'd get a race after
the sin_unlock but before registering the new target.