osd: Revised patches for the 2.6.33 merge window

PATCH In review Boaz Harrosh 2 participants 17 messages Oct 26, 2009 – Nov 1, 2009 SCSI

lore

Share & tools

lore

Boaz Harrosh

author

2009-10-26 16:50 UTC #0 lore raw

cover osd: Revised patches for the 2.6.33 merge window

Hi James

I've revisited my last patches, and am submitting a new set. They address the concern you had that I was searching for devices by /dev/* names where actually user-mode can change those.

Additionally there is a related bugfix.

These are the patches

[PATCH 1/5] libosd: osd_dev_is_ver1 - Minor API cleanup [PATCH 2/5] libosd: osd_sense: OSD_CFO_PERMISSIONS
Same as before, please submit regardless.

[PATCH 3/5] osduld: Ref-counting bug fix
This is a bug fix. In the long past it used to work, by chance. With this fix it does what it's suppose to do. (Please submit)

[PATCH 4/5] osduld: Use device->release instead of internal kref [PATCH 5/5] libosd: osd_dev_info: Unique Identification of an OSD device
These two patches are new implementation to the API introduced in previous patchset. (Plus a related addition needed by multy-device exofs support).

It will now use the class_find_device() API to iterate through devices looking for the requested one. Using class_find_device forced a ref-counting shift. (Please review and submit)

I've tested these patches with multy-device exofs installation also exported to pnfs-objects clients (also in multi-device) and I'm satisfied with the results. There are no apparent new instabilities.

I would want to submit a substantial patchset to exofs, supporting multiple devices, mirror operations. The exofs patches are dependent on these patches to osd

[I'm putting these patches and a rebased set of exofs in linux-next. There should be
no problems merging once they get accepted into scsi-misc. As these patches are based on by exofs and pnfs-objects]

Thanks
Boaz

Boaz Harrosh

author

2009-10-26 16:56 UTC #1 lore raw

PATCH 1/5 libosd: osd_dev_is_ver1 - Minor API cleanup

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 <[email protected]>

---
 include/scsi/osd_initiator.h |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

include/scsi/osd_initiator.h +10 -6

index 02bd9f7..f787d24 100644
@@ -84,6 +84,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);

@@ -120,14 +129,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);
 }
 
 /*

Signature · 1 line

1.6.5.1

Boaz Harrosh

author

2009-10-26 16:57 UTC #2 lore raw

PATCH 2/5 libosd: osd_sense: OSD_CFO_PERMISSIONS

In reply to 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 <[email protected]>

---
 include/scsi/osd_sense.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

include/scsi/osd_sense.h +3 -0

index ff9b33c..91db543 100644
@@ -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__ */

Signature · 1 line

1.6.5.1

Boaz Harrosh

author

2009-10-26 16:57 UTC #3 lore raw

PATCH 3/5 osduld: Ref-counting bug fix

In reply to Boaz Harrosh

If scsi has released the device (logout), and exofs has last reference on the osduld_device it will be freed by osd_uld_release() within the call to fput(). But this will oops in cdev_release() which is called after the fops->release. (cdev is embedded within osduld_device). __uld_get/put pair makes sure we have a cdev for the duration of fput()

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/osd/osd_uld.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

drivers/scsi/osd/osd_uld.c +8 -0

index 0bdef33..1ea6447 100644
@@ -224,7 +224,15 @@ void osduld_put_device(struct osd_dev *od)
 
 		BUG_ON(od->scsi_device != oud->od.scsi_device);
 
+		/* If scsi has released the device (logout), and exofs has last
+		 * reference on oud it will be freed by above osd_uld_release
+		 * within fput below. But this will oops in cdev_release which
+		 * is called after the fops->release. __uld_get/put pair makes
+		 * sure we have a cdev for the duration of fput
+		 */
+		__uld_get(oud);
 		fput(od->file);
+		__uld_put(oud);
 		kfree(od);
 	}
 }

Signature · 1 line

1.6.5.1

Boaz Harrosh

author

2009-10-26 16:58 UTC #4 lore raw

PATCH 4/5 osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

The true logic of this patch will be clear in the next patch where we use the class_find_device???() API. When doing so the use of an internal kref leaves us a narrow window where a find is started while the actual object can go away. Using the device's kobj reference solves this problem because now the same kref is used for both operations. (Remove and find)

Some cleanups
* Use class register/unregister is cleaner for this driver now.
* Call create_device with the right parameters, and save calling set_drv_data() (It was not a bug just a side effect)
* cdev ref-counting games are no longer necessary

Core changes
* At create_device (in osd_probe), returned device->release is saved in an internal member and the final __remove function is now registered as the device->release function. (__Remove was moved in the file to avoid forward declaration)
* At __remove() chain to the old device->release() to de-allocate the device created by create_device.
* __uld_get/put is just device_get/put. Now every thing is accounted for on the device object.

I have incremented the device version string in case of new bugs.

Note: that previous bugfix of taking the reference around fput() still applies.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/osd/osd_uld.c   |  129 ++++++++++++++++++++++--------------------
 include/scsi/osd_initiator.h |    1 -
 2 files changed, 67 insertions(+), 63 deletions(-)

drivers/scsi/osd/osd_uld.c +67 ~~-62~~

index 1ea6447..7ec12ef 100644
@@ -71,8 +71,7 @@
 #define SCSI_OSD_MAX_MINOR 64
 
 static const char osd_name[] = "osd";
-static const char *osd_version_string = "open-osd 0.1.0";
-const char osd_symlink[] = "scsi_osd";
+static const char *osd_version_string = "open-osd 0.2.0";
 
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

@@ -80,18 +79,33 @@ 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;
 	struct cdev cdev;
 	struct osd_dev od;
 	struct gendisk *disk;
 	struct device *class_member;
+	typeof(((struct device *)NULL)->release) save_release;
 };
 
