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.

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

[James.Smart]

My recommendation is to use the patch I last posted, with the call
to target_parent in scsi_alloc_target(). Simple and covers all bases.
This would fix 2.6.12. We can then investigate better alternatives post
2.6.12 release.

-- james s


> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, June 17, 2005 7:12 AM
> To: Patrick Mansfield
> Cc: Christoph Hellwig; Smart, James; linux-scsi@vger.kernel.org
> Subject: Re: [Patch] take 4: Correct issue with midlayer scsi target
> allocation and transports that create the targets
> 
> 
> 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.
> 
>