osd patches for the v2.6.32 merge window

osd patches for the v2.6.32 merge window

[Boaz Harrosh]

James Hi

please submit the flowing patches into scsi-misc for
inclusion into the v2.6.32 Kernel.

These patches have been reviewed on the osd mailing-list and
sat in linux-next for a while. Some other trees will depend on
these changes.

Patches:
[PATCH 1/3] libosd: osd_dev_info: Unique Identification of an OSD device
[PATCH 2/3] libosd: osd_dev_is_ver1 - Minor API cleanup
[PATCH 3/3] libosd: osd_sense: OSD_CFO_PERMISSIONS

Thanks
Boaz

[PATCH 1/3] libosd: osd_dev_info: Unique Identification of an OSD device

[Boaz Harrosh]

Define an osd_dev_info structure that Uniquely identifies an OSD
device lun on the network. The identification is built from unique
target attributes and is the same for all network/SAN machines.

osduld_info_lookup() - NEW
    New API that will lookup an osd_dev by its osd_dev_info.
    This is used by pNFS-objects for cross network global device
    identification.

osduld_device_info() - NEW
    Given an osd_dev handle returns its associated osd_dev_info.
    This is used by exofs to encode the device information for
    network clients. (Get-device-info). The ULD fetches this
    information at startup and hangs it on each OSD device. (This is
    a fast operation that can be called at any condition)

osd_auto_detect_ver() - REVISED
    Now returns an osd_dev_info structure. Is only called once
    by ULD as before. See added comments for how to use.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   22 +++++++++++----
 drivers/scsi/osd/osd_uld.c       |   53 +++++++++++++++++++++++++++++++++++++-
 include/scsi/osd_initiator.h     |   37 +++++++++++++++++++++++---
 3 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 7a117c1..26e1b41 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -73,7 +73,8 @@ static const char *_osd_ver_desc(struct osd_request *or)
 
 #define ATTR_DEF_RI(id, len) ATTR_DEF(OSD_APAGE_ROOT_INFORMATION, id, len)
 
-static int _osd_print_system_info(struct osd_dev *od, void *caps)
+static int _osd_get_print_system_info(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi)
 {
 	struct osd_request *or;
 	struct osd_attr get_attrs[] = {
@@ -137,8 +138,13 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
 	OSD_INFO("PRODUCT_SERIAL_NUMBER  [%s]\n",
 		(char *)pFirst);
 
-	pFirst = get_attrs[a].val_ptr;
-	OSD_INFO("OSD_NAME               [%s]\n", (char *)pFirst);
+	odi->osdname_len = get_attrs[a].len;
+	if (odi->osdname_len) {
+		odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
+	} else
+		odi->osdname = NULL;
+	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
 	a++;
 
 	pFirst = get_attrs[a++].val_ptr;
@@ -171,6 +177,9 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
 				   sid_dump, sizeof(sid_dump), true);
 		OSD_INFO("OSD_SYSTEM_ID(%d)\n"
 			 "        [%s]\n", len, sid_dump);
+
+		odi->systemid_len = len;
+		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
 		a++;
 	}
 out:
@@ -178,16 +187,17 @@ out:
 	return ret;
 }
 
-int osd_auto_detect_ver(struct osd_dev *od, void *caps)
+int osd_auto_detect_ver(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi)
 {
 	int ret;
 
 	/* Auto-detect the osd version */
-	ret = _osd_print_system_info(od, caps);
+	ret = _osd_get_print_system_info(od, caps, odi);
 	if (ret) {
 		osd_dev_set_ver(od, OSD_VER1);
 		OSD_DEBUG("converting to OSD1\n");
-		ret = _osd_print_system_info(od, caps);
+		ret = _osd_get_print_system_info(od, caps, odi);
 	}
 
 	return ret;
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 0bdef33..8c069f9 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -87,6 +87,7 @@ struct osd_uld_device {
 	struct osd_dev od;
 	struct gendisk *disk;
 	struct device *class_member;
+	struct osd_dev_info odi;
 };
 
 static void __uld_get(struct osd_uld_device *oud);
@@ -216,6 +217,48 @@ free_od:
 }
 EXPORT_SYMBOL(osduld_path_lookup);
 