+struct osd_dev_handle {
+	struct osd_dev od;
+	struct file *file;
+	struct osd_uld_device *oud;
+} ;
+
+static DEFINE_IDA(osd_minor_ida);
+
 static void __uld_get(struct osd_uld_device *oud);
 static void __uld_put(struct osd_uld_device *oud);
 
+static struct class osd_uld_class = {
+	.owner		= THIS_MODULE,
+	.name		= "scsi_osd",
+};
+
 /*
  * Char Device operations
  */

@@ -177,7 +191,7 @@ static const struct file_operations osd_fops = {
 struct osd_dev *osduld_path_lookup(const char *name)
 {
 	struct osd_uld_device *oud;
-	struct osd_dev *od;
+	struct osd_dev_handle *odh;
 	struct file *file;
 	int error;

@@ -186,8 +200,8 @@ struct osd_dev *osduld_path_lookup(const char *name)
 		return ERR_PTR(-EINVAL);
 	}
 
-	od = kzalloc(sizeof(*od), GFP_KERNEL);
-	if (!od)
+	odh = kzalloc(sizeof(*odh), GFP_KERNEL);
+	if (unlikely(!odh))
 		return ERR_PTR(-ENOMEM);
 
 	file = filp_open(name, O_RDWR, 0);

@@ -203,24 +217,26 @@ struct osd_dev *osduld_path_lookup(const char *name)
 
 	oud = file->private_data;
 
-	*od = oud->od;
-	od->file = file;
+	odh->od = oud->od;
+	odh->file = file;
+	odh->oud = oud;
 
-	return od;
+	return &odh->od;
 
 close_file:
 	fput(file);
 free_od:
-	kfree(od);
+	kfree(odh);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL(osduld_path_lookup);
 
 void osduld_put_device(struct osd_dev *od)
 {
-
 	if (od && !IS_ERR(od)) {
-		struct osd_uld_device *oud = od->file->private_data;
+		struct osd_dev_handle *odh =
+				container_of(od, struct osd_dev_handle, od);
+		struct osd_uld_device *oud = odh->oud;
 
 		BUG_ON(od->scsi_device != oud->od.scsi_device);

@@ -231,9 +247,9 @@ void osduld_put_device(struct osd_dev *od)
 		 * sure we have a cdev for the duration of fput
 		 */
 		__uld_get(oud);
-		fput(od->file);
+		fput(odh->file);
 		__uld_put(oud);
-		kfree(od);
+		kfree(odh);
 	}
 }
 EXPORT_SYMBOL(osduld_put_device);

@@ -264,8 +280,28 @@ static int __detect_osd(struct osd_uld_device *oud)
 	return 0;
 }
 
-static struct class *osd_sysfs_class;
-static DEFINE_IDA(osd_minor_ida);
+static void __remove(struct device *dev)
+{
+	struct osd_uld_device *oud = dev_get_drvdata(dev);
+	struct scsi_device *scsi_device = oud->od.scsi_device;
+
+	oud->save_release(oud->class_member);
+
+	if (oud->cdev.owner)
+		cdev_del(&oud->cdev);
+
+	osd_dev_fini(&oud->od);
+	scsi_device_put(scsi_device);
+
+	OSD_INFO("osd_remove %s\n",
+		 oud->disk ? oud->disk->disk_name : NULL);
+
+	if (oud->disk)
+		put_disk(oud->disk);
+	ida_remove(&osd_minor_ida, oud->minor);
+
+	kfree(oud);
+}
 
 static int osd_probe(struct device *dev)
 {

@@ -297,7 +333,6 @@ static int osd_probe(struct device *dev)
 	if (NULL == oud)
 		goto err_retract_minor;
 
-	kref_init(&oud->kref);
 	dev_set_drvdata(dev, oud);
 	oud->minor = minor;

@@ -310,7 +345,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

@@ -335,18 +370,19 @@ static int osd_probe(struct device *dev)
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
-	kobject_get(&oud->cdev.kobj); /* 2nd ref see osd_remove() */
 
 	/* class_member */
-	oud->class_member = device_create(osd_sysfs_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), "%s", disk->disk_name);
+	oud->class_member = device_create(&osd_uld_class, dev,
+		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
 	if (IS_ERR(oud->class_member)) {
 		OSD_ERR("class_device_create failed\n");
 		error = PTR_ERR(oud->class_member);
 		goto err_put_cdev;
 	}
+	oud->save_release = oud->class_member->release;
+	oud->class_member->release = __remove;
 
-	dev_set_drvdata(oud->class_member, oud);
+	__uld_get(oud);
 
 	OSD_INFO("osd_probe %s\n", disk->disk_name);
 	return 0;

@@ -376,51 +412,21 @@ static int osd_remove(struct device *dev)
 	}
 
 	if (oud->class_member)
-		device_destroy(osd_sysfs_class,
+		device_destroy(&osd_uld_class,
 			       MKDEV(SCSI_OSD_MAJOR, oud->minor));
 
-	/* We have 2 references to the cdev. One is released here
-	 * and also takes down the /dev/osdX mapping. The second
-	 * Will be released in __remove() after all users have released
-	 * the osd_uld_device.
-	 */
-	if (oud->cdev.owner)
-		cdev_del(&oud->cdev);
-
 	__uld_put(oud);
 	return 0;
 }
 
