target code updates to support scanned targets
James, this looks like it would be really useful for my driver and I hope it goes upstream soon. I do have one question.
Quoted 2 lines
2. rejig the scanning code to do target scanning via a parent generic device rather than the HCTL numbers.
I see the internal changes to scsi_scan.c that this refers to, but I'm not totally clear on what a driver should do to allocate a target and scan it when it finds out about a new target port.
Thanks,
Joe
Quoted 3 lines · Joe Scsi
I see the internal changes to
scsi_scan.cthat this refers to, but I'm not totally clear on what a driver should do to allocate a target and scan it when it finds out about a new target port.
A driver doesn't allocate a target. In this code, a target device is purely a LUN container and is managed by the mid-layer. The driver requests the scan of a target by parent device, channel and id. This request for a scan creates and parents the target object, but reaps it again if no actual LUNs are discovered.
James
Quoted 11 lines · James Bottomley
Quoted 3 lines · Joe Scsi
I see the internal changes to
scsi_scan.cthat this refers to, but I'm not totally clear on what a driver should do to allocate a target and scan it when it finds out about a new target port.A driver doesn't allocate a target. In this code, a target device is purely a LUN container and is managed by the mid-layer. The driver requests the scan of a target by parent device, channel and id. This request for a scan creates and parents the target object, but reaps it again if no actual LUNs are discovered.
Yes, but the parent needs to know if the starget is actually created. With the fc_rports snapshot I've been working with, I've coded up the following:
+ scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
+ SCAN_WILD_CARD, 0);
+
+ dev = container_of(rport->dev.children.next, struct device, node);
+ rport->starget = to_scsi_target(dev);
+ if (unlikely(!rport->starget))
dev_printk(KERN_ERR, &rport->dev, TGT_ALLOC_FAILURE_MSG, __FUNCTION__, shost->host_no);
so when the rport gets dropped (i.e. fc_remote_port_delete()), a scsi_remove_target() call can be issued. I'm not entirely thrilled with the structure-member poking (any other suggestions on how to get the child, welcomed), but it does work.
Also, the patch neglected to add scsi_scan_target() to an appropriate header file. Attached patch will cure -- I've added it to scsi_host.h, may wish to add to scsi_device.h.
--
AV
include/scsi/scsi_host.h +2 -0
@@ -575,6 +575,8 @@ extern void scsi_remove_host(struct Scsiextern struct Scsi_Host *scsi_host_get (struct Scsi_Host *);extern void scsi_host_put (struct Scsi_Host *t);extern struct Scsi_Host *scsi_host_lookup (unsigned short ); +extern void scsi_scan_target (struct device *,unsigned int ,unsigned int , +unsigned int ,int );extern u64 scsi_calculate_bounce_limit (struct Scsi_Host *);
Quoted 3 lines · Andrew Vasquez
Yes, but the parent needs to know if the starget is actually created. With the
fc_rportssnapshot I've been working with, I've coded up the following:
Why do you need to know if the scan actually found any LUNs?
Review
Quoted 6 lines
(quoted diff) +5
-0+ scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id, + SCAN_WILD_CARD, 0); + + dev = container_of(rport->dev.children.next, struct device, node); + rport->starget = to_scsi_target(dev);
can we get rid of this too ... does the rport really need to know if it has a target?
Quoted 8 lines
(quoted diff) +1
-0+ if (unlikely(!rport->starget))dev_printk(KERN_ERR, &rport->dev, TGT_ALLOC_FAILURE_MSG, __FUNCTION__, shost->host_no);so when the rport gets dropped (i.e.
fc_remote_port_delete()), ascsi_remove_target()call can be issued. I'm not entirely thrilled with the structure-member poking (any other suggestions on how to get the child, welcomed), but it does work.
Well, how about a different format for this, so there's a scsi_remove_target that takes a generic device (analogous to the scsi_scan_target) except that this time if the device isn't a target, we remove all the children of the device that are targets?
Quoted 3 lines
Also, the patch neglected to add
scsi_scan_target()to an appropriate header file. Attached patch will cure -- I've added it toscsi_host.h, may wish to add toscsi_device.h.
Yes, it should be in scsi_device.h which is where the scsi target functions currently are. I'll add it.
James
Quoted 7 lines · James Bottomley
Quoted 3 lines · Andrew Vasquez
Yes, but the parent needs to know if the starget is actually created. With the
fc_rportssnapshot I've been working with, I've coded up the following:Why do you need to know if the scan actually found any LUNs?
originally, just to handle the starget-specific interfaces originally defined.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +1
-0+ if (unlikely(!rport->starget))dev_printk(KERN_ERR, &rport->dev, TGT_ALLOC_FAILURE_MSG, __FUNCTION__, shost->host_no);
Quoted 10 lines
Quoted 4 lines
so when the rport gets dropped (i.e.
fc_remote_port_delete()), ascsi_remove_target()call can be issued. I'm not entirely thrilled with the structure-member poking (any other suggestions on how to get the child, welcomed), but it does work.Well, how about a different format for this, so there's a
scsi_remove_targetthat takes a generic device (analogous to thescsi_scan_target) except that this time if the device isn't a target, we remove all the children of the device that are targets?
along the same vein, we'll also need generic-device accessors to block and unblock all scsi_device children of an scsi_target. currently, scsi_internal_devive_[block|unblock]() is issued across all sdevs for a given starget:
void scsi_block_target(struct device *)
void scsi_unblock_target(struct device *)
one caveat with these parent-accessors is that we also lose 'per-starget' (un)blocking/removal granualarity -- not sure if that's going to be a problem for other interfaces in the future.
--
Andrew
Quoted 12 lines · Andrew Vasquez
Quoted 5 lines · James Bottomley
Well, how about a different format for this, so there's a
scsi_remove_targetthat takes a generic device (analogous to thescsi_scan_target) except that this time if the device isn't a target, we remove all the children of the device that are targets?along the same vein, we'll also need generic-device accessors to block and unblock all
scsi_devicechildren of anscsi_target. currently, scsi_internal_devive_[block|unblock]() is issued across all sdevs for a given starget:
Quoted 7 lines
void
scsi_block_target(struct device *)
voidscsi_unblock_target(struct device *)one caveat with these parent-accessors is that we also lose 'per-starget' (un)blocking/removal granualarity -- not sure if that's going to be a problem for other interfaces in the future.
Does the following seem reasonable?
* add scsi_target_block() and scsi_target_unblock() routines which take a generic-device. Side note: there are mixture of scsi_<object>_<action>() and scsi_<action>_<noun>() functions defined in the scsi APIs -- going forward are there any 'guides' or suggestions on which to choose?
* modify scsi_remove_target() to take a generic-device.
Signature · 140 lines
Andrew Vasquez
===== drivers/scsi/scsi_lib.c 1.149 vs edited =====
--- 1.149/drivers/scsi/scsi_lib.c 2005-02-15 23:48:33 -08:00
+++ edited/drivers/scsi/scsi_lib.c 2005-02-17 11:17:22 -08:00
@@ -1889,3 +1889,55 @@ scsi_internal_device_unblock(struct scsi
return 0;
}
EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
+
+static void
+device_block(struct scsi_device *sdev, void *data)
+{
+ scsi_internal_device_block(sdev);
+}
+
+static int
+target_block(struct device *dev, void *data)
+{
+ if (scsi_is_target_device(dev))
+ starget_for_each_device(to_scsi_target(dev), NULL,
+ device_block);
+ return 0;
+}
+
+void
+scsi_target_block(struct device *dev)
+{
+ if (scsi_is_target_device(dev))
+ starget_for_each_device(to_scsi_target(dev), NULL,
+ device_block);
+ else
+ device_for_each_child(dev, NULL, target_block);
+}
+EXPORT_SYMBOL_GPL(scsi_target_block);
+
+static void
+device_unblock(struct scsi_device *sdev, void *data)
+{
+ scsi_internal_device_unblock(sdev);
+}
+
+static int
+target_unblock(struct device *dev, void *data)
+{
+ if (scsi_is_target_device(dev))
+ starget_for_each_device(to_scsi_target(dev), NULL,
+ device_unblock);
+ return 0;
+}
+
+void
+scsi_target_unblock(struct device *dev)
+{
+ if (scsi_is_target_device(dev))
+ starget_for_each_device(to_scsi_target(dev), NULL,
+ device_unblock);
+ else
+ device_for_each_child(dev, NULL, target_unblock);
+}
+EXPORT_SYMBOL_GPL(scsi_target_unblock);
===== drivers/scsi/scsi_scan.c 1.141 vs edited =====
--- 1.141/drivers/scsi/scsi_scan.c 2005-02-15 23:48:33 -08:00
+++ edited/drivers/scsi/scsi_scan.c 2005-02-16 10:21:35 -08:00
@@ -1385,7 +1385,7 @@ void scsi_forget_host(struct Scsi_Host *
spin_lock_irqsave(shost->host_lock, flags);
list_for_each_entry_safe(starget, tmp, &shost->__targets, siblings) {
spin_unlock_irqrestore(shost->host_lock, flags);
- scsi_remove_target(starget);
+ scsi_remove_target(&starget->dev);
spin_lock_irqsave(shost->host_lock, flags);
}
spin_unlock_irqrestore(shost->host_lock, flags);
===== drivers/scsi/scsi_sysfs.c 1.65 vs edited =====
--- 1.65/drivers/scsi/scsi_sysfs.c 2005-02-15 23:48:33 -08:00
+++ edited/drivers/scsi/scsi_sysfs.c 2005-02-16 17:05:37 -08:00
@@ -634,15 +634,7 @@ void scsi_remove_device(struct scsi_devi
}
EXPORT_SYMBOL(scsi_remove_device);
-/**
- * scsi_remove_target - try to remove a target and all its devices
- * @starget: the target to remove
- *
- * Note: This is slightly racy. It is possible that if the user
- * requests the addition of another device then the target won't be
- * removed.
- */
-void scsi_remove_target(struct scsi_target *starget)
+void __scsi_remove_target(struct scsi_target *starget)
{
struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
unsigned long flags;
@@ -660,6 +652,31 @@ void scsi_remove_target(struct scsi_targ
}
spin_unlock_irqrestore(shost->host_lock, flags);
scsi_target_reap(starget);
+}
+
+/**
+ * scsi_remove_target - try to remove a target and all its devices
+ * @dev: generic starget or parent of generic stargets to be removed
+ *
+ * Note: This is slightly racy. It is possible that if the user
+ * requests the addition of another device then the target won't be
+ * removed.
+ */
+void scsi_remove_target(struct device *dev)
+{
+ struct device *rdev, *idev, *next;
+
+ if (scsi_is_target_device(dev)) {
+ __scsi_remove_target(to_scsi_target(dev));
+ return;
+ }
+
+ rdev = get_device(dev);
+ list_for_each_entry_safe(idev, next, &dev->children, node) {
+ if (scsi_is_target_device(idev))
+ __scsi_remove_target(to_scsi_target(idev));
+ }
+ put_device(rdev);
}
EXPORT_SYMBOL(scsi_remove_target);
===== include/scsi/scsi_device.h 1.30 vs edited =====
--- 1.30/include/scsi/scsi_device.h 2005-02-15 23:48:33 -08:00
+++ edited/include/scsi/scsi_device.h 2005-02-17 11:15:51 -08:00
@@ -227,7 +227,9 @@ extern void scsi_device_resume(struct sc
extern void scsi_target_quiesce(struct scsi_target *);
extern void scsi_target_resume(struct scsi_target *);
extern void scsi_target_reap(struct scsi_target *);
-extern void scsi_remove_target(struct scsi_target *);
+extern void scsi_target_block(struct device *);
+extern void scsi_target_unblock(struct device *);
+extern void scsi_remove_target(struct device *);
extern const char *scsi_device_state_name(enum scsi_device_state);
extern int scsi_is_sdev_device(const struct device *);
extern int scsi_is_target_device(const struct device *);Quoted 5 lines
A driver doesn't allocate a target. In this code, a target device is purely a LUN container and is managed by the mid-layer. The driver requests the scan of a target by parent device, channel and id. This request for a scan creates and parents the target object, but reaps it again if no actual LUNs are discovered.
OK (and sorry if I'm being dense) but does this mean that a network SCSI transport should make up an "id" for each target port it connects to and then call scsi_scan_target()?
Thanks,
Joe
Quoted 3 lines · Joe Scsi
OK (and sorry if I'm being dense) but does this mean that a network SCSI transport should make up an "id" for each target port it connects to and then call
scsi_scan_target()?
Currently yes ... now that we allow unscanned hosts, it's not strictly necessary; we could allow the target id to be a string with meaning to the driver, but that's for another day.
James