+static inline bool _the_same_or_null(const u8 *a1, unsigned a1_len,
+				     const u8 *a2, unsigned a2_len)
+{
+	if (!a2_len) /* User string is Empty means don't care */
+		return true;
+
+	if (a1_len != a2_len)
+		return false;
+
+	return 0 == memcmp(a1, a2, a1_len);
+}
+
+struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi)
+{
+	unsigned i;
+
+	for (i = 0; i < SCSI_OSD_MAX_MINOR; i++) {
+		char dev_str[16];
+		struct osd_uld_device *oud;
+		struct osd_dev *od;
+
+		sprintf(dev_str, "/dev/osd%d", i);
+		od = osduld_path_lookup(dev_str);
+		if (IS_ERR(od))
+			continue;
+
+		oud = od->file->private_data;
+		if (_the_same_or_null(oud->odi.systemid, oud->odi.systemid_len,
+				      odi->systemid, odi->systemid_len) &&
+		    _the_same_or_null(oud->odi.osdname, oud->odi.osdname_len,
+				      odi->osdname, odi->osdname_len)) {
+			OSD_DEBUG("found device sysid_len=%d osdname=%d\n",
+				  odi->systemid_len, odi->osdname_len);
+			return od;
+		}
+		osduld_put_device(od);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(osduld_info_lookup);
+
 void osduld_put_device(struct osd_dev *od)
 {
 
@@ -230,6 +273,13 @@ void osduld_put_device(struct osd_dev *od)
 }
 EXPORT_SYMBOL(osduld_put_device);
 
+const struct osd_dev_info *osduld_device_info(struct osd_dev *od)
+{
+	struct osd_uld_device *oud = od->file->private_data;
+	return &oud->odi;
+}
+EXPORT_SYMBOL(osduld_device_info);
+
 /*
  * Scsi Device operations
  */
@@ -250,7 +300,7 @@ static int __detect_osd(struct osd_uld_device *oud)
 		OSD_ERR("warning: scsi_test_unit_ready failed\n");
 
 	osd_sec_init_nosec_doall_caps(caps, &osd_root_object, false, true);
-	if (osd_auto_detect_ver(&oud->od, caps))
+	if (osd_auto_detect_ver(&oud->od, caps, &oud->odi))
 		return -ENODEV;
 
 	return 0;
@@ -402,6 +452,7 @@ static void __remove(struct kref *kref)
 		put_disk(oud->disk);
 
 	ida_remove(&osd_minor_ida, oud->minor);
+	kfree(oud->odi.osdname);
 	kfree(oud);
 }
 
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index 02bd9f7..0d29cc3 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -56,10 +56,23 @@ struct osd_dev {
 #endif
 };
 
-/* Retrieve/return osd_dev(s) for use by Kernel clients */
-struct osd_dev *osduld_path_lookup(const char *dev_name); /*Use IS_ERR/ERR_PTR*/
+/* Unique Identification of an OSD device */
+struct osd_dev_info {
+	unsigned systemid_len;
+	u8 systemid[OSD_SYSTEMID_LEN];
+	unsigned osdname_len;
+	u8 *osdname;
+};
+
+/* Retrieve/return osd_dev(s) for use by Kernel clients
+ * Use IS_ERR/ERR_PTR on returned "osd_dev *".
+ */
+struct osd_dev *osduld_path_lookup(const char *dev_name);
+struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi);
 void osduld_put_device(struct osd_dev *od);
 
+const struct osd_dev_info *osduld_device_info(struct osd_dev *od);
+
 /* Add/remove test ioctls from external modules */
 typedef int (do_test_fn)(struct osd_dev *od, unsigned cmd, unsigned long arg);
 int osduld_register_test(unsigned ioctl, do_test_fn *do_test);
@@ -69,8 +82,24 @@ void osduld_unregister_test(unsigned ioctl);
 void osd_dev_init(struct osd_dev *od, struct scsi_device *scsi_device);
 void osd_dev_fini(struct osd_dev *od);
 