-static void __remove(struct kref *kref)
-{
-	struct osd_uld_device *oud = container_of(kref,
-					struct osd_uld_device, kref);
-	struct scsi_device *scsi_device = oud->od.scsi_device;
-
-	/* now let delete the char_dev */
-	kobject_put(&oud->cdev.kobj);
-
-	osd_dev_fini(&oud->od);
-	scsi_device_put(scsi_device);
-
-	OSD_INFO("osd_remove %s\n",
-		 oud->disk ? oud->disk->disk_name : NULL);
-
-	if (oud->disk)
-		put_disk(oud->disk);
-
-	ida_remove(&osd_minor_ida, oud->minor);
-	kfree(oud);
-}
-
 static void __uld_get(struct osd_uld_device *oud)
 {
-	kref_get(&oud->kref);
+	get_device(oud->class_member);
 }
 
 static void __uld_put(struct osd_uld_device *oud)
 {
-	kref_put(&oud->kref, __remove);
+	put_device(oud->class_member);
 }
 
 /*

@@ -440,11 +446,10 @@ static int __init osd_uld_init(void)
 {
 	int err;
 
-	osd_sysfs_class = class_create(THIS_MODULE, osd_symlink);
-	if (IS_ERR(osd_sysfs_class)) {
-		OSD_ERR("Unable to register sysfs class => %ld\n",
-			PTR_ERR(osd_sysfs_class));
-		return PTR_ERR(osd_sysfs_class);
+	err = class_register(&osd_uld_class);
+	if (err) {
+		OSD_ERR("Unable to register sysfs class => %d\n", err);
+		return err;
 	}
 
 	err = register_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0),

@@ -467,7 +472,7 @@ static int __init osd_uld_init(void)
 err_out_chrdev:
 	unregister_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0), SCSI_OSD_MAX_MINOR);
 err_out:
-	class_destroy(osd_sysfs_class);
+	class_unregister(&osd_uld_class);
 	return err;
 }

@@ -475,7 +480,7 @@ static void __exit osd_uld_exit(void)
 {
 	scsi_unregister_driver(&osd_driver.gendrv);
 	unregister_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0), SCSI_OSD_MAX_MINOR);
-	class_destroy(osd_sysfs_class);
+	class_unregister(&osd_uld_class);
 	OSD_INFO("UNLOADED %s\n", osd_version_string);
 }

include/scsi/osd_initiator.h +0 -1

index f787d24..589e5f0 100644
@@ -48,7 +48,6 @@ enum osd_std_version {
  */
 struct osd_dev {
 	struct scsi_device *scsi_device;
-	struct file *file;
 	unsigned def_timeout;
 
 #ifdef OSD_VER1_SUPPORT

Signature · 1 line

1.6.5.1

Boaz Harrosh

author

2009-10-26 16:58 UTC #15 lore raw

PATCH 5/5 libosd: osd_dev_info: Unique Identification of an OSD device

In reply to 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. And by exofs multy-device support, the device info is specified in the on-disk exofs device table.

osduld_device_info() - NEW
Given an osd_dev handle returns its associated osd_dev_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)

osduld_device_same() - NEW
With a given osd_dev at one hand and an osd_dev_info at another, we would like to know if they are the same device.
Two osd_dev handles can be checked by:
osduld_device_same(od1, osduld_device_info(od2));

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 <[email protected]>

---
 drivers/scsi/osd/osd_initiator.c |   26 ++++++++--
 drivers/scsi/osd/osd_uld.c       |   97 +++++++++++++++++++++++++++++++++++++-
 include/scsi/osd_initiator.h     |   38 +++++++++++++--
 3 files changed, 149 insertions(+), 12 deletions(-)

drivers/scsi/osd/osd_initiator.c +20 -6

index 7a117c1..60b7ca1 100644
@@ -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,12 @@ 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;
+	/* Avoid NULL for memcmp optimization 0-length is good enough */
+	odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+	if (odi->osdname_len)
+		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
+	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
 	a++;
 
 	pFirst = get_attrs[a++].val_ptr;

@@ -171,6 +176,14 @@ 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);
+
+		if (unlikely(len > sizeof(odi->systemid))) {
+			OSD_ERR("OSD Target error: OSD_SYSTEM_ID too long(%d). "
+				"device idetification might not work\n", len);
+			len = sizeof(odi->systemid);
+		}
+		odi->systemid_len = len;
+		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
 		a++;
 	}
 out:

@@ -178,16 +191,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;

drivers/scsi/osd/osd_uld.c +95 -2

133 lines not on this page · raw message

include/scsi/osd_initiator.h +34 -4

index 589e5f0..3ec346e 100644
@@ -55,10 +55,24 @@ 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);
+bool osduld_device_same(struct osd_dev *od, const struct osd_dev_info *odi);
+
 /* 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);

@@ -68,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;

Signature · 1 line

1.6.5.1

James Bottomley

maintainer

2009-10-29 17:11 UTC #5 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

Quoted 8 lines · Boaz Harrosh

drivers/scsi/osd/osd_uld.c +0 -2

@@ -335,18 +370,19 @@ static int osd_probe(struct device *dev)
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
-	kobject_get(&oud->cdev.kobj); /* 2nd ref see osd_remove() */
 
 	/* class_member */
