Hi Tregaron,
My apologies for the delayed follow-up on your patch.
On Fri, 2012-06-08 at 16:44 -0600, Tregaron Bayly wrote:
> I believe I recall seeing something like this discussed on the list a few weeks ago. SCST propagates the device handler name
> through to the initiator via the "model" field, whereas LIO uses a constant string passed from the backstore module - "IBLOCK",
> for instance. The advantage of passing the device handler name through is that if there are multiple luns exported from multiple
> targets it's simpler to use naming conventions to sort out where a lun may have come from and what it is for. Below is a patch I
> put together to override this constant with the config item name from the storage object and it seems to work in our environment.
>
> Before:
> [root@osd3 mnt]# lsscsi
> [0:0:0:0] disk AMCC 9650SE-24M DISK 4.10 /dev/sda
> [12:0:0:0] disk LIO-ORG IBLOCK 4.0 /dev/sdb
> [12:0:0:1] disk LIO-ORG IBLOCK 4.0 /dev/sdc
>
> After:
> [root@osd3 mnt]# lsscsi
> [0:0:0:0] disk AMCC 9650SE-24M DISK 4.10 /dev/sda
> [12:0:0:0] disk LIO-ORG san1_backup1 4.0 /dev/sdb
> [12:0:0:1] disk LIO-ORG san1_backup2 4.0 /dev/sdc
>
> Is this functionality possibly useful? Any pitfalls to this approach?
>
So I do really like the useability principle behind patch, but my main
concern is how this effects existing initiator's who may already be
depending upon the model area for INQUIRY for the SCSI LUN, and how
changing this for existing installs might result in unexpected
consequences.
> diff -durN linux-3.4.1.orig/drivers/target/target_core_transport.c linux-3.4.1/drivers/target/target_core_transport.c
> --- linux-3.4.1.orig/drivers/target/target_core_transport.c 2012-06-01 01:18:44.000000000 -0600
> +++ linux-3.4.1/drivers/target/target_core_transport.c 2012-06-08 14:46:49.807956878 -0600
> @@ -1327,6 +1327,13 @@
> {
> int force_pt;
> struct se_device *dev;
> + const char *configname;
> +
> + configname = config_item_name(&se_dev->se_dev_group.cg_item);
> + if ( configname != NULL) {
> + pr_debug("Changing vendorname from '%s' to '%s'\n", inquiry_prod, configname);
> + inquiry_prod = configname;
> + }
>
> dev = kzalloc(sizeof(struct se_device), GFP_KERNEL);
> if (!dev) {
> @@ -1416,6 +1423,12 @@
>
> strncpy(&dev->se_sub_dev->t10_wwn.vendor[0], "LIO-ORG", 8);
> strncpy(&dev->se_sub_dev->t10_wwn.model[0], inquiry_prod, 16);
> + /*
> + * inquiry_prod comes from the config item name, which is potentially
> + * longer than the 16 bytes copied by strncpy above. Ensure that we
> + * have a null terminator
> + */
> + dev->se_sub_dev->t10_wwn.model[15] = '\0';
> strncpy(&dev->se_sub_dev->t10_wwn.revision[0], inquiry_rev, 4);
> }
> scsi_dump_inquiry(dev);
>
What I think makes more sense is to turn this into an optional device
attribute in target_core_configfs.c that is disabled by default.
This will ensure that existing LUNs continue to function as normal, and
allow new LUNs to (optionally) use the model alias based upon ../$DEV/
configfs backend device name.
Care to respin a patch against target-pending/for-next that adds a
device attribute in:
/sys/kernel/config/target/core/$HBA/$DEV/attrib/emulate_model_alias
that accepts a '1' to enable or '0' to disable the feature..?
Thanks!
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