-/* some hi level device operations */
-int osd_auto_detect_ver(struct osd_dev *od, void *caps);    /* GFP_KERNEL */
+/**
+ * osd_auto_detect_ver - Detect the OSD version, return Unique Identification
+ *
+ * @od:     OSD target lun handle
+ * @caps:   Capabilities authorizing OSD root read attributes access
+ * @odi:    Retrieved information uniquely identifying the osd target lun
+ *          Note: odi->osdname must be kfreed by caller.
+ *
+ * Auto detects the OSD version of the OSD target and sets the @od
+ * accordingly. Meanwhile also returns the "system id" and "osd name" root
+ * attributes which uniquely identify the OSD target. This member is usually
+ * called by the ULD. ULD users should call osduld_device_info().
+ * This rutine allocates osd requests and memory at GFP_KERNEL level and might
+ * sleep.
+ */
+int osd_auto_detect_ver(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi);
+
 static inline struct request_queue *osd_request_queue(struct osd_dev *od)
 {
 	return od->scsi_device->request_queue;
-- 
1.6.2.1

Re: [PATCH 1/3] libosd: osd_dev_info: Unique Identification of an OSD device

[James Bottomley]

On Mon, 2009-09-07 at 14:26 +0300, Boaz Harrosh wrote:
> Define an osd_dev_info structure that Uniquely identifies an OSD
> device lun on the network. The identification is built from unique
> target attributes and is the same for all network/SAN machines.
> 
> osduld_info_lookup() - NEW
>     New API that will lookup an osd_dev by its osd_dev_info.
>     This is used by pNFS-objects for cross network global device
>     identification.
> 
> osduld_device_info() - NEW
>     Given an osd_dev handle returns its associated osd_dev_info.
>     This is used by exofs to encode the device information for
>     network clients. (Get-device-info). The ULD fetches this
>     information at startup and hangs it on each OSD device. (This is
>     a fast operation that can be called at any condition)
> 
> osd_auto_detect_ver() - REVISED
>     Now returns an osd_dev_info structure. Is only called once
>     by ULD as before. See added comments for how to use.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/osd/osd_initiator.c |   22 +++++++++++----
>  drivers/scsi/osd/osd_uld.c       |   53 +++++++++++++++++++++++++++++++++++++-
>  include/scsi/osd_initiator.h     |   37 +++++++++++++++++++++++---
>  3 files changed, 101 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index 7a117c1..26e1b41 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -73,7 +73,8 @@ static const char *_osd_ver_desc(struct osd_request *or)
>  
>  #define ATTR_DEF_RI(id, len) ATTR_DEF(OSD_APAGE_ROOT_INFORMATION, id, len)
>  
> -static int _osd_print_system_info(struct osd_dev *od, void *caps)
> +static int _osd_get_print_system_info(struct osd_dev *od,
> +	void *caps, struct osd_dev_info *odi)
>  {
>  	struct osd_request *or;
>  	struct osd_attr get_attrs[] = {
> @@ -137,8 +138,13 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
>  	OSD_INFO("PRODUCT_SERIAL_NUMBER  [%s]\n",
>  		(char *)pFirst);
>  
> -	pFirst = get_attrs[a].val_ptr;
> -	OSD_INFO("OSD_NAME               [%s]\n", (char *)pFirst);
> +	odi->osdname_len = get_attrs[a].len;
> +	if (odi->osdname_len) {
> +		odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> +		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> +	} else
> +		odi->osdname = NULL;
> +	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
>  	a++;
>  
>  	pFirst = get_attrs[a++].val_ptr;
> @@ -171,6 +177,9 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
>  				   sid_dump, sizeof(sid_dump), true);
>  		OSD_INFO("OSD_SYSTEM_ID(%d)\n"
>  			 "        [%s]\n", len, sid_dump);
> +
> +		odi->systemid_len = len;
> +		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
>  		a++;
>  	}
>  out:
> @@ -178,16 +187,17 @@ out:
>  	return ret;
>  }
>  
> -int osd_auto_detect_ver(struct osd_dev *od, void *caps)
> +int osd_auto_detect_ver(struct osd_dev *od,
> +	void *caps, struct osd_dev_info *odi)
>  {
>  	int ret;
>  
>  	/* Auto-detect the osd version */
> -	ret = _osd_print_system_info(od, caps);
> +	ret = _osd_get_print_system_info(od, caps, odi);
>  	if (ret) {
>  		osd_dev_set_ver(od, OSD_VER1);
>  		OSD_DEBUG("converting to OSD1\n");
> -		ret = _osd_print_system_info(od, caps);
> +		ret = _osd_get_print_system_info(od, caps, odi);
>  	}
>  
>  	return ret;
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index 0bdef33..8c069f9 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -87,6 +87,7 @@ struct osd_uld_device {
>  	struct osd_dev od;
>  	struct gendisk *disk;
>  	struct device *class_member;
> +	struct osd_dev_info odi;
>  };
>  
>  static void __uld_get(struct osd_uld_device *oud);
> @@ -216,6 +217,48 @@ free_od:
>  }
>  EXPORT_SYMBOL(osduld_path_lookup);
>  
> +static inline bool _the_same_or_null(const u8 *a1, unsigned a1_len,
> +				     const u8 *a2, unsigned a2_len)
> +{
> +	if (!a2_len) /* User string is Empty means don't care */
> +		return true;
> +
> +	if (a1_len != a2_len)
> +		return false;
> +
> +	return 0 == memcmp(a1, a2, a1_len);
> +}
> +
> +struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < SCSI_OSD_MAX_MINOR; i++) {
> +		char dev_str[16];
> +		struct osd_uld_device *oud;
> +		struct osd_dev *od;
> +
> +		sprintf(dev_str, "/dev/osd%d", i);

Embedding specific forms user device paths into the kernel really looks
wrong here ... why are you doing this?  What happens if the user uses
udev to give them different names?

> +		od = osduld_path_lookup(dev_str);
> +		if (IS_ERR(od))
> +			continue;
> +
> +		oud = od->file->private_data;
> +		if (_the_same_or_null(oud->odi.systemid, oud->odi.systemid_len,
> +				      odi->systemid, odi->systemid_len) &&
> +		    _the_same_or_null(oud->odi.osdname, oud->odi.osdname_len,
> +				      odi->osdname, odi->osdname_len))

Why not NULL terminate these strings as you pick them out of the device
info?  Then you can dispense with the length and use standard kernel
string comparisons instead of rolling your own.

James

Re: [PATCH 1/3] libosd: osd_dev_info: Unique Identification of an OSD device

[Boaz Harrosh]

On 09/16/2009 08:47 PM, James Bottomley wrote:
> On Mon, 2009-09-07 at 14:26 +0300, Boaz Harrosh wrote:
>> Define an osd_dev_info structure that Uniquely identifies an OSD
>> device lun on the network. The identification is built from unique
>> target attributes and is the same for all network/SAN machines.
>>
>> osduld_info_lookup() - NEW
>>     New API that will lookup an osd_dev by its osd_dev_info.
>>     This is used by pNFS-objects for cross network global device
>>     identification.
>>
>> osduld_device_info() - NEW
>>     Given an osd_dev handle returns its associated osd_dev_info.
>>     This is used by exofs to encode the device information for
>>     network clients. (Get-device-info). The ULD fetches this
>>     information at startup and hangs it on each OSD device. (This is
>>     a fast operation that can be called at any condition)
>>
>> osd_auto_detect_ver() - REVISED
>>     Now returns an osd_dev_info structure. Is only called once
>>     by ULD as before. See added comments for how to use.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  drivers/scsi/osd/osd_initiator.c |   22 +++++++++++----
>>  drivers/scsi/osd/osd_uld.c       |   53 +++++++++++++++++++++++++++++++++++++-
>>  include/scsi/osd_initiator.h     |   37 +++++++++++++++++++++++---
>>  3 files changed, 101 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>> index 7a117c1..26e1b41 100644
>> --- a/drivers/scsi/osd/osd_initiator.c
>> +++ b/drivers/scsi/osd/osd_initiator.c
>> @@ -73,7 +73,8 @@ static const char *_osd_ver_desc(struct osd_request *or)
>>  
>>  #define ATTR_DEF_RI(id, len) ATTR_DEF(OSD_APAGE_ROOT_INFORMATION, id, len)
>>  
>> -static int _osd_print_system_info(struct osd_dev *od, void *caps)
>> +static int _osd_get_print_system_info(struct osd_dev *od,
>> +	void *caps, struct osd_dev_info *odi)
>>  {
>>  	struct osd_request *or;
>>  	struct osd_attr get_attrs[] = {
>> @@ -137,8 +138,13 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
>>  	OSD_INFO("PRODUCT_SERIAL_NUMBER  [%s]\n",
>>  		(char *)pFirst);
>>  
>> -	pFirst = get_attrs[a].val_ptr;
>> -	OSD_INFO("OSD_NAME               [%s]\n", (char *)pFirst);
>> +	odi->osdname_len = get_attrs[a].len;
>> +	if (odi->osdname_len) {
>> +		odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>> +		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>> +	} else
>> +		odi->osdname = NULL;
>> +	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
>>  	a++;
>>  
>>  	pFirst = get_attrs[a++].val_ptr;
>> @@ -171,6 +177,9 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
>>  				   sid_dump, sizeof(sid_dump), true);
>>  		OSD_INFO("OSD_SYSTEM_ID(%d)\n"
>>  			 "        [%s]\n", len, sid_dump);
>> +
>> +		odi->systemid_len = len;
>> +		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
>>  		a++;
>>  	}
>>  out:
>> @@ -178,16 +187,17 @@ out:
>>  	return ret;
>>  }
>>  
>> -int osd_auto_detect_ver(struct osd_dev *od, void *caps)
>> +int osd_auto_detect_ver(struct osd_dev *od,
>> +	void *caps, struct osd_dev_info *odi)
>>  {
>>  	int ret;
>>  
>>  	/* Auto-detect the osd version */
>> -	ret = _osd_print_system_info(od, caps);
>> +	ret = _osd_get_print_system_info(od, caps, odi);
>>  	if (ret) {
>>  		osd_dev_set_ver(od, OSD_VER1);
>>  		OSD_DEBUG("converting to OSD1\n");
>> -		ret = _osd_print_system_info(od, caps);
>> +		ret = _osd_get_print_system_info(od, caps, odi);
>>  	}
>>  
>>  	return ret;
>> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
>> index 0bdef33..8c069f9 100644
>> --- a/drivers/scsi/osd/osd_uld.c
>> +++ b/drivers/scsi/osd/osd_uld.c
>> @@ -87,6 +87,7 @@ struct osd_uld_device {
>>  	struct osd_dev od;
>>  	struct gendisk *disk;
>>  	struct device *class_member;
>> +	struct osd_dev_info odi;
>>  };
>>  
>>  static void __uld_get(struct osd_uld_device *oud);
>> @@ -216,6 +217,48 @@ free_od:
>>  }
>>  EXPORT_SYMBOL(osduld_path_lookup);
>>  
>> +static inline bool _the_same_or_null(const u8 *a1, unsigned a1_len,
>> +				     const u8 *a2, unsigned a2_len)
>> +{
>> +	if (!a2_len) /* User string is Empty means don't care */
>> +		return true;
>> +
>> +	if (a1_len != a2_len)
>> +		return false;
>> +
>> +	return 0 == memcmp(a1, a2, a1_len);
>> +}
>> +
>> +struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi)
>> +{
>> +	unsigned i;
>> +
>> +	for (i = 0; i < SCSI_OSD_MAX_MINOR; i++) {
>> +		char dev_str[16];
>> +		struct osd_uld_device *oud;
>> +		struct osd_dev *od;
>> +
>> +		sprintf(dev_str, "/dev/osd%d", i);
> 
> Embedding specific forms user device paths into the kernel really looks
> wrong here ... why are you doing this?  What happens if the user uses
> udev to give them different names?
> 
Two things

[1]
These names are created here below, exactly as shown. Now udev usually
creates more links and soft-links, I've never seen a delete?
(I agree that maybe a comment or a define could help readability as this
 format string is repeated twice, though slightly different).
Lets say that "udev can do anything but not delete the primary file created
by Kernel", as a prerequisite.

[2]
I could easily just add a global device list, like all the other parts of
the Kernel are trigger happy to shoot this problem.
But I hate it.
All these devices are already held on a Kernel structure called the name_space.
The life_time rules, locking, reference counting, and visibility, is bug-free and
tried out to the bone. Why should I have to reinvent the wheel? Why introduce all
these subtle corner cases, and sleepless debugging nights?

The way I see it Kernel keeps my devices for me and gives me a "Key" to fetch for
them. The key is the name.

That said. I'll do what people want, it is 5 minutes to put all these on a global
list_head. It's not lack of effort on my part.

>> +		od = osduld_path_lookup(dev_str);
>> +		if (IS_ERR(od))
>> +			continue;
>> +
>> +		oud = od->file->private_data;
>> +		if (_the_same_or_null(oud->odi.systemid, oud->odi.systemid_len,
>> +				      odi->systemid, odi->systemid_len) &&
>> +		    _the_same_or_null(oud->odi.osdname, oud->odi.osdname_len,
>> +				      odi->osdname, odi->osdname_len))
> 
> Why not NULL terminate these strings as you pick them out of the device
> info?  Then you can dispense with the length and use standard kernel
> string comparisons instead of rolling your own.
> 

No! these are, by osd definition, not char-strings they are large-binary signatures.

The system_id is defined by scsi, a 4-byte header + 16-bytes cryptographic key.
I use hex-dump to print it. It is set by the target device at the factory.

The osdname is user settable variable-length binary blob. To be used as a system
policy anyway chosen by the application. We are pushing, and that is what the
mkexofs--format is doing to conveniently store a uuid as generated with the uuidgen
utility in it's string, including "-" form. (36 asci chars). When loaded we allocate
an extra null-terminator (if needed) and print it as string every-where. So when worse
comes to worse and it is not a string, only the print-out is bogus. (But safe)
But when actually compering it for authentication, the pNFS standard mandates binary
blob exact match out.

> James
> 
> 

Thanks
Boaz

Re: [PATCH 1/3] libosd: osd_dev_info: Unique Identification of an OSD device

[Boaz Harrosh]

On 09/17/2009 02:28 PM, Boaz Harrosh wrote:
> On 09/16/2009 08:47 PM, James Bottomley wrote:
>>> +		sprintf(dev_str, "/dev/osd%d", i);
>>
>> Embedding specific forms user device paths into the kernel really looks
>> wrong here ... why are you doing this?  What happens if the user uses
>> udev to give them different names?
>>
> Two things
> 
> [1]
> These names are created here below, exactly as shown. Now udev usually
> creates more links and soft-links, I've never seen a delete?
> (I agree that maybe a comment or a define could help readability as this
>  format string is repeated twice, though slightly different).
> Lets say that "udev can do anything but not delete the primary file created
> by Kernel", as a prerequisite.
> 
> [2]
> I could easily just add a global device list, like all the other parts of
> the Kernel are trigger happy to shoot this problem.
> But I hate it.
> All these devices are already held on a Kernel structure called the name_space.
> The life_time rules, locking, reference counting, and visibility, is bug-free and
> tried out to the bone. Why should I have to reinvent the wheel? Why introduce all
> these subtle corner cases, and sleepless debugging nights?
> 
> The way I see it Kernel keeps my devices for me and gives me a "Key" to fetch for
> them. The key is the name.
> 
> That said. I'll do what people want, it is 5 minutes to put all these on a global
> list_head. It's not lack of effort on my part.
> 

I'm resending the same patch with minor changes:
1. Same defined format-string is now used both at creation of device
   and at look-up, so it is clear that these must and are the same.
2. I've added a comment to osduld_info_lookup that documents it's behavior
   So it should be clear from the get go and there are no surprises.

I'm using this code for a long time now, under a pNFS setups, and what can
I say, it does the job. 
Also I like the fact that I can use the same code as osduld_path_lookup()
and open a file handle on the device, which solved some bugs in the past.

Please consider for inclusion.

The other two trivial patches are independent please include those as well.

(I'll post as ver2 in reply to original patch)

Thanks
Boaz

[PATCH 1/3 version 2] libosd: osd_dev_info: Unique Identification of an OSD device

[Boaz Harrosh]

Define an osd_dev_info structure that Uniquely identifies an OSD
device lun on the network. The identification is built from unique
target attributes and is the same for all network/SAN machines.

osduld_info_lookup() - NEW
    New API that will lookup an osd_dev by its osd_dev_info.
    This is used by pNFS-objects for cross network global device
    identification.

osduld_device_info() - NEW
    Given an osd_dev handle returns its associated osd_dev_info.
    This is used by exofs to encode the device information for
    network clients. (Get-device-info). The ULD fetches this
    information at startup and hangs it on each OSD device. (This is
    a fast operation that can be called at any condition)

osd_auto_detect_ver() - REVISED
    Now returns an osd_dev_info structure. Is only called once
    by ULD as before. See added comments for how to use.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   22 +++++++++---
 drivers/scsi/osd/osd_uld.c       |   65 ++++++++++++++++++++++++++++++++++++-
 include/scsi/osd_initiator.h     |   37 +++++++++++++++++++--
 3 files changed, 112 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 7a117c1..26e1b41 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -73,7 +73,8 @@ static const char *_osd_ver_desc(struct osd_request *or)
 
 #define ATTR_DEF_RI(id, len) ATTR_DEF(OSD_APAGE_ROOT_INFORMATION, id, len)
 
-static int _osd_print_system_info(struct osd_dev *od, void *caps)
+static int _osd_get_print_system_info(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi)
 {
 	struct osd_request *or;
 	struct osd_attr get_attrs[] = {
@@ -137,8 +138,13 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
 	OSD_INFO("PRODUCT_SERIAL_NUMBER  [%s]\n",
 		(char *)pFirst);
 
-	pFirst = get_attrs[a].val_ptr;
-	OSD_INFO("OSD_NAME               [%s]\n", (char *)pFirst);
+	odi->osdname_len = get_attrs[a].len;
+	if (odi->osdname_len) {
+		odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
+	} else
+		odi->osdname = NULL;
+	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
 	a++;
 
 	pFirst = get_attrs[a++].val_ptr;
@@ -171,6 +177,9 @@ static int _osd_print_system_info(struct osd_dev *od, void *caps)
 				   sid_dump, sizeof(sid_dump), true);
 		OSD_INFO("OSD_SYSTEM_ID(%d)\n"
 			 "        [%s]\n", len, sid_dump);
+
+		odi->systemid_len = len;
+		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
 		a++;
 	}
 out:
@@ -178,16 +187,17 @@ out:
 	return ret;
 }
 
-int osd_auto_detect_ver(struct osd_dev *od, void *caps)
+int osd_auto_detect_ver(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi)
 {
 	int ret;
 
 	/* Auto-detect the osd version */
-	ret = _osd_print_system_info(od, caps);
+	ret = _osd_get_print_system_info(od, caps, odi);
 	if (ret) {
 		osd_dev_set_ver(od, OSD_VER1);
 		OSD_DEBUG("converting to OSD1\n");
-		ret = _osd_print_system_info(od, caps);
+		ret = _osd_get_print_system_info(od, caps, odi);
 	}
 
 	return ret;
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 0bdef33..7134257 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -80,6 +80,8 @@ MODULE_LICENSE("GPL");
 MODULE_ALIAS_CHARDEV_MAJOR(SCSI_OSD_MAJOR);
 MODULE_ALIAS_SCSI_DEVICE(TYPE_OSD);
 
+#define OSD_FMT_d "osd%d"
+
 struct osd_uld_device {
 	int minor;
 	struct kref kref;
@@ -87,6 +89,7 @@ struct osd_uld_device {
 	struct osd_dev od;
 	struct gendisk *disk;
 	struct device *class_member;
+	struct osd_dev_info odi;
 };
 
 static void __uld_get(struct osd_uld_device *oud);
@@ -216,6 +219,56 @@ free_od:
 }
 EXPORT_SYMBOL(osduld_path_lookup);
 
+static inline bool _the_same_or_null(const u8 *a1, unsigned a1_len,
+				     const u8 *a2, unsigned a2_len)
+{
+	if (!a2_len) /* User string is Empty means don't care */
+		return true;
+
+	if (a1_len != a2_len)
+		return false;
+
+	return 0 == memcmp(a1, a2, a1_len);
+}
+
+/* osduld_info_lookup - Loop through all devices, return the requested osd_dev.
+ *
+ * if @odi->systemid_len and/or @odi->osdname_len are zero, they act as a don't
+ * care. .e.g if they're both zero /dev/osd0 is returned.
+ * We are searching for /dev/osd%d in the name-space, so user-mode who would
+ * like to remove a device from the search can "mv /dev/osdX /my_devs/osdX" and
+ * the device will not be found.
+ */
+struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi)
+{
+	unsigned i;
+
+	for (i = 0; i < SCSI_OSD_MAX_MINOR; i++) {
+		char dev_str[16];
+		struct osd_uld_device *oud;
+		struct osd_dev *od;
+
+		sprintf(dev_str, "/dev/" OSD_FMT_d, i);
+		od = osduld_path_lookup(dev_str);
+		if (IS_ERR(od))
+			continue;
+
+		oud = od->file->private_data;
+		if (_the_same_or_null(oud->odi.systemid, oud->odi.systemid_len,
+				      odi->systemid, odi->systemid_len) &&
+		    _the_same_or_null(oud->odi.osdname, oud->odi.osdname_len,
+				      odi->osdname, odi->osdname_len)) {
+			OSD_DEBUG("found device sysid_len=%d osdname=%d\n",
+				  odi->systemid_len, odi->osdname_len);
+			return od;
+		}
+		osduld_put_device(od);
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(osduld_info_lookup);
+
 void osduld_put_device(struct osd_dev *od)
 {
 
@@ -230,6 +283,13 @@ void osduld_put_device(struct osd_dev *od)
 }
 EXPORT_SYMBOL(osduld_put_device);
 
+const struct osd_dev_info *osduld_device_info(struct osd_dev *od)
+{
+	struct osd_uld_device *oud = od->file->private_data;
+	return &oud->odi;
+}
+EXPORT_SYMBOL(osduld_device_info);
+
 /*
  * Scsi Device operations
  */
@@ -250,7 +310,7 @@ static int __detect_osd(struct osd_uld_device *oud)
 		OSD_ERR("warning: scsi_test_unit_ready failed\n");
 
 	osd_sec_init_nosec_doall_caps(caps, &osd_root_object, false, true);
-	if (osd_auto_detect_ver(&oud->od, caps))
+	if (osd_auto_detect_ver(&oud->od, caps, &oud->odi))
 		return -ENODEV;
 
 	return 0;
@@ -302,7 +362,7 @@ static int osd_probe(struct device *dev)
 	}
 	disk->major = SCSI_OSD_MAJOR;
 	disk->first_minor = oud->minor;
-	sprintf(disk->disk_name, "osd%d", oud->minor);
+	sprintf(disk->disk_name, OSD_FMT_d, oud->minor);
 	oud->disk = disk;
 
 	/* hold one more reference to the scsi_device that will get released
@@ -402,6 +462,7 @@ static void __remove(struct kref *kref)
 		put_disk(oud->disk);
 
 	ida_remove(&osd_minor_ida, oud->minor);
+	kfree(oud->odi.osdname);
 	kfree(oud);
 }
 
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index 02bd9f7..0d29cc3 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -56,10 +56,23 @@ struct osd_dev {
 #endif
 };
 
-/* Retrieve/return osd_dev(s) for use by Kernel clients */
-struct osd_dev *osduld_path_lookup(const char *dev_name); /*Use IS_ERR/ERR_PTR*/
+/* Unique Identification of an OSD device */
+struct osd_dev_info {
+	unsigned systemid_len;
+	u8 systemid[OSD_SYSTEMID_LEN];
+	unsigned osdname_len;
+	u8 *osdname;
+};
+
+/* Retrieve/return osd_dev(s) for use by Kernel clients
+ * Use IS_ERR/ERR_PTR on returned "osd_dev *".
+ */
+struct osd_dev *osduld_path_lookup(const char *dev_name);
+struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi);
 void osduld_put_device(struct osd_dev *od);
 
+const struct osd_dev_info *osduld_device_info(struct osd_dev *od);
+
 /* Add/remove test ioctls from external modules */
 typedef int (do_test_fn)(struct osd_dev *od, unsigned cmd, unsigned long arg);
 int osduld_register_test(unsigned ioctl, do_test_fn *do_test);
@@ -69,8 +82,24 @@ void osduld_unregister_test(unsigned ioctl);
 void osd_dev_init(struct osd_dev *od, struct scsi_device *scsi_device);
 void osd_dev_fini(struct osd_dev *od);
 
-/* some hi level device operations */
-int osd_auto_detect_ver(struct osd_dev *od, void *caps);    /* GFP_KERNEL */
+/**
+ * osd_auto_detect_ver - Detect the OSD version, return Unique Identification
+ *
+ * @od:     OSD target lun handle
+ * @caps:   Capabilities authorizing OSD root read attributes access
+ * @odi:    Retrieved information uniquely identifying the osd target lun
+ *          Note: odi->osdname must be kfreed by caller.
+ *
+ * Auto detects the OSD version of the OSD target and sets the @od
+ * accordingly. Meanwhile also returns the "system id" and "osd name" root
+ * attributes which uniquely identify the OSD target. This member is usually
+ * called by the ULD. ULD users should call osduld_device_info().
+ * This rutine allocates osd requests and memory at GFP_KERNEL level and might
+ * sleep.
+ */
+int osd_auto_detect_ver(struct osd_dev *od,
+	void *caps, struct osd_dev_info *odi);
+
 static inline struct request_queue *osd_request_queue(struct osd_dev *od)
 {
 	return od->scsi_device->request_queue;
-- 
1.6.2.1

[PATCH 2/3] libosd: osd_dev_is_ver1 - Minor API cleanup

[Boaz Harrosh]

define a new osd_dev_is_ver1 that operates on devices
and the old osd_req_is_ver1 uses that new API.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/scsi/osd_initiator.h |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index 0d29cc3..7bda109 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -113,6 +113,15 @@ static inline void osd_dev_set_ver(struct osd_dev *od, enum osd_std_version v)
 #endif
 }
 
+static inline bool osd_dev_is_ver1(struct osd_dev *od)
+{
+#ifdef OSD_VER1_SUPPORT
+	return od->version == OSD_VER1;
+#else
+	return false;
+#endif
+}
+
 struct osd_request;
 typedef void (osd_req_done_fn)(struct osd_request *or, void *private);
 
@@ -149,14 +158,9 @@ struct osd_request {
 	int async_error;
 };
 
-/* OSD Version control */
 static inline bool osd_req_is_ver1(struct osd_request *or)
 {
-#ifdef OSD_VER1_SUPPORT
-	return or->osd_dev->version == OSD_VER1;
-#else
-	return false;
-#endif
+	return osd_dev_is_ver1(or->osd_dev);
 }
 
 /*
-- 
1.6.2.1

[PATCH 3/3] libosd: osd_sense: OSD_CFO_PERMISSIONS

[Boaz Harrosh]

Add one more important cdb_field_offset that can be returned with
scsi_invalid_field_in_cdb. It is the offset of the permissions_bit_mask
field in the capabilities structure.

Interestingly, the offset is the same for V1/V2

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/scsi/osd_sense.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/scsi/osd_sense.h b/include/scsi/osd_sense.h
index ff9b33c..91db543 100644
--- a/include/scsi/osd_sense.h
+++ b/include/scsi/osd_sense.h
@@ -255,6 +255,9 @@ enum osdv2_cdb_field_offset {
 	OSD_CFO_STARTING_BYTE	= OSD_CDB_OFFSET(v2.start_address),
 	OSD_CFO_PARTITION_ID	= OSD_CDB_OFFSET(partition),
 	OSD_CFO_OBJECT_ID	= OSD_CDB_OFFSET(object),
+	OSD_CFO_PERMISSIONS	= sizeof(struct osd_cdb_head) +
+					offsetof(struct osd_capability_head,
+						 permissions_bit_mask),
 };
 
 #endif /* ndef __OSD_SENSE_H__ */
-- 
1.6.2.1