-	oud->class_member = device_create(osd_sysfs_class, dev,

drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 383

Quoted 10 lines

drivers/scsi/osd/osd_uld.c +4 -1

-		MKDEV(SCSI_OSD_MAJOR, oud->minor), "%s", disk->disk_name);
+	oud->class_member = device_create(&osd_uld_class, dev,
+		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
 	if (IS_ERR(oud->class_member)) {
 		OSD_ERR("class_device_create failed\n");
 		error = PTR_ERR(oud->class_member);
 		goto err_put_cdev;
 	}
+	oud->save_release = oud->class_member->release;
+	oud->class_member->release = __remove;

What exactly is the reason for this iffy manipulation? Can't this be done properly by adding a dev_release member to osd_uld_class without this unnecessary and improper function chaining?

James

Boaz Harrosh

author

2009-10-29 17:24 UTC #6 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

Quoted 7 lines · James Bottomley

Quoted 6 lines · Boaz Harrosh

drivers/scsi/osd/osd_uld.c +0 -1

@@ -335,18 +370,19 @@ static int osd_probe(struct device *dev)
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
-	kobject_get(&oud->cdev.kobj); /* 2nd ref see osd_remove() */

Quoted 17 lines

drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 383
Quoted 12 lines
drivers/scsi/osd/osd_uld.c +4 -2
 	/* class_member */
-	oud->class_member = device_create(osd_sysfs_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), "%s", disk->disk_name);
+	oud->class_member = device_create(&osd_uld_class, dev,
+		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
 	if (IS_ERR(oud->class_member)) {
 		OSD_ERR("class_device_create failed\n");
 		error = PTR_ERR(oud->class_member);
 		goto err_put_cdev;
 	}
+	oud->save_release = oud->class_member->release;
+	oud->class_member->release = __remove;
What exactly is the reason for this iffy manipulation? Can't this be done properly by adding a dev_release member to osd_uld_class without this unnecessary and improper function chaining?

I tried, it does not work if I use device_create.

because in device_release(struct kobject *kobj) it has ruffly this:

	if (dev->release)
		dev->release(dev);
	else if (class->dev_release)
		class->dev_release(dev);

(See core.c:110)

so since device_create sets a dev->release = it masks out my class->dev_release

Now I have two choice. Above release chaining, or duplicate the 9 lines of code done in device_create() before the call to device_register. I prefer the first since it is more resilient to future core changes. (And it is less code lines)

Quoted 3 lines

James

Boaz

James Bottomley

maintainer

2009-10-29 17:41 UTC #7 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

Quoted 34 lines · Boaz Harrosh

Quoted 7 lines · James Bottomley
Quoted 6 lines · Boaz Harrosh
drivers/scsi/osd/osd_uld.c +0 -1
@@ -335,18 +370,19 @@ static int osd_probe(struct device *dev)
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
-	kobject_get(&oud->cdev.kobj); /* 2nd ref see osd_remove() */
 
Quoted 17 lines
drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 383
Quoted 12 lines
drivers/scsi/osd/osd_uld.c +4 -2
 	/* class_member */
-	oud->class_member = device_create(osd_sysfs_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), "%s", disk->disk_name);
+	oud->class_member = device_create(&osd_uld_class, dev,
+		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
 	if (IS_ERR(oud->class_member)) {
 		OSD_ERR("class_device_create failed\n");
 		error = PTR_ERR(oud->class_member);
 		goto err_put_cdev;
 	}
+	oud->save_release = oud->class_member->release;
+	oud->class_member->release = __remove;
What exactly is the reason for this iffy manipulation? Can't this be done properly by adding a dev_release member to osd_uld_class without this unnecessary and improper function chaining?
I tried, it does not work if I use device_create.
because in device_release(struct kobject *kobj) it has ruffly this:
	if (dev->release)
		dev->release(dev);
	else if (class->dev_release)
		class->dev_release(dev);

Quoted 7 lines

(See core.c:110)
so since device_create sets a dev->release = it masks out my class->dev_release
Now I have two choice. Above release chaining, or duplicate the 9 lines of code done in device_create() before the call to device_register. I prefer the first since it is more resilient to future core changes. (And it is less code lines)

Chaining methods like this because of inner knowledge of the implementation isn't resilient, it's very fragile.

Isn't the correct answer to embed your device in struct osd_uld_device? They both look to have identical lifetimes and the release rules can then handle the dual destruction?

James

Boaz Harrosh

author

2009-10-29 17:50 UTC #8 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

Some thing like below.

There is to much knolage of device in my opinion. To much fidling of private device members. What when some new members are added?

I hate it.

(compile tested only)
---
git diff --stat -p drivers/scsi/osd/osd_uld.c

 drivers/scsi/osd/osd_uld.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

drivers/scsi/osd/osd_uld.c +18 ~~-17~~

index 25ef993..5f308be 100644
@@ -87,7 +87,7 @@ struct osd_uld_device {
 	struct osd_dev od;
 	struct osd_dev_info odi;
 	struct gendisk *disk;
-	struct device *class_member;
+	struct device class_dev;
 	typeof(((struct device *)NULL)->release) save_release;
 };

@@ -316,7 +316,7 @@ void osduld_put_device(struct osd_dev *od)
 		if (odh->file)
 			fput(odh->file);
 		else
-			put_device(oud->class_member);
+			put_device(&oud->class_dev);
 		__uld_put(oud);
 		kfree(odh);
 	}

@@ -378,7 +378,6 @@ static void __remove(struct device *dev)
 	struct scsi_device *scsi_device = oud->od.scsi_device;
 
 	kfree(oud->odi.osdname);
-	oud->save_release(oud->class_member);
 
 	if (oud->cdev.owner)
 		cdev_del(&oud->cdev);

@@ -464,16 +463,20 @@ static int osd_probe(struct device *dev)
 		goto err_put_disk;
 	}
 
-	/* class_member */
-	oud->class_member = device_create(&osd_uld_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
-	if (IS_ERR(oud->class_member)) {
-		OSD_ERR("class_device_create failed\n");
-		error = PTR_ERR(oud->class_member);
+	/* class device member */
+	oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
+	oud->class_dev.class = &osd_uld_class;
+	oud->class_dev.parent = dev;
+	oud->class_dev.release = __remove;
+	dev_set_drvdata(&oud->class_dev, oud);
+
+	error = kobject_set_name(&oud->class_dev.kobj, disk->disk_name);
+	if (error)
+		goto err_put_cdev;
+
+	error = device_register(&oud->class_dev);
+	if (error)
 		goto err_put_cdev;
-	}
-	oud->save_release = oud->class_member->release;
-	oud->class_member->release = __remove;
 
 	__uld_get(oud);

@@ -504,9 +507,7 @@ static int osd_remove(struct device *dev)
 			scsi_device);
 	}
 
-	if (oud->class_member)
-		device_destroy(&osd_uld_class,
-			       MKDEV(SCSI_OSD_MAJOR, oud->minor));
+	device_unregister(&oud->class_dev);
 
 	__uld_put(oud);
 	return 0;

