* [PATCH 1/2] osduld: Use device->release instead of internal kref
2009-11-29 14:23 ` Boaz Harrosh
@ 2009-11-29 14:25 ` Boaz Harrosh
2009-11-29 14:26 ` [PATCH 2/2] libosd: osd_dev_info: Unique Identification of an OSD device Boaz Harrosh
2009-11-29 15:23 ` linux-next: manual merge of the osd tree with the scsi tree James Bottomley
2 siblings, 0 replies; 9+ messages in thread
From: Boaz Harrosh @ 2009-11-29 14:25 UTC (permalink / raw)
To: James Bottomley; +Cc: Stephen Rothwell, linux-next, open-osd, linux-scsi
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 every thing is accounted
for on the device object. Internal kref is removed.
* At __remove() we can safely de-allocate the uld_ structure. (The
function has 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 <bharrosh@panasas.com>
---
drivers/scsi/osd/osd_uld.c | 162 ++++++++++++++++++++----------------------
include/scsi/osd_initiator.h | 1 -
2 files changed, 77 insertions(+), 86 deletions(-)
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 1ea6447..fc6fc1c 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -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 <bharrosh@panasas.com>");
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,27 @@ 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 = container_of(dev, struct osd_uld_device,
+ class_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 +326,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 +363,25 @@ 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;
+ 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;
}
- dev_set_drvdata(oud->class_member, oud);
+ error = device_register(&oud->class_dev);
+ if (error) {
+ OSD_ERR("device_register failed => %d\n", error);
+ goto err_put_cdev;
+ }
+
+ 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);
}
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index f787d24..589e5f0 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -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
--
1.6.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] libosd: osd_dev_info: Unique Identification of an OSD device
2009-11-29 14:23 ` Boaz Harrosh
2009-11-29 14:25 ` [PATCH 1/2] osduld: Use device->release instead of internal kref Boaz Harrosh
@ 2009-11-29 14:26 ` Boaz Harrosh
2009-11-29 15:23 ` linux-next: manual merge of the osd tree with the scsi tree James Bottomley
2 siblings, 0 replies; 9+ messages in thread
From: Boaz Harrosh @ 2009-11-29 14:26 UTC (permalink / raw)
To: James Bottomley; +Cc: Stephen Rothwell, linux-next, open-osd, linux-scsi
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 <bharrosh@panasas.com>
---
drivers/scsi/osd/osd_initiator.c | 26 ++++++++--
drivers/scsi/osd/osd_uld.c | 100 ++++++++++++++++++++++++++++++++++++-
include/scsi/osd_initiator.h | 38 +++++++++++++--
3 files changed, 151 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 7a117c1..60b7ca1 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,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;
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index fc6fc1c..0a90702 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -84,6 +84,7 @@ struct osd_uld_device {
struct device class_dev;
struct cdev cdev;
struct osd_dev od;
+ struct osd_dev_info odi;
struct gendisk *disk;
};
@@ -225,6 +226,72 @@ 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 find_oud_t {
+ const struct osd_dev_info *odi;
+ struct device *dev;
+ struct osd_uld_device *oud;
+} ;
+
+int _mach_odi(struct device *dev, void *find_data)
+{
+ struct osd_uld_device *oud = container_of(dev, struct osd_uld_device,
+ class_dev);
+ struct find_oud_t *fot = find_data;
+ const struct osd_dev_info *odi = fot->odi;
+
+ 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);
+ fot->oud = oud;
+ return 1;
+ } else {
+ return 0;
+ }
+}
+
+/* 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.
+ */
+struct osd_dev *osduld_info_lookup(const struct osd_dev_info *odi)
+{
+ struct find_oud_t find = {.odi = odi};
+
+ find.dev = class_find_device(&osd_uld_class, NULL, &find, _mach_odi);
+ if (likely(find.dev)) {
+ struct osd_dev_handle *odh = kzalloc(sizeof(*odh), GFP_KERNEL);
+
+ if (unlikely(!odh)) {
+ put_device(find.dev);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ odh->od = find.oud->od;
+ odh->oud = find.oud;
+
+ return &odh->od;
+ }
+
+ return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL(osduld_info_lookup);
+
void osduld_put_device(struct osd_dev *od)
{
if (od && !IS_ERR(od)) {
@@ -240,14 +307,39 @@ void osduld_put_device(struct osd_dev *od)
* is called after the fops->release. A get_/put_ pair makes
* sure we have a cdev for the duration of fput
*/
- get_device(&oud->class_dev);
- fput(odh->file);
+ if (odh->file) {
+ get_device(&oud->class_dev);
+ fput(odh->file);
+ }
put_device(&oud->class_dev);
kfree(odh);
}
}
EXPORT_SYMBOL(osduld_put_device);
+const struct osd_dev_info *osduld_device_info(struct osd_dev *od)
+{
+ struct osd_dev_handle *odh =
+ container_of(od, struct osd_dev_handle, od);
+ return &odh->oud->odi;
+}
+EXPORT_SYMBOL(osduld_device_info);
+
+bool osduld_device_same(struct osd_dev *od, const struct osd_dev_info *odi)
+{
+ struct osd_dev_handle *odh =
+ container_of(od, struct osd_dev_handle, od);
+ struct osd_uld_device *oud = odh->oud;
+
+ return (oud->odi.systemid_len == odi->systemid_len) &&
+ _the_same_or_null(oud->odi.systemid, oud->odi.systemid_len,
+ odi->systemid, odi->systemid_len) &&
+ (oud->odi.osdname_len == odi->osdname_len) &&
+ _the_same_or_null(oud->odi.osdname, oud->odi.osdname_len,
+ odi->osdname, odi->osdname_len);
+}
+EXPORT_SYMBOL(osduld_device_same);
+
/*
* Scsi Device operations
*/
@@ -268,7 +360,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;
@@ -280,6 +372,8 @@ static void __remove(struct device *dev)
class_dev);
struct scsi_device *scsi_device = oud->od.scsi_device;
+ kfree(oud->odi.osdname);
+
if (oud->cdev.owner)
cdev_del(&oud->cdev);
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index 589e5f0..3ec346e 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -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;
--
1.6.5.2
^ permalink raw reply related [flat|nested] 9+ messages in thread