[PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

This patch series provides a SCSI option for persistent device
names in kernel. With this option, user can always assign a
same device name (e.g. sda) to a specific device according to
udev rules and device id.

Issue:
Recently, kernel registers block devices in parallel. As a result,
different device names will be assigned at each boot time. This
will confuse file-system mounter, thus we usually use persistent
symbolic links provided by udev. However, dmesg and procfs outputs
show device names instead of the symbolic link names. This causes
a serious problem when managing multiple devices (e.g. on a
large-scale storage), because usually, device errors are output
with device names on dmesg. We also concern about some commands
which output device names, as well as kernel messages.

Solution:
To assign a persistent device name, I create unnamed devices (not
a block device, but an intermediate device. users cannot read/write
this device). When scsi device driver finds a LU, the LU is registered
as an unnamed device and inform to udev. After udev finds the unnamed
device, udev assigns a persistent device name to a specific device
according to udev rules and registers it as a block device. Hence,
this is just a naming, not a renaming.

Some users are satisfied with current udev solution. Therefore, my
proposal is implemented as an option.

If using this option, add the following kernel parameter.

	scsi_mod.persistent_name=1

Also, if you want to assign a device name with sda, add the
following udev rules.

SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
-c 'echo -n sda > /sys/%p/disk_name'"

Todo:
- usb support
- hot-remove support

I've already started discussion about device naming at LKML.
https://lkml.org/lkml/2010/10/8/31

Signed-off-by: Nao Nishijima <nao.nishijima.xt@hitachi.com>
---

 drivers/scsi/Makefile       |    1 
 drivers/scsi/scsi_sysfs.c   |   26 +++
 drivers/scsi/scsi_unnamed.c |  340 +++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_unnamed.h |   25 +++
 4 files changed, 387 insertions(+), 5 deletions(-)
 create mode 100644 drivers/scsi/scsi_unnamed.c
 create mode 100644 drivers/scsi/scsi_unnamed.h

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 7ad0b8a..782adc1 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -167,6 +167,7 @@ scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
 scsi_mod-y			+= scsi_trace.o
 scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
+scsi_mod-y			+= scsi_unnamed.o
 
 scsi_tgt-y			+= scsi_tgt_lib.o scsi_tgt_if.o
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e44ff64..7959f5d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -22,6 +22,7 @@
 
 #include "scsi_priv.h"
 #include "scsi_logging.h"
+#include "scsi_unnamed.h"
 
 static struct device_type scsi_dev_type;
 
@@ -393,13 +394,27 @@ int scsi_sysfs_register(void)
 {
 	int error;
 
+	error = scsi_unnamed_register(&scsi_bus_type);
+	if (error)
+		goto cleanup;
+
 	error = bus_register(&scsi_bus_type);
-	if (!error) {
-		error = class_register(&sdev_class);
-		if (error)
-			bus_unregister(&scsi_bus_type);
-	}
+	if (error)
+		goto cleanup_scsi_unnamed;
+
+	error = class_register(&sdev_class);
+	if (error)
+		goto cleanup_bus;
+
+	return 0;
+
+cleanup_bus:
+	bus_unregister(&scsi_bus_type);
+
+cleanup_scsi_unnamed:
+	scsi_unnamed_unregister();
 
+cleanup:
 	return error;
 }
 
@@ -407,6 +422,7 @@ void scsi_sysfs_unregister(void)
 {
 	class_unregister(&sdev_class);
 	bus_unregister(&scsi_bus_type);
+	scsi_unnamed_unregister();
 }
 
 /*
diff --git a/drivers/scsi/scsi_unnamed.c b/drivers/scsi/scsi_unnamed.c
new file mode 100644
index 0000000..ed96945
--- /dev/null
+++ b/drivers/scsi/scsi_unnamed.c
@@ -0,0 +1,340 @@
+/*
+ * scsi_unnamed.c - SCSI driver for unnmaed device
+ *
+ * Copyright: Copyright (c) Hitachi, Ltd. 2011
+ *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/kobject.h>
+#include <linux/kdev_t.h>
+#include <linux/sysdev.h>
+#include <linux/list.h>
+#include <linux/wait.h>
+
+#include <scsi/scsi_driver.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi.h>
+
+#define MAX_BUFFER_LEN	256
+#define DISK_NAME_LEN	32
+
+#define SCSI_ID_BINARY	1
+#define SCSI_ID_ASCII	2
+#define SCSI_ID_UTF8	3
+
+static LIST_HEAD(su_list);
+static DEFINE_MUTEX(su_list_lock);
+
+static int persistent_name;
+MODULE_PARM_DESC(persistent_name, "SCSI unnamed device support");
+module_param(persistent_name, bool, S_IRUGO);
+
+static struct class su_sysfs_class = {
+	.name	= "scsi_unnamed",
+};
+
+struct scsi_unnamed {
+	struct list_head list;
+	struct device dev;
+	char disk_name[DISK_NAME_LEN];
+	char disk_lun[MAX_BUFFER_LEN];
+	char disk_id[MAX_BUFFER_LEN];
+	bool assigned;
+};
+
+#define to_scsi_unnamed(d) \
+	container_of(d, struct scsi_unnamed, dev)
+
+/* Get device identification VPD page */
+static void store_disk_id(struct scsi_device *sdev, char *disk_id)
+{
+	unsigned char *page_83;
+	int page_len, id_len, j = 0, i = 8;
+	static const char hex_str[] = "0123456789abcdef";
+
+	page_83 = kmalloc(MAX_BUFFER_LEN, GFP_KERNEL);
+	if (!page_83)
+		return;
+
+	if (scsi_get_vpd_page(sdev, 0x83, page_83, MAX_BUFFER_LEN))
+		goto err;
+
+	page_len = ((page_83[2] << 8) | page_83[3]) + 4;
+	if (page_len > 0) {
+		if ((page_83[5] & 0x30) != 0)
+			goto err;
+
+		id_len = page_83[7];
+		if (id_len > 0) {
+			switch (page_83[4] & 0x0f) {
+			case SCSI_ID_BINARY:
+				while (i < id_len) {
+					disk_id[j++] = hex_str[(page_83[i]
+						& 0xf0) >> 4];
+					disk_id[j++] = hex_str[page_83[i]
+						& 0x0f];
+					i++;
+				}
+				break;
+			case SCSI_ID_ASCII:
+			case SCSI_ID_UTF8:
+				strncpy(disk_id, strim(page_83 + 8), id_len);
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+err:
+	kfree(page_83);
+	return;
+}
+
+static ssize_t show_disk_name(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_name);
+}
+
+static ssize_t show_disk_lun(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_lun);
+}
+
+static ssize_t show_disk_id(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_id);
+}
+
+/* Assign a persistent device name */
+static ssize_t store_disk_name(struct device *dev,
+		struct device_attribute *attr, char *buf, size_t count)
+{
+	struct scsi_unnamed *su = to_scsi_unnamed(dev);
+	struct scsi_unnamed *tmp;
+	struct device *lu = dev->parent;
+	int ret = -EINVAL;
+
+	if (count >= DISK_NAME_LEN) {
+		printk(KERN_WARNING "su: Too long a persistent name!\n");
+		return -EINVAL;
+	}
+
+	if (su->assigned) {
+		printk(KERN_WARNING "su: Already assigned a persistent name!\n");
+		return -EINVAL;
+	}
+
+	if (strncmp(lu->driver->name, buf, 2)) {
+		printk(KERN_WARNING "su: Wrong prefix!\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&su_list_lock);
+	list_for_each_entry(tmp, &su_list, list) {
+		if (!strncmp(tmp->disk_name, buf, DISK_NAME_LEN)) {
+			printk(KERN_WARNING "su: Duplicate name exsists!\n");
+			return -EINVAL;
+		}
+	}
+	strncpy(su->disk_name, buf, DISK_NAME_LEN);
+	mutex_unlock(&su_list_lock);
+
+	if (!lu->driver->probe)
+		return -EINVAL;
+
+	lu->init_name = buf;
+
+	ret = lu->driver->probe(lu);
+	if (ret) {
+		lu->init_name = NULL;
+		su->disk_name[0] = '\0';
+		return ret;
+	}
+
+	su->assigned = true;
+	return count;
+}
+
+static DEVICE_ATTR(disk_name, S_IRUGO|S_IWUSR, show_disk_name,
+			store_disk_name);
+static DEVICE_ATTR(disk_lun, S_IRUGO, show_disk_lun, NULL);
+static DEVICE_ATTR(disk_id, S_IRUGO, show_disk_id, NULL);
+
+/* Confirm the driver name and the device type */
+static int check_device_type(char type, const char *name)
+{
+	switch (type) {
+	case TYPE_DISK:
+	case TYPE_MOD:
+	case TYPE_RBC:
+		if (strncmp("sd", name, 2))
+			return -ENODEV;
+		break;
+	case TYPE_ROM:
+	case TYPE_WORM:
+		if (strncmp("sr", name, 2))
+			return -ENODEV;
+		break;
+	case TYPE_TAPE:
+		if (strncmp("st", name, 2))
+			return -ENODEV;
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+/**
+ * scsi_unnamed_probe - Setup the unnamed device.
+ * @dev: parent scsi device
+ *
+ * This function adds a unnamed device to the device model and
+ * gets a number of device information by scsi inquiry commands.
+ * Udev identify a device by those information.
+ *
+ * Unnamed devices:
+ * This device is not a block device and user can not read/write
+ * this device. But it can advertise device information in sysfs.
+ */
+int scsi_unnamed_probe(struct device *dev)
+{
+	struct scsi_unnamed *su;
+	struct scsi_device *sdev = to_scsi_device(dev);
+	int ret = -EINVAL;
+	static int i;
+
+	if (check_device_type(sdev->type, dev->driver->name))
+		return -ENODEV;
+
+	su = kzalloc(sizeof(*su), GFP_KERNEL);
+	if (!su)
+		return -ENOMEM;
+
+	device_initialize(&su->dev);
+	su->dev.parent = dev;
+	su->dev.class = &su_sysfs_class;
+	dev_set_name(&su->dev, "su%d", i++);
+	strncpy(su->disk_lun, dev_name(dev), MAX_BUFFER_LEN);
+
+	ret = device_add(&su->dev);
+	if (ret)
+		goto err;
+
+	ret = device_create_file(&su->dev, &dev_attr_disk_name);
+	if (ret)
+		goto err_disk_name;
+
+	ret = device_create_file(&su->dev, &dev_attr_disk_lun);
+	if (ret)
+		goto err_disk_lun;
+
+	store_disk_id(sdev, su->disk_id);
+	if (su->disk_id[0] != '\0') {
+		ret = device_create_file(&su->dev, &dev_attr_disk_id);
+		if (ret)
+			goto err_disk_id;
+	}
+
+	su->assigned = false;
+	mutex_lock(&su_list_lock);
+	list_add(&su->list, &su_list);
+	mutex_unlock(&su_list_lock);
+
+	return 0;
+
+err_disk_id:
+	device_remove_file(&su->dev, &dev_attr_disk_lun);
+
+err_disk_lun:
+	device_remove_file(&su->dev, &dev_attr_disk_name);
+
+err_disk_name:
+	device_del(&su->dev);
+
+err:
+	kfree(su);
+	return ret;
+}
+
+/* Remove a unnamed device from su_list and release resources */
+void scsi_unnamed_remove(struct device *dev)
+{
+	struct scsi_unnamed *tmp;
+
+	mutex_lock(&su_list_lock);
+	list_for_each_entry(tmp, &su_list, list) {
+		if (dev == tmp->dev.parent) {
+			list_del(&tmp->list);
+			break;
+		}
+	}
+	mutex_unlock(&su_list_lock);
+
+	if (tmp->disk_id[0] != '\0')
+		device_remove_file(&tmp->dev, &dev_attr_disk_id);
+	device_remove_file(&tmp->dev, &dev_attr_disk_name);
+	device_remove_file(&tmp->dev, &dev_attr_disk_lun);
+	device_del(&tmp->dev);
+
+	device_release_driver(dev);
+
+	kfree(tmp);
+}
+
+/**
+ * copy_persistent_name - Copy the device name
+ * @disk_name: allocate to
+ * @dev: scsi device
+ *
+ * Copy an initial name of the device to disk_name.
+ */
+int copy_persistent_name(char *disk_name, struct device *dev)
+{
+	if (persistent_name) {
+		strncpy(disk_name, dev->init_name, DISK_NAME_LEN);
+		return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(copy_persistent_name);
+
+int scsi_unnamed_register(struct bus_type *bus)
+{
+	if (persistent_name) {
+		bus->probe = scsi_unnamed_probe;
+		bus->remove = scsi_unnamed_remove;
+		return class_register(&su_sysfs_class);
+	}
+	return 0;
+}
+EXPORT_SYMBOL(scsi_unnamed_register);
+
+void scsi_unnamed_unregister(void)
+{
+	if (persistent_name)
+		class_unregister(&su_sysfs_class);
+}
+EXPORT_SYMBOL(scsi_unnamed_unregister);
diff --git a/drivers/scsi/scsi_unnamed.h b/drivers/scsi/scsi_unnamed.h
new file mode 100644
index 0000000..ca4e852
--- /dev/null
+++ b/drivers/scsi/scsi_unnamed.h
@@ -0,0 +1,25 @@
+/*
+ * scsi_unnamed.h - SCSI driver for unnmaed device
+ *
+ * Copyright: Copyright (c) Hitachi, Ltd. 2011
+ *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA
+ */
+
+extern int check_disk_name_prefix(char *, struct device *);
+extern int copy_persistent_name(char *, struct device *);
+extern int scsi_unnamed_register(struct bus_type *);
+extern void scsi_unnamed_unregister(void);

[PATCH 2/2] SCSI: modify SCSI subsystem

[Nao Nishijima]

Add a SCSI option for persistent device names in Kernel.

If scsi_mod.persistent_name=1, device names is assigned by udev.
If scsi_mod.persistent_name=0, device names is assigned the order
of logical unit recognizing.

Signed-off-by: Nao Nishijima <nao.nishijima.xt@hitachi.com>
---

 drivers/scsi/sd.c |   10 +++++++---
 drivers/scsi/sr.c |    4 +++-
 drivers/scsi/st.c |    4 +++-
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b61ebec..94ad290 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -65,6 +65,7 @@
 
 #include "sd.h"
 #include "scsi_logging.h"
+#include "scsi_unnamed.h"
 
 MODULE_AUTHOR("Eric Youngdale");
 MODULE_DESCRIPTION("SCSI disk (sd) driver");
@@ -2554,9 +2555,12 @@ static int sd_probe(struct device *dev)
 		goto out_free_index;
 	}
 
-	error = sd_format_disk_name("sd", index, gd->disk_name, DISK_NAME_LEN);
-	if (error)
-		goto out_free_index;
+	if (!copy_persistent_name(gd->disk_name, dev)) {
+		error = sd_format_disk_name("sd", index,
+				gd->disk_name, DISK_NAME_LEN);
+		if (error)
+			goto out_free_index;
+	}
 
 	sdkp->device = sdp;
 	sdkp->driver = &sd_template;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index aefadc6..682b1a4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -58,6 +58,7 @@
 
 #include "scsi_logging.h"
 #include "sr.h"
+#include "scsi_unnamed.h"
 
 
 MODULE_DESCRIPTION("SCSI cdrom (sr) driver");
@@ -634,7 +635,8 @@ static int sr_probe(struct device *dev)
 
 	disk->major = SCSI_CDROM_MAJOR;
 	disk->first_minor = minor;
-	sprintf(disk->disk_name, "sr%d", minor);
+	if (!copy_persistent_name(disk->disk_name, dev))
+		sprintf(disk->disk_name, "sr%d", minor);
 	disk->fops = &sr_bdops;
 	disk->flags = GENHD_FL_CD;
 	disk->events = DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST;
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 1871b8a..9acf8b2 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -74,6 +74,7 @@ static const char *verstr = "20101219";
 
 #include "st_options.h"
 #include "st.h"
+#include "scsi_unnamed.h"
 
 static DEFINE_MUTEX(st_mutex);
 static int buffer_kbs;
@@ -4051,7 +4052,8 @@ static int st_probe(struct device *dev)
 	}
 	kref_init(&tpnt->kref);
 	tpnt->disk = disk;
-	sprintf(disk->disk_name, "st%d", i);
+	if (!copy_persistent_name(disk->disk_name, dev))
+		sprintf(disk->disk_name, "st%d", i);
 	disk->private_data = &tpnt->driver;
 	disk->queue = SDp->request_queue;
 	tpnt->driver = &st_template;

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> This patch series provides a SCSI option for persistent device
> names in kernel. With this option, user can always assign a
> same device name (e.g. sda) to a specific device according to
> udev rules and device id.
> 
> Issue:
> Recently, kernel registers block devices in parallel. As a result,
> different device names will be assigned at each boot time. This
> will confuse file-system mounter, thus we usually use persistent
> symbolic links provided by udev.

Yes, that is what you should use if you care about this.

> However, dmesg and procfs outputs
> show device names instead of the symbolic link names. This causes
> a serious problem when managing multiple devices (e.g. on a
> large-scale storage), because usually, device errors are output
> with device names on dmesg.

Then fix your tools that read the output of these files to point to the
proper persistent name instead of trying to get the kernel to solve the
problem.

> We also concern about some commands
> which output device names, as well as kernel messages.

What commands are you concerned about?

> Solution:
> To assign a persistent device name, I create unnamed devices (not
> a block device, but an intermediate device. users cannot read/write
> this device). When scsi device driver finds a LU, the LU is registered
> as an unnamed device and inform to udev. After udev finds the unnamed
> device, udev assigns a persistent device name to a specific device
> according to udev rules and registers it as a block device. Hence,
> this is just a naming, not a renaming.

Ick, so the kernel waits for userspace before you are able to use the
device?  That sounds messy.

> Some users are satisfied with current udev solution. Therefore, my
> proposal is implemented as an option.
> 
> If using this option, add the following kernel parameter.
> 
> 	scsi_mod.persistent_name=1
> 
> Also, if you want to assign a device name with sda, add the
> following udev rules.
> 
> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
> -c 'echo -n sda > /sys/%p/disk_name'"
> 
> Todo:
> - usb support

Why?  USB uses scsi, so why would it not work as-is today?  What about
libata devices?

> - hot-remove support

So once you have assigned a name and the device is removed bad things
happen?  What is the problem with this?

Note, any review of the code below does not imply that I agree with the
ideas here at all.  In fact, I really don't like this idea at all, but I
might as well review the code anyway for your future contributions :)

> 
> I've already started discussion about device naming at LKML.
> https://lkml.org/lkml/2010/10/8/31
> 
> Signed-off-by: Nao Nishijima <nao.nishijima.xt@hitachi.com>
> ---
> 
>  drivers/scsi/Makefile       |    1 
>  drivers/scsi/scsi_sysfs.c   |   26 +++
>  drivers/scsi/scsi_unnamed.c |  340 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/scsi_unnamed.h |   25 +++
>  4 files changed, 387 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/scsi/scsi_unnamed.c
>  create mode 100644 drivers/scsi/scsi_unnamed.h
> 
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 7ad0b8a..782adc1 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -167,6 +167,7 @@ scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
>  scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
>  scsi_mod-y			+= scsi_trace.o
>  scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
> +scsi_mod-y			+= scsi_unnamed.o
>  
>  scsi_tgt-y			+= scsi_tgt_lib.o scsi_tgt_if.o
>  
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index e44ff64..7959f5d 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -22,6 +22,7 @@
>  
>  #include "scsi_priv.h"
>  #include "scsi_logging.h"
> +#include "scsi_unnamed.h"
>  
>  static struct device_type scsi_dev_type;
>  
> @@ -393,13 +394,27 @@ int scsi_sysfs_register(void)
>  {
>  	int error;
>  
> +	error = scsi_unnamed_register(&scsi_bus_type);
> +	if (error)
> +		goto cleanup;
> +
>  	error = bus_register(&scsi_bus_type);
> -	if (!error) {
> -		error = class_register(&sdev_class);
> -		if (error)
> -			bus_unregister(&scsi_bus_type);
> -	}
> +	if (error)
> +		goto cleanup_scsi_unnamed;
> +
> +	error = class_register(&sdev_class);
> +	if (error)
> +		goto cleanup_bus;
> +
> +	return 0;
> +
> +cleanup_bus:
> +	bus_unregister(&scsi_bus_type);
> +
> +cleanup_scsi_unnamed:
> +	scsi_unnamed_unregister();
>  
> +cleanup:
>  	return error;
>  }
>  
> @@ -407,6 +422,7 @@ void scsi_sysfs_unregister(void)
>  {
>  	class_unregister(&sdev_class);
>  	bus_unregister(&scsi_bus_type);
> +	scsi_unnamed_unregister();
>  }
>  
>  /*
> diff --git a/drivers/scsi/scsi_unnamed.c b/drivers/scsi/scsi_unnamed.c
> new file mode 100644
> index 0000000..ed96945
> --- /dev/null
> +++ b/drivers/scsi/scsi_unnamed.c
> @@ -0,0 +1,340 @@
> +/*
> + * scsi_unnamed.c - SCSI driver for unnmaed device
> + *
> + * Copyright: Copyright (c) Hitachi, Ltd. 2011
> + *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Do you _REALLY_ mean "any later version?  Are you sure about that?  If
so, fine, just be careful please.

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA

Unless you wish to track the movements of the FSF's office space for the
next 40+ years and keep this file up to date, just remove this paragraph
please.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/kobject.h>
> +#include <linux/kdev_t.h>
> +#include <linux/sysdev.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +
> +#include <scsi/scsi_driver.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi.h>
> +
> +#define MAX_BUFFER_LEN	256
> +#define DISK_NAME_LEN	32

Why this limitation?

> +
> +#define SCSI_ID_BINARY	1
> +#define SCSI_ID_ASCII	2
> +#define SCSI_ID_UTF8	3
> +
> +static LIST_HEAD(su_list);
> +static DEFINE_MUTEX(su_list_lock);
> +
> +static int persistent_name;
> +MODULE_PARM_DESC(persistent_name, "SCSI unnamed device support");
> +module_param(persistent_name, bool, S_IRUGO);

Why is this a module parameter?

> +
> +static struct class su_sysfs_class = {
> +	.name	= "scsi_unnamed",
> +};

Why a static class?  What is it used for?  If you are adding sysfs
files, you need to also add Documentation/ABI/ entries at the same time.
Please do that if you really are adding new sysfs files.

> +
> +struct scsi_unnamed {
> +	struct list_head list;
> +	struct device dev;
> +	char disk_name[DISK_NAME_LEN];
> +	char disk_lun[MAX_BUFFER_LEN];
> +	char disk_id[MAX_BUFFER_LEN];
> +	bool assigned;
> +};
> +
> +#define to_scsi_unnamed(d) \
> +	container_of(d, struct scsi_unnamed, dev)
> +
> +/* Get device identification VPD page */
> +static void store_disk_id(struct scsi_device *sdev, char *disk_id)
> +{
> +	unsigned char *page_83;
> +	int page_len, id_len, j = 0, i = 8;
> +	static const char hex_str[] = "0123456789abcdef";

Don't we have functions that do this already?

> +
> +	page_83 = kmalloc(MAX_BUFFER_LEN, GFP_KERNEL);
> +	if (!page_83)
> +		return;
> +
> +	if (scsi_get_vpd_page(sdev, 0x83, page_83, MAX_BUFFER_LEN))
> +		goto err;
> +
> +	page_len = ((page_83[2] << 8) | page_83[3]) + 4;
> +	if (page_len > 0) {
> +		if ((page_83[5] & 0x30) != 0)
> +			goto err;
> +
> +		id_len = page_83[7];
> +		if (id_len > 0) {
> +			switch (page_83[4] & 0x0f) {
> +			case SCSI_ID_BINARY:
> +				while (i < id_len) {
> +					disk_id[j++] = hex_str[(page_83[i]
> +						& 0xf0) >> 4];
> +					disk_id[j++] = hex_str[page_83[i]
> +						& 0x0f];
> +					i++;
> +				}
> +				break;
> +			case SCSI_ID_ASCII:
> +			case SCSI_ID_UTF8:
> +				strncpy(disk_id, strim(page_83 + 8), id_len);
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +
> +err:
> +	kfree(page_83);
> +	return;
> +}
> +
> +static ssize_t show_disk_name(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_name);
> +}
> +
> +static ssize_t show_disk_lun(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_lun);
> +}
> +
> +static ssize_t show_disk_id(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_id);
> +}
> +
> +/* Assign a persistent device name */
> +static ssize_t store_disk_name(struct device *dev,
> +		struct device_attribute *attr, char *buf, size_t count)
> +{
> +	struct scsi_unnamed *su = to_scsi_unnamed(dev);
> +	struct scsi_unnamed *tmp;
> +	struct device *lu = dev->parent;
> +	int ret = -EINVAL;
> +
> +	if (count >= DISK_NAME_LEN) {
> +		printk(KERN_WARNING "su: Too long a persistent name!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (su->assigned) {
> +		printk(KERN_WARNING "su: Already assigned a persistent name!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (strncmp(lu->driver->name, buf, 2)) {
> +		printk(KERN_WARNING "su: Wrong prefix!\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&su_list_lock);
> +	list_for_each_entry(tmp, &su_list, list) {
> +		if (!strncmp(tmp->disk_name, buf, DISK_NAME_LEN)) {
> +			printk(KERN_WARNING "su: Duplicate name exsists!\n");
> +			return -EINVAL;
> +		}
> +	}
> +	strncpy(su->disk_name, buf, DISK_NAME_LEN);
> +	mutex_unlock(&su_list_lock);
> +
> +	if (!lu->driver->probe)
> +		return -EINVAL;
> +
> +	lu->init_name = buf;
> +
> +	ret = lu->driver->probe(lu);
> +	if (ret) {
> +		lu->init_name = NULL;
> +		su->disk_name[0] = '\0';
> +		return ret;
> +	}
> +
> +	su->assigned = true;
> +	return count;
> +}
> +
> +static DEVICE_ATTR(disk_name, S_IRUGO|S_IWUSR, show_disk_name,
> +			store_disk_name);
> +static DEVICE_ATTR(disk_lun, S_IRUGO, show_disk_lun, NULL);
> +static DEVICE_ATTR(disk_id, S_IRUGO, show_disk_id, NULL);
> +
> +/* Confirm the driver name and the device type */
> +static int check_device_type(char type, const char *name)
> +{
> +	switch (type) {
> +	case TYPE_DISK:
> +	case TYPE_MOD:
> +	case TYPE_RBC:
> +		if (strncmp("sd", name, 2))
> +			return -ENODEV;
> +		break;
> +	case TYPE_ROM:
> +	case TYPE_WORM:
> +		if (strncmp("sr", name, 2))
> +			return -ENODEV;
> +		break;
> +	case TYPE_TAPE:
> +		if (strncmp("st", name, 2))
> +			return -ENODEV;
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * scsi_unnamed_probe - Setup the unnamed device.
> + * @dev: parent scsi device
> + *
> + * This function adds a unnamed device to the device model and
> + * gets a number of device information by scsi inquiry commands.
> + * Udev identify a device by those information.
> + *
> + * Unnamed devices:
> + * This device is not a block device and user can not read/write
> + * this device. But it can advertise device information in sysfs.
> + */
> +int scsi_unnamed_probe(struct device *dev)
> +{
> +	struct scsi_unnamed *su;
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	int ret = -EINVAL;
> +	static int i;
> +
> +	if (check_device_type(sdev->type, dev->driver->name))
> +		return -ENODEV;
> +
> +	su = kzalloc(sizeof(*su), GFP_KERNEL);
> +	if (!su)
> +		return -ENOMEM;
> +
> +	device_initialize(&su->dev);
> +	su->dev.parent = dev;
> +	su->dev.class = &su_sysfs_class;

You just created a device that can not be removed from the system (i.e.
you have no release function.)  This is a major bug and as per the
documentation for kobjects and the driver model in the kernel tree, I am
allowed to mock this code mercilessly :)

> +	dev_set_name(&su->dev, "su%d", i++);
> +	strncpy(su->disk_lun, dev_name(dev), MAX_BUFFER_LEN);
> +
> +	ret = device_add(&su->dev);
> +	if (ret)
> +		goto err;
> +
> +	ret = device_create_file(&su->dev, &dev_attr_disk_name);

Nope, you just raced with userspace here.  NEVER create a sysfs file
after you have added it to the system.  Userspace just got notified of
the device and is already starting to read the file.  As it's not there
yet, you just failed :(

Use attributes properly to keep this race from happening.


> +	if (ret)
> +		goto err_disk_name;
> +
> +	ret = device_create_file(&su->dev, &dev_attr_disk_lun);
> +	if (ret)
> +		goto err_disk_lun;
> +
> +	store_disk_id(sdev, su->disk_id);
> +	if (su->disk_id[0] != '\0') {
> +		ret = device_create_file(&su->dev, &dev_attr_disk_id);
> +		if (ret)
> +			goto err_disk_id;
> +	}
> +
> +	su->assigned = false;
> +	mutex_lock(&su_list_lock);
> +	list_add(&su->list, &su_list);
> +	mutex_unlock(&su_list_lock);
> +
> +	return 0;
> +
> +err_disk_id:
> +	device_remove_file(&su->dev, &dev_attr_disk_lun);
> +
> +err_disk_lun:
> +	device_remove_file(&su->dev, &dev_attr_disk_name);
> +
> +err_disk_name:
> +	device_del(&su->dev);
> +
> +err:
> +	kfree(su);
> +	return ret;
> +}
> +
> +/* Remove a unnamed device from su_list and release resources */
> +void scsi_unnamed_remove(struct device *dev)
> +{
> +	struct scsi_unnamed *tmp;
> +
> +	mutex_lock(&su_list_lock);
> +	list_for_each_entry(tmp, &su_list, list) {
> +		if (dev == tmp->dev.parent) {
> +			list_del(&tmp->list);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&su_list_lock);
> +
> +	if (tmp->disk_id[0] != '\0')
> +		device_remove_file(&tmp->dev, &dev_attr_disk_id);
> +	device_remove_file(&tmp->dev, &dev_attr_disk_name);
> +	device_remove_file(&tmp->dev, &dev_attr_disk_lun);
> +	device_del(&tmp->dev);

Your kerrnel just spit out some very nasty messages when this function
was called, right?  Why are you ignoring them?

> +
> +	device_release_driver(dev);
> +
> +	kfree(tmp);
> +}
> +
> +/**
> + * copy_persistent_name - Copy the device name
> + * @disk_name: allocate to
> + * @dev: scsi device
> + *
> + * Copy an initial name of the device to disk_name.
> + */
> +int copy_persistent_name(char *disk_name, struct device *dev)
> +{
> +	if (persistent_name) {
> +		strncpy(disk_name, dev->init_name, DISK_NAME_LEN);
> +		return 1;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(copy_persistent_name);

That's a very bad global function name.

EXPORT_SYMBOL_GPL?  And why is it exported at all?  Who would ever need
to call this from a module?

> +
> +int scsi_unnamed_register(struct bus_type *bus)
> +{
> +	if (persistent_name) {
> +		bus->probe = scsi_unnamed_probe;
> +		bus->remove = scsi_unnamed_remove;
> +		return class_register(&su_sysfs_class);
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(scsi_unnamed_register);

Same here, why is this exported?

> +
> +void scsi_unnamed_unregister(void)
> +{
> +	if (persistent_name)
> +		class_unregister(&su_sysfs_class);
> +}
> +EXPORT_SYMBOL(scsi_unnamed_unregister);

And here.

> diff --git a/drivers/scsi/scsi_unnamed.h b/drivers/scsi/scsi_unnamed.h
> new file mode 100644
> index 0000000..ca4e852
> --- /dev/null
> +++ b/drivers/scsi/scsi_unnamed.h
> @@ -0,0 +1,25 @@
> +/*
> + * scsi_unnamed.h - SCSI driver for unnmaed device
> + *
> + * Copyright: Copyright (c) Hitachi, Ltd. 2011
> + *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Same comment as above.

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA

And again, same comment as above.

thanks,

greg k-h

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> This patch series provides a SCSI option for persistent device
> names in kernel. With this option, user can always assign a
> same device name (e.g. sda) to a specific device according to
> udev rules and device id.
> 
> Issue:
> Recently, kernel registers block devices in parallel. As a result,
> different device names will be assigned at each boot time. This
> will confuse file-system mounter, thus we usually use persistent
> symbolic links provided by udev. However, dmesg and procfs outputs
> show device names instead of the symbolic link names. This causes
> a serious problem when managing multiple devices (e.g. on a
> large-scale storage), because usually, device errors are output
> with device names on dmesg. We also concern about some commands
> which output device names, as well as kernel messages.
> 
> Solution:
> To assign a persistent device name, I create unnamed devices (not
> a block device, but an intermediate device. users cannot read/write
> this device). When scsi device driver finds a LU, the LU is registered
> as an unnamed device and inform to udev. After udev finds the unnamed
> device, udev assigns a persistent device name to a specific device
> according to udev rules and registers it as a block device. Hence,
> this is just a naming, not a renaming.
> 
> Some users are satisfied with current udev solution. Therefore, my
> proposal is implemented as an option.
> 
> If using this option, add the following kernel parameter.
> 
> 	scsi_mod.persistent_name=1
> 
> Also, if you want to assign a device name with sda, add the
> following udev rules.
> 
> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
> -c 'echo -n sda > /sys/%p/disk_name'"

Also, where is the "real" program you have created to properly name
devices from userspace?  You need that to properly test this patch,
right?

thanks,

greg k-h

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

Hi,

(2011/04/06 1:14), Greg KH wrote:
> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
>> This patch series provides a SCSI option for persistent device
>> names in kernel. With this option, user can always assign a
>> same device name (e.g. sda) to a specific device according to
>> udev rules and device id.
>>
>> Issue:
>> Recently, kernel registers block devices in parallel. As a result,
>> different device names will be assigned at each boot time. This
>> will confuse file-system mounter, thus we usually use persistent
>> symbolic links provided by udev. However, dmesg and procfs outputs
>> show device names instead of the symbolic link names. This causes
>> a serious problem when managing multiple devices (e.g. on a
>> large-scale storage), because usually, device errors are output
>> with device names on dmesg. We also concern about some commands
>> which output device names, as well as kernel messages.
>>
>> Solution:
>> To assign a persistent device name, I create unnamed devices (not
>> a block device, but an intermediate device. users cannot read/write
>> this device). When scsi device driver finds a LU, the LU is registered
>> as an unnamed device and inform to udev. After udev finds the unnamed
>> device, udev assigns a persistent device name to a specific device
>> according to udev rules and registers it as a block device. Hence,
>> this is just a naming, not a renaming.
>>
>> Some users are satisfied with current udev solution. Therefore, my
>> proposal is implemented as an option.
>>
>> If using this option, add the following kernel parameter.
>>
>> 	scsi_mod.persistent_name=1
>>
>> Also, if you want to assign a device name with sda, add the
>> following udev rules.
>>
>> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
>> -c 'echo -n sda > /sys/%p/disk_name'"
> 
> Also, where is the "real" program you have created to properly name
> devices from userspace?  You need that to properly test this patch,
> right?
> 

In the udev rule described above, notation “xxx” indicated by
ATTR(disk_id) is scsi id given by disk. Then, when udev finds this rule,
"/bin/sh -c 'echo -n sda>  /sys/%p/disk_name'" indicated by PROGRAM is
operated using xxx (scsi id) if udev find the disk with scic id xxx.
Thus, persistent device name is assigned.

I have tested this patch using the udev rule. and It works well.


> thanks,
> 
> greg k-h
> 

Thanks,

-- 
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email： nao.nishijima.xt@hitachi.com

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

Hi,

(2011/04/06 1:14), Greg KH wrote:
> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
>> This patch series provides a SCSI option for persistent device
>> names in kernel. With this option, user can always assign a
>> same device name (e.g. sda) to a specific device according to
>> udev rules and device id.
>>
>> Issue:
>> Recently, kernel registers block devices in parallel. As a result,
>> different device names will be assigned at each boot time. This
>> will confuse file-system mounter, thus we usually use persistent
>> symbolic links provided by udev.
> 
> Yes, that is what you should use if you care about this.
> 
>> However, dmesg and procfs outputs
>> show device names instead of the symbolic link names. This causes
>> a serious problem when managing multiple devices (e.g. on a
>> large-scale storage), because usually, device errors are output
>> with device names on dmesg.
> 
> Then fix your tools that read the output of these files to point to the
> proper persistent name instead of trying to get the kernel to solve the
> problem.
> 

Yes, the tools should be revised if users get a device name using them.

The problem I would like to discuss here is that users can not identify
a disk from kernel messages when they DIRECTLY refer to these messages.
For example, a device name is used instead of a symbolic link names in
bootup messages, I/O devices errors and /proc/partitions …etc.

In particular, users can not identify an appropriate device from a
device name in syslog since different device name may be assigned to it
at each boot time.

My idea is able to fix this issue with small changes in scsi subsystem.
Also, it is implemented as an option. Therefore, it does not affect
users who do not select this option.


>> We also concern about some commands
>> which output device names, as well as kernel messages.
> 
> What commands are you concerned about?
> 

They are iostat, df, fdisk, parted and so on.
For example, the following sdc did not point a same disk.

# ls -l /dev/disk/by-id
...
lrwxrwxrwx. 1 root root  9 Apr  8 11:17 wwn-0x50014ee057a61330 -> ../../sdc
lrwxrwxrwx. 1 root root  9 Apr  8 11:17 wwn-0x50014ee204d3fcda -> ../../sdd

# iostat
...
Device:            tps   Blk_read/s   Blk_wrtn/s   Blk_read   Blk_wrtn
...
sdc               5.78        19.25        78.98     394124    1616821
sdd               7.66       893.45         0.86   18289518      17604

After rebooting, the same symbolic link name pointed different device name.


# ls -l /dev/disk/by-id
...
lrwxrwxrwx. 1 root root  9 Apr  8 11:23 wwn-0x50014ee204d3fcda -> ../../sdc
lrwxrwxrwx. 1 root root  9 Apr  8 11:23 wwn-0x50014ee057a61330 -> ../../sdd


# iostat
...
Device:            tps   Blk_read/s   Blk_wrtn/s   Blk_read   Blk_wrtn
...
sdc               7.65       892.22         0.86   18289518      17604
sdd               5.78        19.23        78.90     394124    1617447

>> Solution:
>> To assign a persistent device name, I create unnamed devices (not
>> a block device, but an intermediate device. users cannot read/write
>> this device). When scsi device driver finds a LU, the LU is registered
>> as an unnamed device and inform to udev. After udev finds the unnamed
>> device, udev assigns a persistent device name to a specific device
>> according to udev rules and registers it as a block device. Hence,
>> this is just a naming, not a renaming.
> 
> Ick, so the kernel waits for userspace before you are able to use the
> device?  That sounds messy.
> 

This option only affects the system where users selected it. Therefore,
I think that the unnamed device should not be available before users
give the name.


>> Some users are satisfied with current udev solution. Therefore, my
>> proposal is implemented as an option.
>>
>> If using this option, add the following kernel parameter.
>>
>> 	scsi_mod.persistent_name=1
>>
>> Also, if you want to assign a device name with sda, add the
>> following udev rules.
>>
>> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
>> -c 'echo -n sda > /sys/%p/disk_name'"
>>
>> Todo:
>> - usb support
> 
> Why?  USB uses scsi, so why would it not work as-is today?  What about
> libata devices?
> 

Sorry, my explanation was not sufficient.
It is required to get scsi id using a scsi inguiry command in order to
create the udev rule, especially in ATTR(disk_id) item in it. The USB
devices, however, do not respond to the command and we can not get their
scsi id.


>> - hot-remove support
> 
> So once you have assigned a name and the device is removed bad things
> happen?  What is the problem with this?
> 

I am deeply sorry for that I forgot including the release function in
this patch.


> Note, any review of the code below does not imply that I agree with the
> ideas here at all.  In fact, I really don't like this idea at all, but I
> might as well review the code anyway for your future contributions :)
> 

I am profoundly grateful for your review :)


>>
>> I've already started discussion about device naming at LKML.
>> https://lkml.org/lkml/2010/10/8/31
>>
>> Signed-off-by: Nao Nishijima <nao.nishijima.xt@hitachi.com>
>> ---
>>
>>  drivers/scsi/Makefile       |    1 
>>  drivers/scsi/scsi_sysfs.c   |   26 +++
>>  drivers/scsi/scsi_unnamed.c |  340 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/scsi_unnamed.h |   25 +++
>>  4 files changed, 387 insertions(+), 5 deletions(-)
>>  create mode 100644 drivers/scsi/scsi_unnamed.c
>>  create mode 100644 drivers/scsi/scsi_unnamed.h
>>
>> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
>> index 7ad0b8a..782adc1 100644
>> --- a/drivers/scsi/Makefile
>> +++ b/drivers/scsi/Makefile
>> @@ -167,6 +167,7 @@ scsi_mod-$(CONFIG_SYSCTL)	+= scsi_sysctl.o
>>  scsi_mod-$(CONFIG_SCSI_PROC_FS)	+= scsi_proc.o
>>  scsi_mod-y			+= scsi_trace.o
>>  scsi_mod-$(CONFIG_PM)		+= scsi_pm.o
>> +scsi_mod-y			+= scsi_unnamed.o
>>  
>>  scsi_tgt-y			+= scsi_tgt_lib.o scsi_tgt_if.o
>>  
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index e44ff64..7959f5d 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -22,6 +22,7 @@
>>  
>>  #include "scsi_priv.h"
>>  #include "scsi_logging.h"
>> +#include "scsi_unnamed.h"
>>  
>>  static struct device_type scsi_dev_type;
>>  
>> @@ -393,13 +394,27 @@ int scsi_sysfs_register(void)
>>  {
>>  	int error;
>>  
>> +	error = scsi_unnamed_register(&scsi_bus_type);
>> +	if (error)
>> +		goto cleanup;
>> +
>>  	error = bus_register(&scsi_bus_type);
>> -	if (!error) {
>> -		error = class_register(&sdev_class);
>> -		if (error)
>> -			bus_unregister(&scsi_bus_type);
>> -	}
>> +	if (error)
>> +		goto cleanup_scsi_unnamed;
>> +
>> +	error = class_register(&sdev_class);
>> +	if (error)
>> +		goto cleanup_bus;
>> +
>> +	return 0;
>> +
>> +cleanup_bus:
>> +	bus_unregister(&scsi_bus_type);
>> +
>> +cleanup_scsi_unnamed:
>> +	scsi_unnamed_unregister();
>>  
>> +cleanup:
>>  	return error;
>>  }
>>  
>> @@ -407,6 +422,7 @@ void scsi_sysfs_unregister(void)
>>  {
>>  	class_unregister(&sdev_class);
>>  	bus_unregister(&scsi_bus_type);
>> +	scsi_unnamed_unregister();
>>  }
>>  
>>  /*
>> diff --git a/drivers/scsi/scsi_unnamed.c b/drivers/scsi/scsi_unnamed.c
>> new file mode 100644
>> index 0000000..ed96945
>> --- /dev/null
>> +++ b/drivers/scsi/scsi_unnamed.c
>> @@ -0,0 +1,340 @@
>> +/*
>> + * scsi_unnamed.c - SCSI driver for unnmaed device
>> + *
>> + * Copyright: Copyright (c) Hitachi, Ltd. 2011
>> + *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
> 
> Do you _REALLY_ mean "any later version?  Are you sure about that?  If
> so, fine, just be careful please.
> 

OK, I will remove those paragraph.

>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA
> 
> Unless you wish to track the movements of the FSF's office space for the
> next 40+ years and keep this file up to date, just remove this paragraph
> please.
> 

OK, I will remove those paragraph.


>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <linux/kobject.h>
>> +#include <linux/kdev_t.h>
>> +#include <linux/sysdev.h>
>> +#include <linux/list.h>
>> +#include <linux/wait.h>
>> +
>> +#include <scsi/scsi_driver.h>
>> +#include <scsi/scsi_device.h>
>> +#include <scsi/scsi.h>
>> +
>> +#define MAX_BUFFER_LEN	256
>> +#define DISK_NAME_LEN	32
> 
> Why this limitation?
> 

Device name limitation which used kernel is 32.

include/linux/genhd.h
...
#define DISK_NAME_LEN                   32
...
struct gendisk {
...
char disk_name[DISK_NAME_LEN];  /* name of major driver */

I will include genhd.h.


>> +
>> +#define SCSI_ID_BINARY	1
>> +#define SCSI_ID_ASCII	2
>> +#define SCSI_ID_UTF8	3
>> +
>> +static LIST_HEAD(su_list);
>> +static DEFINE_MUTEX(su_list_lock);
>> +
>> +static int persistent_name;
>> +MODULE_PARM_DESC(persistent_name, "SCSI unnamed device support");
>> +module_param(persistent_name, bool, S_IRUGO);
> 
> Why is this a module parameter?
> 

I implemented this idea as an option.


>> +
>> +static struct class su_sysfs_class = {
>> +	.name	= "scsi_unnamed",
>> +};
> 
> Why a static class?  What is it used for?  If you are adding sysfs
> files, you need to also add Documentation/ABI/ entries at the same time.
> Please do that if you really are adding new sysfs files.
> 

OK, I have a rethink on it.

>> +
>> +struct scsi_unnamed {
>> +	struct list_head list;
>> +	struct device dev;
>> +	char disk_name[DISK_NAME_LEN];
>> +	char disk_lun[MAX_BUFFER_LEN];
>> +	char disk_id[MAX_BUFFER_LEN];
>> +	bool assigned;
>> +};
>> +
>> +#define to_scsi_unnamed(d) \
>> +	container_of(d, struct scsi_unnamed, dev)
>> +
>> +/* Get device identification VPD page */
>> +static void store_disk_id(struct scsi_device *sdev, char *disk_id)
>> +{
>> +	unsigned char *page_83;
>> +	int page_len, id_len, j = 0, i = 8;
>> +	static const char hex_str[] = "0123456789abcdef";
> 
> Don't we have functions that do this already?
> 

I will use pack_hex_byte(), thanks.

>> +
>> +	page_83 = kmalloc(MAX_BUFFER_LEN, GFP_KERNEL);
>> +	if (!page_83)
>> +		return;
>> +
>> +	if (scsi_get_vpd_page(sdev, 0x83, page_83, MAX_BUFFER_LEN))
>> +		goto err;
>> +
>> +	page_len = ((page_83[2] << 8) | page_83[3]) + 4;
>> +	if (page_len > 0) {
>> +		if ((page_83[5] & 0x30) != 0)
>> +			goto err;
>> +
>> +		id_len = page_83[7];
>> +		if (id_len > 0) {
>> +			switch (page_83[4] & 0x0f) {
>> +			case SCSI_ID_BINARY:
>> +				while (i < id_len) {
>> +					disk_id[j++] = hex_str[(page_83[i]
>> +						& 0xf0) >> 4];
>> +					disk_id[j++] = hex_str[page_83[i]
>> +						& 0x0f];
>> +					i++;
>> +				}
>> +				break;
>> +			case SCSI_ID_ASCII:
>> +			case SCSI_ID_UTF8:
>> +				strncpy(disk_id, strim(page_83 + 8), id_len);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +err:
>> +	kfree(page_83);
>> +	return;
>> +}
>> +
>> +static ssize_t show_disk_name(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_name);
>> +}
>> +
>> +static ssize_t show_disk_lun(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_lun);
>> +}
>> +
>> +static ssize_t show_disk_id(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	return sprintf(buf, "%s\n", to_scsi_unnamed(dev)->disk_id);
>> +}
>> +
>> +/* Assign a persistent device name */
>> +static ssize_t store_disk_name(struct device *dev,
>> +		struct device_attribute *attr, char *buf, size_t count)
>> +{
>> +	struct scsi_unnamed *su = to_scsi_unnamed(dev);
>> +	struct scsi_unnamed *tmp;
>> +	struct device *lu = dev->parent;
>> +	int ret = -EINVAL;
>> +
>> +	if (count >= DISK_NAME_LEN) {
>> +		printk(KERN_WARNING "su: Too long a persistent name!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (su->assigned) {
>> +		printk(KERN_WARNING "su: Already assigned a persistent name!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (strncmp(lu->driver->name, buf, 2)) {
>> +		printk(KERN_WARNING "su: Wrong prefix!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&su_list_lock);
>> +	list_for_each_entry(tmp, &su_list, list) {
>> +		if (!strncmp(tmp->disk_name, buf, DISK_NAME_LEN)) {
>> +			printk(KERN_WARNING "su: Duplicate name exsists!\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +	strncpy(su->disk_name, buf, DISK_NAME_LEN);
>> +	mutex_unlock(&su_list_lock);
>> +
>> +	if (!lu->driver->probe)
>> +		return -EINVAL;
>> +
>> +	lu->init_name = buf;
>> +
>> +	ret = lu->driver->probe(lu);
>> +	if (ret) {
>> +		lu->init_name = NULL;
>> +		su->disk_name[0] = '\0';
>> +		return ret;
>> +	}
>> +
>> +	su->assigned = true;
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR(disk_name, S_IRUGO|S_IWUSR, show_disk_name,
>> +			store_disk_name);
>> +static DEVICE_ATTR(disk_lun, S_IRUGO, show_disk_lun, NULL);
>> +static DEVICE_ATTR(disk_id, S_IRUGO, show_disk_id, NULL);
>> +
>> +/* Confirm the driver name and the device type */
>> +static int check_device_type(char type, const char *name)
>> +{
>> +	switch (type) {
>> +	case TYPE_DISK:
>> +	case TYPE_MOD:
>> +	case TYPE_RBC:
>> +		if (strncmp("sd", name, 2))
>> +			return -ENODEV;
>> +		break;
>> +	case TYPE_ROM:
>> +	case TYPE_WORM:
>> +		if (strncmp("sr", name, 2))
>> +			return -ENODEV;
>> +		break;
>> +	case TYPE_TAPE:
>> +		if (strncmp("st", name, 2))
>> +			return -ENODEV;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * scsi_unnamed_probe - Setup the unnamed device.
>> + * @dev: parent scsi device
>> + *
>> + * This function adds a unnamed device to the device model and
>> + * gets a number of device information by scsi inquiry commands.
>> + * Udev identify a device by those information.
>> + *
>> + * Unnamed devices:
>> + * This device is not a block device and user can not read/write
>> + * this device. But it can advertise device information in sysfs.
>> + */
>> +int scsi_unnamed_probe(struct device *dev)
>> +{
>> +	struct scsi_unnamed *su;
>> +	struct scsi_device *sdev = to_scsi_device(dev);
>> +	int ret = -EINVAL;
>> +	static int i;
>> +
>> +	if (check_device_type(sdev->type, dev->driver->name))
>> +		return -ENODEV;
>> +
>> +	su = kzalloc(sizeof(*su), GFP_KERNEL);
>> +	if (!su)
>> +		return -ENOMEM;
>> +
>> +	device_initialize(&su->dev);
>> +	su->dev.parent = dev;
>> +	su->dev.class = &su_sysfs_class;
> 
> You just created a device that can not be removed from the system (i.e.
> you have no release function.)  This is a major bug and as per the
> documentation for kobjects and the driver model in the kernel tree, I am
> allowed to mock this code mercilessly :)
> 

Oh... sorry :(


>> +	dev_set_name(&su->dev, "su%d", i++);
>> +	strncpy(su->disk_lun, dev_name(dev), MAX_BUFFER_LEN);
>> +
>> +	ret = device_add(&su->dev);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = device_create_file(&su->dev, &dev_attr_disk_name);
> 
> Nope, you just raced with userspace here.  NEVER create a sysfs file
> after you have added it to the system.  Userspace just got notified of
> the device and is already starting to read the file.  As it's not there
> yet, you just failed :(
> 
> Use attributes properly to keep this race from happening.
> 

OK. I'll try to be more careful in future


> 
>> +	if (ret)
>> +		goto err_disk_name;
>> +
>> +	ret = device_create_file(&su->dev, &dev_attr_disk_lun);
>> +	if (ret)
>> +		goto err_disk_lun;
>> +
>> +	store_disk_id(sdev, su->disk_id);
>> +	if (su->disk_id[0] != '\0') {
>> +		ret = device_create_file(&su->dev, &dev_attr_disk_id);
>> +		if (ret)
>> +			goto err_disk_id;
>> +	}
>> +
>> +	su->assigned = false;
>> +	mutex_lock(&su_list_lock);
>> +	list_add(&su->list, &su_list);
>> +	mutex_unlock(&su_list_lock);
>> +
>> +	return 0;
>> +
>> +err_disk_id:
>> +	device_remove_file(&su->dev, &dev_attr_disk_lun);
>> +
>> +err_disk_lun:
>> +	device_remove_file(&su->dev, &dev_attr_disk_name);
>> +
>> +err_disk_name:
>> +	device_del(&su->dev);
>> +
>> +err:
>> +	kfree(su);
>> +	return ret;
>> +}
>> +
>> +/* Remove a unnamed device from su_list and release resources */
>> +void scsi_unnamed_remove(struct device *dev)
>> +{
>> +	struct scsi_unnamed *tmp;
>> +
>> +	mutex_lock(&su_list_lock);
>> +	list_for_each_entry(tmp, &su_list, list) {
>> +		if (dev == tmp->dev.parent) {
>> +			list_del(&tmp->list);
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&su_list_lock);
>> +
>> +	if (tmp->disk_id[0] != '\0')
>> +		device_remove_file(&tmp->dev, &dev_attr_disk_id);
>> +	device_remove_file(&tmp->dev, &dev_attr_disk_name);
>> +	device_remove_file(&tmp->dev, &dev_attr_disk_lun);
>> +	device_del(&tmp->dev);
> 
> Your kerrnel just spit out some very nasty messages when this function
> was called, right?  Why are you ignoring them?
> 

Sorry. I will check it.


>> +
>> +	device_release_driver(dev);
>> +
>> +	kfree(tmp);
>> +}
>> +
>> +/**
>> + * copy_persistent_name - Copy the device name
>> + * @disk_name: allocate to
>> + * @dev: scsi device
>> + *
>> + * Copy an initial name of the device to disk_name.
>> + */
>> +int copy_persistent_name(char *disk_name, struct device *dev)
>> +{
>> +	if (persistent_name) {
>> +		strncpy(disk_name, dev->init_name, DISK_NAME_LEN);
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(copy_persistent_name);
> 
> That's a very bad global function name.
> 

I have a bad sense :(
I will name this function scsi_unnamed_copy_persistent_name.


> EXPORT_SYMBOL_GPL?  And why is it exported at all?  Who would ever need
> to call this from a module?
> 

Please see the patch 2/2. This function is called by external

>> +
>> +int scsi_unnamed_register(struct bus_type *bus)
>> +{
>> +	if (persistent_name) {
>> +		bus->probe = scsi_unnamed_probe;
>> +		bus->remove = scsi_unnamed_remove;
>> +		return class_register(&su_sysfs_class);
>> +	}
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(scsi_unnamed_register);
> 
> Same here, why is this exported?
> 
>> +
>> +void scsi_unnamed_unregister(void)
>> +{
>> +	if (persistent_name)
>> +		class_unregister(&su_sysfs_class);
>> +}
>> +EXPORT_SYMBOL(scsi_unnamed_unregister);
> 
> And here.
> 
>> diff --git a/drivers/scsi/scsi_unnamed.h b/drivers/scsi/scsi_unnamed.h
>> new file mode 100644
>> index 0000000..ca4e852
>> --- /dev/null
>> +++ b/drivers/scsi/scsi_unnamed.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * scsi_unnamed.h - SCSI driver for unnmaed device
>> + *
>> + * Copyright: Copyright (c) Hitachi, Ltd. 2011
>> + *            Authors: Nao Nishijima <nao.nishijima.xt@hitachi.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
> 
> Same comment as above.
> 
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111 USA
> 
> And again, same comment as above.
> 
> thanks,
> 
> greg k-h
> 

Thanks,

-- 
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email： nao.nishijima.xt@hitachi.com

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Hannes Reinecke]

On 04/08/2011 07:12 AM, Nao Nishijima wrote:
> Hi,
> 
> (2011/04/06 1:14), Greg KH wrote:
>> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
>>> This patch series provides a SCSI option for persistent device
>>> names in kernel. With this option, user can always assign a
>>> same device name (e.g. sda) to a specific device according to
>>> udev rules and device id.
>>>
>>> Issue:
>>> Recently, kernel registers block devices in parallel. As a result,
>>> different device names will be assigned at each boot time. This
>>> will confuse file-system mounter, thus we usually use persistent
>>> symbolic links provided by udev.
>>
>> Yes, that is what you should use if you care about this.
>>
>>> However, dmesg and procfs outputs
>>> show device names instead of the symbolic link names. This causes
>>> a serious problem when managing multiple devices (e.g. on a
>>> large-scale storage), because usually, device errors are output
>>> with device names on dmesg.
>>
>> Then fix your tools that read the output of these files to point to the
>> proper persistent name instead of trying to get the kernel to solve the
>> problem.
>>
> 
> Yes, the tools should be revised if users get a device name using them.
> 
> The problem I would like to discuss here is that users can not identify
> a disk from kernel messages when they DIRECTLY refer to these messages.
> For example, a device name is used instead of a symbolic link names in
> bootup messages, I/O devices errors and /proc/partitions …etc.
> 
> In particular, users can not identify an appropriate device from a
> device name in syslog since different device name may be assigned to it
> at each boot time.
> 
> My idea is able to fix this issue with small changes in scsi subsystem.
> Also, it is implemented as an option. Therefore, it does not affect
> users who do not select this option.
> 
We have been discussing this problem several times in the past, and
indeed on these very mailing list.

The conclusion we arrived at is that the kernel-provided device node
name is inherently unstable and impossible to fix within the existing
'sdX' naming scheme.
So the choices have been to either move to a totally different naming
scheme or keep the naming scheme and provide the required information
by other means.
We have decided on the latter, and agreed on using udev to provide
persistent device names.
We are fully aware that any kernel related messages are subject to
chance after reboot, but then most kernel related messages are
(PID number, timestamps, login tty etc).
And also we are aware that any kernel messages need to be matched
against the current system layout to figure out any hardware-related
issue.

But then basically all products requiring to filter out information
from kernel messages already do so I don't see a problem with that.

Just adding an in-kernel identifier to the LUN will only be an
incomplete solution, as other identifiers will still be volatile.

So I would prefer by keeping the in-kernel information as small
as possible to reduce memory consumption and rely on out-of-band
programs to provide the required mapping.

Cheers,

Hannes
--
Dr. Hannes Reinecke              zSeries & Storage
hare@suse.de                  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[James Bottomley]

On Fri, 2011-04-08 at 07:33 -0700, Hannes Reinecke wrote:
> > The problem I would like to discuss here is that users can not identify
> > a disk from kernel messages when they DIRECTLY refer to these messages.
> > For example, a device name is used instead of a symbolic link names in
> > bootup messages, I/O devices errors and /proc/partitions …etc.
> > 
> > In particular, users can not identify an appropriate device from a
> > device name in syslog since different device name may be assigned to it
> > at each boot time.
> > 
> > My idea is able to fix this issue with small changes in scsi subsystem.
> > Also, it is implemented as an option. Therefore, it does not affect
> > users who do not select this option.
> > 
> We have been discussing this problem several times in the past, and
> indeed on these very mailing list.
> 
> The conclusion we arrived at is that the kernel-provided device node
> name is inherently unstable and impossible to fix within the existing
> 'sdX' naming scheme.
> So the choices have been to either move to a totally different naming
> scheme or keep the naming scheme and provide the required information
> by other means.
> We have decided on the latter, and agreed on using udev to provide
> persistent device names.
> We are fully aware that any kernel related messages are subject to
> chance after reboot, but then most kernel related messages are
> (PID number, timestamps, login tty etc).
> And also we are aware that any kernel messages need to be matched
> against the current system layout to figure out any hardware-related
> issue.
> 
> But then basically all products requiring to filter out information
> from kernel messages already do so I don't see a problem with that.
> 
> Just adding an in-kernel identifier to the LUN will only be an
> incomplete solution, as other identifiers will still be volatile.
> 
> So I would prefer by keeping the in-kernel information as small
> as possible to reduce memory consumption and rely on out-of-band
> programs to provide the required mapping.

So, while I agree totally with the above: udev and userspace is the way
to go, I'm not totally opposed to having a non-invasive mechanism for
indicating a user's preferred name for a device.  I think there are a
couple of ways to do this:

     1. Entirely in userspace: just have udev consult a preferred name
        file and create say /dev/disk/by-preferred.  Then have all the
        tools that normally output device information do the same (i.e.
        since real name to preferred name is 1:1, they could all do a
        reverse lookup).
     2. have a writeable sysfs preferred_name field, either in the
        generic device or just in SCSI.  The preferred name would be
        used by outbound only (i.e. kernel dev_printk messages and
        possibly /proc/partitions).  All inbound uses of the device
        would come via the standard udev mechanisms
        (i.e. /dev/disk/by-preferred would be the usual symlink).  This
        means from the kernel point of view, no renaming has happened.
        We'd just try to print out the preferred name in certain
        circumstances, which should solve most of the described problem.

James

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Fri, Apr 08, 2011 at 11:07:41PM +0900, Nao Nishijima wrote:
> Hi,
> 
> (2011/04/06 1:14), Greg KH wrote:
> > On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> >> This patch series provides a SCSI option for persistent device
> >> names in kernel. With this option, user can always assign a
> >> same device name (e.g. sda) to a specific device according to
> >> udev rules and device id.
> >>
> >> Issue:
> >> Recently, kernel registers block devices in parallel. As a result,
> >> different device names will be assigned at each boot time. This
> >> will confuse file-system mounter, thus we usually use persistent
> >> symbolic links provided by udev. However, dmesg and procfs outputs
> >> show device names instead of the symbolic link names. This causes
> >> a serious problem when managing multiple devices (e.g. on a
> >> large-scale storage), because usually, device errors are output
> >> with device names on dmesg. We also concern about some commands
> >> which output device names, as well as kernel messages.
> >>
> >> Solution:
> >> To assign a persistent device name, I create unnamed devices (not
> >> a block device, but an intermediate device. users cannot read/write
> >> this device). When scsi device driver finds a LU, the LU is registered
> >> as an unnamed device and inform to udev. After udev finds the unnamed
> >> device, udev assigns a persistent device name to a specific device
> >> according to udev rules and registers it as a block device. Hence,
> >> this is just a naming, not a renaming.
> >>
> >> Some users are satisfied with current udev solution. Therefore, my
> >> proposal is implemented as an option.
> >>
> >> If using this option, add the following kernel parameter.
> >>
> >> 	scsi_mod.persistent_name=1
> >>
> >> Also, if you want to assign a device name with sda, add the
> >> following udev rules.
> >>
> >> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
> >> -c 'echo -n sda > /sys/%p/disk_name'"
> > 
> > Also, where is the "real" program you have created to properly name
> > devices from userspace?  You need that to properly test this patch,
> > right?
> > 
> 
> In the udev rule described above, notation “xxx” indicated by
> ATTR(disk_id) is scsi id given by disk. Then, when udev finds this rule,
> "/bin/sh -c 'echo -n sda>  /sys/%p/disk_name'" indicated by PROGRAM is
> operated using xxx (scsi id) if udev find the disk with scic id xxx.
> Thus, persistent device name is assigned.
> 
> I have tested this patch using the udev rule. and It works well.

That's nice, but it's a "toy".  What have you used to test this with
20000 scsi devices to properly work like it does today?  Where is the
program that will name them in a deterministic manner?

We have that functionality today, you can't break it.

thanks,

greg k-h

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Fri, Apr 08, 2011 at 08:14:12AM -0700, James Bottomley wrote:
> So, while I agree totally with the above: udev and userspace is the way
> to go, I'm not totally opposed to having a non-invasive mechanism for
> indicating a user's preferred name for a device.  I think there are a
> couple of ways to do this:
> 
>      1. Entirely in userspace: just have udev consult a preferred name
>         file and create say /dev/disk/by-preferred.  Then have all the
>         tools that normally output device information do the same (i.e.
>         since real name to preferred name is 1:1, they could all do a
>         reverse lookup).
>      2. have a writeable sysfs preferred_name field, either in the
>         generic device or just in SCSI.  The preferred name would be
>         used by outbound only (i.e. kernel dev_printk messages and
>         possibly /proc/partitions).  All inbound uses of the device
>         would come via the standard udev mechanisms
>         (i.e. /dev/disk/by-preferred would be the usual symlink).  This
>         means from the kernel point of view, no renaming has happened.
>         We'd just try to print out the preferred name in certain
>         circumstances, which should solve most of the described problem.

Either, or both, of those options are fine with me as well.

thanks,

greg k-h

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Kay Sievers]

On Fri, Apr 8, 2011 at 18:14, Greg KH <greg@kroah.com> wrote:
> On Fri, Apr 08, 2011 at 08:14:12AM -0700, James Bottomley wrote:
>> So, while I agree totally with the above: udev and userspace is the way
>> to go, I'm not totally opposed to having a non-invasive mechanism for
>> indicating a user's preferred name for a device.  I think there are a
>> couple of ways to do this:
>>
>>      1. Entirely in userspace: just have udev consult a preferred name
>>         file and create say /dev/disk/by-preferred.  Then have all the
>>         tools that normally output device information do the same (i.e.
>>         since real name to preferred name is 1:1, they could all do a
>>         reverse lookup).
>>      2. have a writeable sysfs preferred_name field, either in the
>>         generic device or just in SCSI.  The preferred name would be
>>         used by outbound only (i.e. kernel dev_printk messages and
>>         possibly /proc/partitions).  All inbound uses of the device
>>         would come via the standard udev mechanisms
>>         (i.e. /dev/disk/by-preferred would be the usual symlink).  This
>>         means from the kernel point of view, no renaming has happened.
>>         We'd just try to print out the preferred name in certain
>>         circumstances, which should solve most of the described problem.
>
> Either, or both, of those options are fine with me as well.

I guess a generic sysfs file, that can carry a user-given name for a
device, and which is looked-up with dev_printk() macro, and maybe
/proc/partitions is all what we would need if we want to involve the
kernel here.

Kay

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Stefan Richter]

On Apr 08 Nao Nishijima wrote:
> (2011/04/06 1:14), Greg KH wrote:
> > On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> >> Todo:
> >> - usb support
> > 
> > Why?  USB uses scsi, so why would it not work as-is today?  What about
> > libata devices?
> > 
> 
> Sorry, my explanation was not sufficient.
> It is required to get scsi id using a scsi inguiry command in order to
> create the udev rule, especially in ATTR(disk_id) item in it. The USB
> devices, however, do not respond to the command and we can not get their
> scsi id.

Identifiers and names of target ports and of logical units (per SAM-2 and
later, Annex A) --- whether they are persistent, in which scope they are
unique, how they look like, and how they can be obtained --- are transport
specific.

Neither the INQUIRY command (per SPC) nor any other command is generally
useful to obtain e.g. logical unit identifiers.  Any tool that needs to
obtain identifiers or names of target ports and of logical units must talk
with the transport driver through driver-specific interfaces.

Years ago it was suggested on this list that scsi-mod exposes standard
sysfs attributes for target and LU identifiers and names for all devices,
so that tools that need identfiers or names have a single place where they
can look them up.  (The read() method of these attributes would have to
be provided kernel-internally by transport drivers; and tools that read
these attributes need to be aware that there is not a single format for
identifiers and names.)

The SCSI folks were not interested in such a more standardized sysfs ABI
at that time.  (There was only a proposal anyway, not a patch.)  Hence,
tools have to dig at various places for these SAM-2 artifacts.  These ABIs
have been defined ad hoc by the transport driver implementers.

Anyway.  /How/ to obtain identifiers is just a side issue.  Back to the
proposal of letting userland tell the kernel how to name devices:

> >> I create unnamed devices (not
> >> a block device, but an intermediate device.
[...]
> >> After udev finds the unnamed
> >> device, udev assigns a persistent device name to a specific device
> >> according to udev rules and registers it as a block device. Hence,
> >> this is just a naming, not a renaming.

I disagree to this conclusion.  The "unnamed" device most definitely will
have a name before it can be shown to userland.  This preliminary name
fulfills the very same requirement that "s[drt][a-z]+[0-9]*" fulfill
today:  It is unique within the system during the lifetime of the
device.  So, this /is/ renaming, only that the first name is only exposed
to special tools that know of this new ABI.

It seems to me that today's procedure of /not/ renaming devices but of
providing as many alternative additional names as anyone could ever need
(symlinks within /dev/) is more robust.

If the contents of /var/log/messages or the output of iostat etc. is not
what is needed, how about simply filtering that through a grep/ sed based
script that rewrites names?  (This needs to be run during the lifetime of
the device of course, otherwise a wrong name could be put in.)  The fine
symlinks that udev provides can be used in such a text filter.
-- 
Stefan Richter
-=====-==-== -=-- -=---
http://arcgraph.de/sr/

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

Hi, James

(2011/04/09 0:14), James Bottomley wrote:
> On Fri, 2011-04-08 at 07:33 -0700, Hannes Reinecke wrote:
>>> The problem I would like to discuss here is that users can not identify
>>> a disk from kernel messages when they DIRECTLY refer to these messages.
>>> For example, a device name is used instead of a symbolic link names in
>>> bootup messages, I/O devices errors and /proc/partitions …etc.
>>>
>>> In particular, users can not identify an appropriate device from a
>>> device name in syslog since different device name may be assigned to it
>>> at each boot time.
>>>
>>> My idea is able to fix this issue with small changes in scsi subsystem.
>>> Also, it is implemented as an option. Therefore, it does not affect
>>> users who do not select this option.
>>>
>> We have been discussing this problem several times in the past, and
>> indeed on these very mailing list.
>>
>> The conclusion we arrived at is that the kernel-provided device node
>> name is inherently unstable and impossible to fix within the existing
>> 'sdX' naming scheme.
>> So the choices have been to either move to a totally different naming
>> scheme or keep the naming scheme and provide the required information
>> by other means.
>> We have decided on the latter, and agreed on using udev to provide
>> persistent device names.
>> We are fully aware that any kernel related messages are subject to
>> chance after reboot, but then most kernel related messages are
>> (PID number, timestamps, login tty etc).
>> And also we are aware that any kernel messages need to be matched
>> against the current system layout to figure out any hardware-related
>> issue.
>>
>> But then basically all products requiring to filter out information
>> from kernel messages already do so I don't see a problem with that.
>>
>> Just adding an in-kernel identifier to the LUN will only be an
>> incomplete solution, as other identifiers will still be volatile.
>>
>> So I would prefer by keeping the in-kernel information as small
>> as possible to reduce memory consumption and rely on out-of-band
>> programs to provide the required mapping.
> 
> So, while I agree totally with the above: udev and userspace is the way
> to go, I'm not totally opposed to having a non-invasive mechanism for
> indicating a user's preferred name for a device.  I think there are a
> couple of ways to do this:
> 
>      1. Entirely in userspace: just have udev consult a preferred name
>         file and create say /dev/disk/by-preferred.  Then have all the
>         tools that normally output device information do the same (i.e.
>         since real name to preferred name is 1:1, they could all do a
>         reverse lookup).
>      2. have a writeable sysfs preferred_name field, either in the
>         generic device or just in SCSI.  The preferred name would be
>         used by outbound only (i.e. kernel dev_printk messages and
>         possibly /proc/partitions).  All inbound uses of the device
>         would come via the standard udev mechanisms
>         (i.e. /dev/disk/by-preferred would be the usual symlink).  This
>         means from the kernel point of view, no renaming has happened.
>         We'd just try to print out the preferred name in certain
>         circumstances, which should solve most of the described problem.
> 
> James
> 
> 
> 

I have a question. Why is in-kernel device name necessary? The kernel
can identify a device by major/miner number and udev can create a
device node of a prefer name.

Currently, device names are only used to show to users. Therefore I
think that in-kernel device name is unnecessary if we introduce your
/dev/disk/by-prefferd idea.


thanks,

-- 
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email： nao.nishijima.xt@hitachi.com

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[James Bottomley]

On Tue, 2011-04-12 at 22:23 +0900, Nao Nishijima wrote:
> (2011/04/09 0:14), James Bottomley wrote:
> > On Fri, 2011-04-08 at 07:33 -0700, Hannes Reinecke wrote:
> >>> The problem I would like to discuss here is that users can not identify
> >>> a disk from kernel messages when they DIRECTLY refer to these messages.
> >>> For example, a device name is used instead of a symbolic link names in
> >>> bootup messages, I/O devices errors and /proc/partitions …etc.
> >>>
> >>> In particular, users can not identify an appropriate device from a
> >>> device name in syslog since different device name may be assigned to it
> >>> at each boot time.
> >>>
> >>> My idea is able to fix this issue with small changes in scsi subsystem.
> >>> Also, it is implemented as an option. Therefore, it does not affect
> >>> users who do not select this option.
> >>>
> >> We have been discussing this problem several times in the past, and
> >> indeed on these very mailing list.
> >>
> >> The conclusion we arrived at is that the kernel-provided device node
> >> name is inherently unstable and impossible to fix within the existing
> >> 'sdX' naming scheme.
> >> So the choices have been to either move to a totally different naming
> >> scheme or keep the naming scheme and provide the required information
> >> by other means.
> >> We have decided on the latter, and agreed on using udev to provide
> >> persistent device names.
> >> We are fully aware that any kernel related messages are subject to
> >> chance after reboot, but then most kernel related messages are
> >> (PID number, timestamps, login tty etc).
> >> And also we are aware that any kernel messages need to be matched
> >> against the current system layout to figure out any hardware-related
> >> issue.
> >>
> >> But then basically all products requiring to filter out information
> >> from kernel messages already do so I don't see a problem with that.
> >>
> >> Just adding an in-kernel identifier to the LUN will only be an
> >> incomplete solution, as other identifiers will still be volatile.
> >>
> >> So I would prefer by keeping the in-kernel information as small
> >> as possible to reduce memory consumption and rely on out-of-band
> >> programs to provide the required mapping.
> > 
> > So, while I agree totally with the above: udev and userspace is the way
> > to go, I'm not totally opposed to having a non-invasive mechanism for
> > indicating a user's preferred name for a device.  I think there are a
> > couple of ways to do this:
> > 
> >      1. Entirely in userspace: just have udev consult a preferred name
> >         file and create say /dev/disk/by-preferred.  Then have all the
> >         tools that normally output device information do the same (i.e.
> >         since real name to preferred name is 1:1, they could all do a
> >         reverse lookup).
> >      2. have a writeable sysfs preferred_name field, either in the
> >         generic device or just in SCSI.  The preferred name would be
> >         used by outbound only (i.e. kernel dev_printk messages and
> >         possibly /proc/partitions).  All inbound uses of the device
> >         would come via the standard udev mechanisms
> >         (i.e. /dev/disk/by-preferred would be the usual symlink).  This
> >         means from the kernel point of view, no renaming has happened.
> >         We'd just try to print out the preferred name in certain
> >         circumstances, which should solve most of the described problem.
> > 
> > James
> > 
> > 
> > 
> 
> I have a question. Why is in-kernel device name necessary? The kernel
> can identify a device by major/miner number and udev can create a
> device node of a prefer name.

Well, that's the inbound vs outbound name I referred to above.  If all
you care about is inbound, this is true.

> Currently, device names are only used to show to users. Therefore I
> think that in-kernel device name is unnecessary if we introduce your
> /dev/disk/by-prefferd idea.

So if you have no problem with the kernel still printing out sdX in logs
and it appearing in /proc/partitions, I'm happy with this.

James

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

Hi Hannes

(2011/04/08 23:33), Hannes Reinecke wrote:
> On 04/08/2011 07:12 AM, Nao Nishijima wrote:
>> Hi,
>>
>> (2011/04/06 1:14), Greg KH wrote:
>>> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
>>>> This patch series provides a SCSI option for persistent device
>>>> names in kernel. With this option, user can always assign a
>>>> same device name (e.g. sda) to a specific device according to
>>>> udev rules and device id.
>>>>
>>>> Issue:
>>>> Recently, kernel registers block devices in parallel. As a result,
>>>> different device names will be assigned at each boot time. This
>>>> will confuse file-system mounter, thus we usually use persistent
>>>> symbolic links provided by udev.
>>>
>>> Yes, that is what you should use if you care about this.
>>>
>>>> However, dmesg and procfs outputs
>>>> show device names instead of the symbolic link names. This causes
>>>> a serious problem when managing multiple devices (e.g. on a
>>>> large-scale storage), because usually, device errors are output
>>>> with device names on dmesg.
>>>
>>> Then fix your tools that read the output of these files to point to the
>>> proper persistent name instead of trying to get the kernel to solve the
>>> problem.
>>>
>>
>> Yes, the tools should be revised if users get a device name using them.
>>
>> The problem I would like to discuss here is that users can not identify
>> a disk from kernel messages when they DIRECTLY refer to these messages.
>> For example, a device name is used instead of a symbolic link names in
>> bootup messages, I/O devices errors and /proc/partitions …etc.
>>
>> In particular, users can not identify an appropriate device from a
>> device name in syslog since different device name may be assigned to it
>> at each boot time.
>>
>> My idea is able to fix this issue with small changes in scsi subsystem.
>> Also, it is implemented as an option. Therefore, it does not affect
>> users who do not select this option.
>>
> We have been discussing this problem several times in the past, and
> indeed on these very mailing list.
> 
> The conclusion we arrived at is that the kernel-provided device node
> name is inherently unstable and impossible to fix within the existing
> 'sdX' naming scheme.
> So the choices have been to either move to a totally different naming
> scheme or keep the naming scheme and provide the required information
> by other means.
> We have decided on the latter, and agreed on using udev to provide
> persistent device names.

Could you tell me why you chose the latter?


> We are fully aware that any kernel related messages are subject to
> chance after reboot, but then most kernel related messages are
> (PID number, timestamps, login tty etc).
> And also we are aware that any kernel messages need to be matched
> against the current system layout to figure out any hardware-related
> issue.
> 
> But then basically all products requiring to filter out information
> from kernel messages already do so I don't see a problem with that.
> 

All users did not always use the products. Users can see directly
kernel messages (dmesg, /proc/partitions). Therefore I think that
kernel messages need to provide the required mapping.


> Just adding an in-kernel identifier to the LUN will only be an
> incomplete solution, as other identifiers will still be volatile.
> 
> So I would prefer by keeping the in-kernel information as small
> as possible to reduce memory consumption and rely on out-of-band
> programs to provide the required mapping.
> 
> Cheers,
> 

Thanks,

-- 
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email： nao.nishijima.xt@hitachi.com

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Thu, Apr 14, 2011 at 11:06:33AM +0900, Nao Nishijima wrote:
> Hi Hannes
> 
> (2011/04/08 23:33), Hannes Reinecke wrote:
> > On 04/08/2011 07:12 AM, Nao Nishijima wrote:
> >> Hi,
> >>
> >> (2011/04/06 1:14), Greg KH wrote:
> >>> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> >>>> This patch series provides a SCSI option for persistent device
> >>>> names in kernel. With this option, user can always assign a
> >>>> same device name (e.g. sda) to a specific device according to
> >>>> udev rules and device id.
> >>>>
> >>>> Issue:
> >>>> Recently, kernel registers block devices in parallel. As a result,
> >>>> different device names will be assigned at each boot time. This
> >>>> will confuse file-system mounter, thus we usually use persistent
> >>>> symbolic links provided by udev.
> >>>
> >>> Yes, that is what you should use if you care about this.
> >>>
> >>>> However, dmesg and procfs outputs
> >>>> show device names instead of the symbolic link names. This causes
> >>>> a serious problem when managing multiple devices (e.g. on a
> >>>> large-scale storage), because usually, device errors are output
> >>>> with device names on dmesg.
> >>>
> >>> Then fix your tools that read the output of these files to point to the
> >>> proper persistent name instead of trying to get the kernel to solve the
> >>> problem.
> >>>
> >>
> >> Yes, the tools should be revised if users get a device name using them.
> >>
> >> The problem I would like to discuss here is that users can not identify
> >> a disk from kernel messages when they DIRECTLY refer to these messages.
> >> For example, a device name is used instead of a symbolic link names in
> >> bootup messages, I/O devices errors and /proc/partitions …etc.
> >>
> >> In particular, users can not identify an appropriate device from a
> >> device name in syslog since different device name may be assigned to it
> >> at each boot time.
> >>
> >> My idea is able to fix this issue with small changes in scsi subsystem.
> >> Also, it is implemented as an option. Therefore, it does not affect
> >> users who do not select this option.
> >>
> > We have been discussing this problem several times in the past, and
> > indeed on these very mailing list.
> > 
> > The conclusion we arrived at is that the kernel-provided device node
> > name is inherently unstable and impossible to fix within the existing
> > 'sdX' naming scheme.
> > So the choices have been to either move to a totally different naming
> > scheme or keep the naming scheme and provide the required information
> > by other means.
> > We have decided on the latter, and agreed on using udev to provide
> > persistent device names.
> 
> Could you tell me why you chose the latter?
> 
> 
> > We are fully aware that any kernel related messages are subject to
> > chance after reboot, but then most kernel related messages are
> > (PID number, timestamps, login tty etc).
> > And also we are aware that any kernel messages need to be matched
> > against the current system layout to figure out any hardware-related
> > issue.
> > 
> > But then basically all products requiring to filter out information
> > from kernel messages already do so I don't see a problem with that.
> > 
> 
> All users did not always use the products. Users can see directly
> kernel messages (dmesg, /proc/partitions). Therefore I think that
> kernel messages need to provide the required mapping.

No they don't.  Anyone who wants to look at those files "knows" what
they are doing and the kernel name is fine to use.

thanks,

greg k-h

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Nao Nishijima]

Hi,

(2011/04/09 1:12), Greg KH wrote:
> On Fri, Apr 08, 2011 at 11:07:41PM +0900, Nao Nishijima wrote:
>> Hi,
>>
>> (2011/04/06 1:14), Greg KH wrote:
>>> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
>>>> This patch series provides a SCSI option for persistent device
>>>> names in kernel. With this option, user can always assign a
>>>> same device name (e.g. sda) to a specific device according to
>>>> udev rules and device id.
>>>>
>>>> Issue:
>>>> Recently, kernel registers block devices in parallel. As a result,
>>>> different device names will be assigned at each boot time. This
>>>> will confuse file-system mounter, thus we usually use persistent
>>>> symbolic links provided by udev. However, dmesg and procfs outputs
>>>> show device names instead of the symbolic link names. This causes
>>>> a serious problem when managing multiple devices (e.g. on a
>>>> large-scale storage), because usually, device errors are output
>>>> with device names on dmesg. We also concern about some commands
>>>> which output device names, as well as kernel messages.
>>>>
>>>> Solution:
>>>> To assign a persistent device name, I create unnamed devices (not
>>>> a block device, but an intermediate device. users cannot read/write
>>>> this device). When scsi device driver finds a LU, the LU is registered
>>>> as an unnamed device and inform to udev. After udev finds the unnamed
>>>> device, udev assigns a persistent device name to a specific device
>>>> according to udev rules and registers it as a block device. Hence,
>>>> this is just a naming, not a renaming.
>>>>
>>>> Some users are satisfied with current udev solution. Therefore, my
>>>> proposal is implemented as an option.
>>>>
>>>> If using this option, add the following kernel parameter.
>>>>
>>>> 	scsi_mod.persistent_name=1
>>>>
>>>> Also, if you want to assign a device name with sda, add the
>>>> following udev rules.
>>>>
>>>> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
>>>> -c 'echo -n sda > /sys/%p/disk_name'"
>>>
>>> Also, where is the "real" program you have created to properly name
>>> devices from userspace?  You need that to properly test this patch,
>>> right?
>>>
>>
>> In the udev rule described above, notation “xxx” indicated by
>> ATTR(disk_id) is scsi id given by disk. Then, when udev finds this rule,
>> "/bin/sh -c 'echo -n sda>  /sys/%p/disk_name'" indicated by PROGRAM is
>> operated using xxx (scsi id) if udev find the disk with scic id xxx.
>> Thus, persistent device name is assigned.
>>
>> I have tested this patch using the udev rule. and It works well.
> 
> That's nice, but it's a "toy".  What have you used to test this with
> 20000 scsi devices to properly work like it does today?  Where is the
> program that will name them in a deterministic manner?
> 
> We have that functionality today, you can't break it.
> 
> thanks,

Indeed, I have not (can not, of course) tested 20000 scsi devices to
properly work. This feature targets only scsi disk/cdrom/tape devices.
Not all devices. We expect the target users are system administrators
who need to control all devices for maintenance. They know all devices
connected to the server and those devices will tested before connecting.

We expect that admin will enable this option after installation. This
means that device names are assigned by current mechanism when
installation. Admin enables the option and makes udev rules (we are
planning to make shell script which generate udev rules use by-id and
assigned device names). In other word, this feature will be used just
for "fixing" the names.

In our scenario, admin appends a new rule manually when a new LU is
added. In the future, we are planning to enhance udev to append a new
rule automatically as like as NIC.

Thanks,


-- 
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email： nao.nishijima.xt@hitachi.com

Re: [PATCH 1/2] SCSI: Add a SCSI option for persistent device names in Kernel.

[Greg KH]

On Thu, Apr 14, 2011 at 05:15:36PM +0900, Nao Nishijima wrote:
> Hi,
> 
> (2011/04/09 1:12), Greg KH wrote:
> > On Fri, Apr 08, 2011 at 11:07:41PM +0900, Nao Nishijima wrote:
> >> Hi,
> >>
> >> (2011/04/06 1:14), Greg KH wrote:
> >>> On Tue, Apr 05, 2011 at 09:49:46PM +0900, Nao Nishijima wrote:
> >>>> This patch series provides a SCSI option for persistent device
> >>>> names in kernel. With this option, user can always assign a
> >>>> same device name (e.g. sda) to a specific device according to
> >>>> udev rules and device id.
> >>>>
> >>>> Issue:
> >>>> Recently, kernel registers block devices in parallel. As a result,
> >>>> different device names will be assigned at each boot time. This
> >>>> will confuse file-system mounter, thus we usually use persistent
> >>>> symbolic links provided by udev. However, dmesg and procfs outputs
> >>>> show device names instead of the symbolic link names. This causes
> >>>> a serious problem when managing multiple devices (e.g. on a
> >>>> large-scale storage), because usually, device errors are output
> >>>> with device names on dmesg. We also concern about some commands
> >>>> which output device names, as well as kernel messages.
> >>>>
> >>>> Solution:
> >>>> To assign a persistent device name, I create unnamed devices (not
> >>>> a block device, but an intermediate device. users cannot read/write
> >>>> this device). When scsi device driver finds a LU, the LU is registered
> >>>> as an unnamed device and inform to udev. After udev finds the unnamed
> >>>> device, udev assigns a persistent device name to a specific device
> >>>> according to udev rules and registers it as a block device. Hence,
> >>>> this is just a naming, not a renaming.
> >>>>
> >>>> Some users are satisfied with current udev solution. Therefore, my
> >>>> proposal is implemented as an option.
> >>>>
> >>>> If using this option, add the following kernel parameter.
> >>>>
> >>>> 	scsi_mod.persistent_name=1
> >>>>
> >>>> Also, if you want to assign a device name with sda, add the
> >>>> following udev rules.
> >>>>
> >>>> SUBSYSTEM=="scsi_unnamed", ATTR{disk_id}=="xxx", PROGRAM="/bin/sh
> >>>> -c 'echo -n sda > /sys/%p/disk_name'"
> >>>
> >>> Also, where is the "real" program you have created to properly name
> >>> devices from userspace?  You need that to properly test this patch,
> >>> right?
> >>>
> >>
> >> In the udev rule described above, notation “xxx” indicated by
> >> ATTR(disk_id) is scsi id given by disk. Then, when udev finds this rule,
> >> "/bin/sh -c 'echo -n sda>  /sys/%p/disk_name'" indicated by PROGRAM is
> >> operated using xxx (scsi id) if udev find the disk with scic id xxx.
> >> Thus, persistent device name is assigned.
> >>
> >> I have tested this patch using the udev rule. and It works well.
> > 
> > That's nice, but it's a "toy".  What have you used to test this with
> > 20000 scsi devices to properly work like it does today?  Where is the
> > program that will name them in a deterministic manner?
> > 
> > We have that functionality today, you can't break it.
> > 
> > thanks,
> 
> Indeed, I have not (can not, of course) tested 20000 scsi devices to
> properly work.

Why not?  You should be able to do this easily with a laptop these days
with the dummy scsi device code.

> This feature targets only scsi disk/cdrom/tape devices.
> Not all devices. We expect the target users are system administrators
> who need to control all devices for maintenance. They know all devices
> connected to the server and those devices will tested before connecting.

But that's not the large majority of the people using Linux.  You need a
sane default, which this does not provide.

> We expect that admin will enable this option after installation. This
> means that device names are assigned by current mechanism when
> installation. Admin enables the option and makes udev rules (we are
> planning to make shell script which generate udev rules use by-id and
> assigned device names). In other word, this feature will be used just
> for "fixing" the names.

So admins are going to write their own udev rules by hand?  I find that
very unlikely.

> In our scenario, admin appends a new rule manually when a new LU is
> added. In the future, we are planning to enhance udev to append a new
> rule automatically as like as NIC.

I recommend doing the other options that have been proposed please.

thanks,

greg k-h