@@ -514,12 +515,12 @@ static int osd_remove(struct device *dev)
 
 static void __uld_get(struct osd_uld_device *oud)
 {
-	get_device(oud->class_member);
+	get_device(&oud->class_dev);
 }
 
 static void __uld_put(struct osd_uld_device *oud)
 {
-	put_device(oud->class_member);
+	put_device(&oud->class_dev);
 }
 
 /*

Boaz Harrosh

author

2009-10-29 17:58 UTC #12 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to James Bottomley

Quoted 4 lines · James Bottomley

Chaining methods like this because of inner knowledge of the implementation isn't resilient, it's very fragile.

If I would just kfree, because I know the inner code, that would be fragile.

But overriding a destructor, do what you need, and call previous distroctor. Does not take any inner knowledge. Just the published fact that it is a distructor, which will destroy the object.

Quoted 4 lines

Isn't the correct answer to embed your device in struct osd_uld_device? They both look to have identical lifetimes and the release rules can then handle the dual destruction?

See the patch. there I have direct manipulation with inner members. The chaining is more resilient, i think.

Quoted 3 lines

James

Boaz

James Bottomley

maintainer

2009-10-29 18:03 UTC #9 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

Quoted 4 lines · Boaz Harrosh

Some thing like below.
There is to much knolage of device in my opinion. To much fidling of private device members. What when some new members are added?

What private device members ... the pieces you fill in are the public ones. If you want a macro to do that, I'm sure Greg would be amenable ...

Quoted 1 line

I hate it.

Because you have to type six more lines of code? To me it looks cleaner because there's only a single allocation instead of two and now no need of chaining destructors because everything goes together thus making the lifetime rules more obvious and robust.

It also saves memory because it has two fewer pointers.

James

Quoted 99 lines

(compile tested only)
---
git diff --stat -p drivers/scsi/osd/osd_uld.c

 drivers/scsi/osd/osd_uld.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

drivers/scsi/osd/osd_uld.c +18 ~~-18~~

index 25ef993..5f308be 100644
@@ -87,7 +87,7 @@ struct osd_uld_device {
 	struct osd_dev od;
 	struct osd_dev_info odi;
 	struct gendisk *disk;
-	struct device *class_member;
+	struct device class_dev;
 	typeof(((struct device *)NULL)->release) save_release;
 };

@@ -316,7 +316,7 @@ void osduld_put_device(struct osd_dev *od)
 		if (odh->file)
 			fput(odh->file);
 		else
-			put_device(oud->class_member);
+			put_device(&oud->class_dev);
 		__uld_put(oud);
 		kfree(odh);
 	}

@@ -378,7 +378,6 @@ static void __remove(struct device *dev)
 	struct scsi_device *scsi_device = oud->od.scsi_device;
 
 	kfree(oud->odi.osdname);
-	oud->save_release(oud->class_member);
 
 	if (oud->cdev.owner)
 		cdev_del(&oud->cdev);

@@ -464,16 +463,20 @@ static int osd_probe(struct device *dev)
 		goto err_put_disk;
 	}
 
-	/* class_member */
-	oud->class_member = device_create(&osd_uld_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
-	if (IS_ERR(oud->class_member)) {
-		OSD_ERR("class_device_create failed\n");
-		error = PTR_ERR(oud->class_member);
+	/* class device member */
+	oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
+	oud->class_dev.class = &osd_uld_class;
+	oud->class_dev.parent = dev;
+	oud->class_dev.release = __remove;
+	dev_set_drvdata(&oud->class_dev, oud);
+
+	error = kobject_set_name(&oud->class_dev.kobj, disk->disk_name);
+	if (error)
+		goto err_put_cdev;
+
+	error = device_register(&oud->class_dev);
+	if (error)
 		goto err_put_cdev;
-	}
-	oud->save_release = oud->class_member->release;
-	oud->class_member->release = __remove;
 
 	__uld_get(oud);

@@ -504,9 +507,7 @@ static int osd_remove(struct device *dev)
 			scsi_device);
 	}
 
-	if (oud->class_member)
-		device_destroy(&osd_uld_class,
-			       MKDEV(SCSI_OSD_MAJOR, oud->minor));
+	device_unregister(&oud->class_dev);
 
 	__uld_put(oud);
 	return 0;

@@ -514,12 +515,12 @@ static int osd_remove(struct device *dev)
 
 static void __uld_get(struct osd_uld_device *oud)
 {
-	get_device(oud->class_member);
+	get_device(&oud->class_dev);
 }
 
 static void __uld_put(struct osd_uld_device *oud)
 {
-	put_device(oud->class_member);
+	put_device(&oud->class_dev);
 }
 
 /*
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Boaz Harrosh

author

2009-10-29 18:05 UTC #11 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

Quoted 54 lines · Boaz Harrosh

Some thing like below.

There is to much knolage of device in my opinion. To much fidling of private device members. What when some new members are added?

I hate it.

(compile tested only)
---
git diff --stat -p drivers/scsi/osd/osd_uld.c

 drivers/scsi/osd/osd_uld.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

drivers/scsi/osd/osd_uld.c +3 -9

index 25ef993..5f308be 100644
@@ -87,7 +87,7 @@ struct osd_uld_device {
 	struct osd_dev od;
 	struct osd_dev_info odi;
 	struct gendisk *disk;
-	struct device *class_member;
+	struct device class_dev;
 	typeof(((struct device *)NULL)->release) save_release;
 };

@@ -316,7 +316,7 @@ void osduld_put_device(struct osd_dev *od)
 		if (odh->file)
 			fput(odh->file);
 		else
-			put_device(oud->class_member);
+			put_device(&oud->class_dev);
 		__uld_put(oud);
 		kfree(odh);
 	}

@@ -378,7 +378,6 @@ static void __remove(struct device *dev)
 	struct scsi_device *scsi_device = oud->od.scsi_device;
 
 	kfree(oud->odi.osdname);
-	oud->save_release(oud->class_member);
 
 	if (oud->cdev.owner)
 		cdev_del(&oud->cdev);

@@ -464,16 +463,20 @@ static int osd_probe(struct device *dev)
 		goto err_put_disk;
 	}
 
-	/* class_member */
-	oud->class_member = device_create(&osd_uld_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), oud, disk->disk_name);
-	if (IS_ERR(oud->class_member)) {
-		OSD_ERR("class_device_create failed\n");
-		error = PTR_ERR(oud->class_member);
+	/* class device member */

drivers/scsi/osd/osd_uld.c static int osd_probe(struct device *dev) · line 476

Quoted 10 lines

drivers/scsi/osd/osd_uld.c +10 -0

+	oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
+	oud->class_dev.class = &osd_uld_class;
+	oud->class_dev.parent = dev;
+	oud->class_dev.release = __remove;
+	dev_set_drvdata(&oud->class_dev, oud);
+
+	error = kobject_set_name(&oud->class_dev.kobj, disk->disk_name);
+	if (error)
+		goto err_put_cdev;
+

With cdev (above) I have "cdev_init(&oud->cdev, &osd_fops);" If I would only have a:

    device_init(&osd_uld_class, dev,MKDEV(SCSI_OSD_MAJOR, oud->minor),
		oud, disk->disk_name);

That does these code lines above I would feel much more comfortable

Boaz

Quoted 38 lines

drivers/scsi/osd/osd_uld.c +5 -8

+	error = device_register(&oud->class_dev);
+	if (error)
 		goto err_put_cdev;
-	}
-	oud->save_release = oud->class_member->release;
-	oud->class_member->release = __remove;
 
 	__uld_get(oud);

@@ -504,9 +507,7 @@ static int osd_remove(struct device *dev)
 			scsi_device);
 	}
 
-	if (oud->class_member)
-		device_destroy(&osd_uld_class,
-			       MKDEV(SCSI_OSD_MAJOR, oud->minor));
+	device_unregister(&oud->class_dev);
 
 	__uld_put(oud);
 	return 0;

@@ -514,12 +515,12 @@ static int osd_remove(struct device *dev)
 
 static void __uld_get(struct osd_uld_device *oud)
 {
-	get_device(oud->class_member);
+	get_device(&oud->class_dev);
 }
 
 static void __uld_put(struct osd_uld_device *oud)
 {
-	put_device(oud->class_member);
+	put_device(&oud->class_dev);
 }
 
 /*

James Bottomley

maintainer

2009-10-29 18:06 UTC #13 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

Quoted 6 lines · Boaz Harrosh

Quoted 4 lines · James Bottomley
Chaining methods like this because of inner knowledge of the implementation isn't resilient, it's very fragile.

Quoted 6 lines

If I would just kfree, because I know the inner code, that would be fragile.
But overriding a destructor, do what you need, and call previous distroctor. Does not take any inner knowledge. Just the published fact that it is a distructor, which will destroy the object.

You can't even justify this on OO grounds: In OO code, you get this override by extending the object not hijacking the method and arbitrarily linking two separate objects.

Embedding the device is the C equivalent of the OO object extension.

Method hijacks are almost always wrong.

James

Boaz Harrosh

author

2009-10-29 18:10 UTC #10 lore raw

Re: [PATCH 4/5] osduld: Use device->release instead of internal kref

In reply to James Bottomley

Quoted 12 lines · James Bottomley

Quoted 4 lines · Boaz Harrosh
Some thing like below.
There is to much knolage of device in my opinion. To much fidling of private device members. What when some new members are added?
What private device members ... the pieces you fill in are the public ones. If you want a macro to do that, I'm sure Greg would be amenable ...
Quoted 1 line
I hate it.

Quoted 9 lines

Because you have to type six more lines of code? To me it looks cleaner because there's only a single allocation instead of two and now no need of chaining destructors because everything goes together thus making the lifetime rules more obvious and robust.
It also saves memory because it has two fewer pointers.
James

OK I'll roll up a patch that introduces a device_init helper and a patch for Greg that exchanges my helper with an export.

But I will have to test it so only next week.

Thanks for the review. Is that the only thing?

Boaz

Boaz Harrosh

author

2009-11-01 16:43 UTC #14 lore raw

PATCH 4/5 osduld: Use device->release instead of internal kref

In reply to Boaz Harrosh

The true logic of this patch will be clear in the next patch where we use the class_find_device() API. When doing so the use of an internal kref leaves us a narrow window where a find is started while the actual object can go away. Using the device's kobj reference solves this problem because now the same kref is used for both operations. (Remove and find)

Core changes
* Embed a struct device in uld_ structure and use device_register instead of devie_create. Set __remove to be the device release function.
* __uld_get/put is just get_/put_device. Now everything is accounted for on the device object. Internal kref is removed.
* At __remove() we can safely de-allocate the uld_ structure. (The function was moved to avoid forward declaration)

Some cleanups
* Use class register/unregister is cleaner for this driver now.
* cdev ref-counting games are no longer necessary

I have incremented the device version string in case of new bugs.

Note: Previous bugfix of taking the reference around fput() still
applies.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/osd/osd_uld.c   |  162 ++++++++++++++++++++----------------------
 include/scsi/osd_initiator.h |    1 -
 2 files changed, 77 insertions(+), 86 deletions(-)

drivers/scsi/osd/osd_uld.c +77 ~~-85~~

index 1ea6447..9dd5948 100644
@@ -71,8 +71,7 @@
 #define SCSI_OSD_MAX_MINOR 64
 
 static const char osd_name[] = "osd";
-static const char *osd_version_string = "open-osd 0.1.0";
-const char osd_symlink[] = "scsi_osd";
+static const char *osd_version_string = "open-osd 0.2.0";
 
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

@@ -82,15 +81,24 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_OSD);
 
 struct osd_uld_device {
 	int minor;
-	struct kref kref;
+	struct device class_dev;
 	struct cdev cdev;
 	struct osd_dev od;
 	struct gendisk *disk;
-	struct device *class_member;
 };
 
-static void __uld_get(struct osd_uld_device *oud);
-static void __uld_put(struct osd_uld_device *oud);
+struct osd_dev_handle {
+	struct osd_dev od;
+	struct file *file;
+	struct osd_uld_device *oud;
+} ;
+
+static DEFINE_IDA(osd_minor_ida);
+
+static struct class osd_uld_class = {
+	.owner		= THIS_MODULE,
+	.name		= "scsi_osd",
+};
 
 /*
  * Char Device operations

@@ -101,7 +109,7 @@ static int osd_uld_open(struct inode *inode, struct file *file)
 	struct osd_uld_device *oud = container_of(inode->i_cdev,
 					struct osd_uld_device, cdev);
 
-	__uld_get(oud);
+	get_device(&oud->class_dev);
 	/* cache osd_uld_device on file handle */
 	file->private_data = oud;
 	OSD_DEBUG("osd_uld_open %p\n", oud);

@@ -114,7 +122,7 @@ static int osd_uld_release(struct inode *inode, struct file *file)
 
 	OSD_DEBUG("osd_uld_release %p\n", file->private_data);
 	file->private_data = NULL;
-	__uld_put(oud);
+	put_device(&oud->class_dev);
 	return 0;
 }

@@ -177,7 +185,7 @@ static const struct file_operations osd_fops = {
 struct osd_dev *osduld_path_lookup(const char *name)
 {
 	struct osd_uld_device *oud;
-	struct osd_dev *od;
+	struct osd_dev_handle *odh;
 	struct file *file;
 	int error;

@@ -186,8 +194,8 @@ struct osd_dev *osduld_path_lookup(const char *name)
 		return ERR_PTR(-EINVAL);
 	}
 
-	od = kzalloc(sizeof(*od), GFP_KERNEL);
-	if (!od)
+	odh = kzalloc(sizeof(*odh), GFP_KERNEL);
+	if (unlikely(!odh))
 		return ERR_PTR(-ENOMEM);
 
 	file = filp_open(name, O_RDWR, 0);

@@ -203,37 +211,39 @@ struct osd_dev *osduld_path_lookup(const char *name)
 
 	oud = file->private_data;
 
-	*od = oud->od;
-	od->file = file;
+	odh->od = oud->od;
+	odh->file = file;
+	odh->oud = oud;
 
-	return od;
+	return &odh->od;
 
 close_file:
 	fput(file);
 free_od:
-	kfree(od);
+	kfree(odh);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL(osduld_path_lookup);
 
 void osduld_put_device(struct osd_dev *od)
 {
-
 	if (od && !IS_ERR(od)) {
-		struct osd_uld_device *oud = od->file->private_data;
+		struct osd_dev_handle *odh =
+				container_of(od, struct osd_dev_handle, od);
+		struct osd_uld_device *oud = odh->oud;
 
 		BUG_ON(od->scsi_device != oud->od.scsi_device);
 
 		/* If scsi has released the device (logout), and exofs has last
 		 * reference on oud it will be freed by above osd_uld_release
 		 * within fput below. But this will oops in cdev_release which
-		 * is called after the fops->release. __uld_get/put pair makes
+		 * is called after the fops->release. A get_/put_ pair makes
 		 * sure we have a cdev for the duration of fput
 		 */
-		__uld_get(oud);
-		fput(od->file);
-		__uld_put(oud);
-		kfree(od);
+		get_device(&oud->class_dev);
+		fput(odh->file);
+		put_device(&oud->class_dev);
+		kfree(odh);
 	}
 }
 EXPORT_SYMBOL(osduld_put_device);

@@ -264,8 +274,26 @@ static int __detect_osd(struct osd_uld_device *oud)
 	return 0;
 }
 
-static struct class *osd_sysfs_class;
-static DEFINE_IDA(osd_minor_ida);
+static void __remove(struct device *dev)
+{
+	struct osd_uld_device *oud = dev_get_drvdata(dev);
+	struct scsi_device *scsi_device = oud->od.scsi_device;
+
+	if (oud->cdev.owner)
+		cdev_del(&oud->cdev);
+
+	osd_dev_fini(&oud->od);
+	scsi_device_put(scsi_device);
+
+	OSD_INFO("osd_remove %s\n",
+		 oud->disk ? oud->disk->disk_name : NULL);
+
+	if (oud->disk)
+		put_disk(oud->disk);
+	ida_remove(&osd_minor_ida, oud->minor);
+
+	kfree(oud);
+}
 
 static int osd_probe(struct device *dev)
 {

@@ -297,7 +325,6 @@ static int osd_probe(struct device *dev)
 	if (NULL == oud)
 		goto err_retract_minor;
 
-	kref_init(&oud->kref);
 	dev_set_drvdata(dev, oud);
 	oud->minor = minor;

@@ -335,18 +362,26 @@ static int osd_probe(struct device *dev)
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
-	kobject_get(&oud->cdev.kobj); /* 2nd ref see osd_remove() */
-
-	/* class_member */
-	oud->class_member = device_create(osd_sysfs_class, dev,
-		MKDEV(SCSI_OSD_MAJOR, oud->minor), "%s", disk->disk_name);
-	if (IS_ERR(oud->class_member)) {
-		OSD_ERR("class_device_create failed\n");
-		error = PTR_ERR(oud->class_member);
+
+	/* class device member */
+	oud->class_dev.devt = oud->cdev.dev;
+	oud->class_dev.class = &osd_uld_class;
+	oud->class_dev.parent = dev;
+	oud->class_dev.release = __remove;
+	dev_set_drvdata(&oud->class_dev, oud);
+	error = dev_set_name(&oud->class_dev, disk->disk_name);
+	if (error) {
+		OSD_ERR("dev_set_name failed => %d\n", error);
+		goto err_put_cdev;
+	}
+
+	error = device_register(&oud->class_dev);
+	if (error) {
+		OSD_ERR("device_register failed => %d\n", error);
 		goto err_put_cdev;
 	}
 
-	dev_set_drvdata(oud->class_member, oud);
+	get_device(&oud->class_dev);
 
 	OSD_INFO("osd_probe %s\n", disk->disk_name);
 	return 0;

@@ -375,54 +410,12 @@ static int osd_remove(struct device *dev)
 			scsi_device);
 	}
 
-	if (oud->class_member)
-		device_destroy(osd_sysfs_class,
-			       MKDEV(SCSI_OSD_MAJOR, oud->minor));
-
-	/* We have 2 references to the cdev. One is released here
-	 * and also takes down the /dev/osdX mapping. The second
-	 * Will be released in __remove() after all users have released
-	 * the osd_uld_device.
-	 */
-	if (oud->cdev.owner)
-		cdev_del(&oud->cdev);
+	device_unregister(&oud->class_dev);
 
-	__uld_put(oud);
+	put_device(&oud->class_dev);
 	return 0;
 }
 
-static void __remove(struct kref *kref)
-{
-	struct osd_uld_device *oud = container_of(kref,
-					struct osd_uld_device, kref);
-	struct scsi_device *scsi_device = oud->od.scsi_device;
-
-	/* now let delete the char_dev */
-	kobject_put(&oud->cdev.kobj);
-
-	osd_dev_fini(&oud->od);
-	scsi_device_put(scsi_device);
-
-	OSD_INFO("osd_remove %s\n",
-		 oud->disk ? oud->disk->disk_name : NULL);
-
-	if (oud->disk)
-		put_disk(oud->disk);
-
-	ida_remove(&osd_minor_ida, oud->minor);
-	kfree(oud);
-}
-
-static void __uld_get(struct osd_uld_device *oud)
-{
-	kref_get(&oud->kref);
-}
-
-static void __uld_put(struct osd_uld_device *oud)
-{
-	kref_put(&oud->kref, __remove);
-}
-
 /*
  * Global driver and scsi registration
  */

@@ -440,11 +433,10 @@ static int __init osd_uld_init(void)
 {
 	int err;
 
-	osd_sysfs_class = class_create(THIS_MODULE, osd_symlink);
-	if (IS_ERR(osd_sysfs_class)) {
-		OSD_ERR("Unable to register sysfs class => %ld\n",
-			PTR_ERR(osd_sysfs_class));
-		return PTR_ERR(osd_sysfs_class);
+	err = class_register(&osd_uld_class);
+	if (err) {
+		OSD_ERR("Unable to register sysfs class => %d\n", err);
+		return err;
 	}
 
 	err = register_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0),

@@ -467,7 +459,7 @@ static int __init osd_uld_init(void)
 err_out_chrdev:
 	unregister_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0), SCSI_OSD_MAX_MINOR);
 err_out:
-	class_destroy(osd_sysfs_class);
+	class_unregister(&osd_uld_class);
 	return err;
 }

@@ -475,7 +467,7 @@ static void __exit osd_uld_exit(void)
 {
 	scsi_unregister_driver(&osd_driver.gendrv);
 	unregister_chrdev_region(MKDEV(SCSI_OSD_MAJOR, 0), SCSI_OSD_MAX_MINOR);
-	class_destroy(osd_sysfs_class);
+	class_unregister(&osd_uld_class);
 	OSD_INFO("UNLOADED %s\n", osd_version_string);
 }

include/scsi/osd_initiator.h +0 -1

index f787d24..589e5f0 100644
@@ -48,7 +48,6 @@ enum osd_std_version {
  */
 struct osd_dev {
 	struct scsi_device *scsi_device;
-	struct file *file;
 	unsigned def_timeout;
 
 #ifdef OSD_VER1_SUPPORT

Signature · 1 line

1.6.5.1

Boaz Harrosh

author

2009-11-01 16:45 UTC #16 lore raw

PATCH 5/5 libosd: osd_dev_info: Unique Identification of an OSD device

In reply to 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.

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 <[email protected]>

---
 drivers/scsi/osd/osd_initiator.c |   26 ++++++++--
 drivers/scsi/osd/osd_uld.c       |   99 ++++++++++++++++++++++++++++++++++++-
 include/scsi/osd_initiator.h     |   38 +++++++++++++--
 3 files changed, 150 insertions(+), 13 deletions(-)

drivers/scsi/osd/osd_initiator.c +20 -6

index 7a117c1..60b7ca1 100644
@@ -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,12 @@ 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;
+	/* Avoid NULL for memcmp optimization 0-length is good enough */
+	odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+	if (odi->osdname_len)
+		memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
+	OSD_INFO("OSD_NAME               [%s]\n", odi->osdname);
 	a++;
 
 	pFirst = get_attrs[a++].val_ptr;

@@ -171,6 +176,14 @@ 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);
+
+		if (unlikely(len > sizeof(odi->systemid))) {
+			OSD_ERR("OSD Target error: OSD_SYSTEM_ID too long(%d). "
+				"device idetification might not work\n", len);
+			len = sizeof(odi->systemid);
+		}
+		odi->systemid_len = len;
+		memcpy(odi->systemid, get_attrs[a].val_ptr, len);
 		a++;
 	}
 out:

@@ -178,16 +191,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;

drivers/scsi/osd/osd_uld.c +96 -3

135 lines not on this page · raw message

include/scsi/osd_initiator.h +34 -4

index 589e5f0..3ec346e 100644
@@ -55,10 +55,24 @@ 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);
+bool osduld_device_same(struct osd_dev *od, const struct osd_dev_info *odi);
+
 /* 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);

@@ -68,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;

Signature · 1 line

1.6.5.1