Skip to contentSkip to conversation

block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"

PATCH In review Tejun Heo 10 participants 13 messages Nov 9, 2011 – Nov 24, 2011

Subsystems (2)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

http://thread.gmane.org/gmane.linux.kernel/1155104
http://thread.gmane.org/gmane.linux.scsi/68632
http://thread.gmane.org/gmane.linux.scsi/69776

Cc Jens Axboe <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
Cc Nao Nishijima <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Alan Cox <[email protected]>
Cc Al Viro <[email protected]>
---
 Documentation/ABI/testing/sysfs-block |   13 ------
 block/genhd.c                         |   71 ---------------------------------
 include/linux/genhd.h                 |    4 --
 3 files changed, 0 insertions(+), 88 deletions(-)
Documentation/ABI/testing/sysfs-block +0 -13
index 2b5d561..c1eb41c 100644
@@ -206,16 +206,3 @@ Description:
 		when a discarded area is read the discard_zeroes_data
 		parameter will be set to one. Otherwise it will be 0 and
 		the result of reading a discarded area is undefined.
-What:		/sys/block/<disk>/alias
-Date:		Aug 2011
-Contact:	Nao Nishijima <[email protected]>
-Description:
-		A raw device name of a disk does not always point a same disk
-		each boot-up time. Therefore, users have to use persistent
-		device names, which udev creates when the kernel finds a disk,
-		instead of raw device name. However, kernel doesn't show those
-		persistent names on its messages (e.g. dmesg).
-		This file can store an alias of the disk and it would be
-		appeared in kernel messages if it is set. A disk can have an
-		alias which length is up to 255bytes. Users can use alphabets,
-		numbers, "-" and "_" in alias name. This file is writeonce.
block/genhd.c +0 -71

95 lines not on this page · raw message

include/linux/genhd.h +0 -4
index 9de31bc..6d18f35 100644
@@ -21,8 +21,6 @@
 #define dev_to_part(device)	container_of((device), struct hd_struct, __dev)
 #define disk_to_dev(disk)	(&(disk)->part0.__dev)
 #define part_to_dev(part)	(&((part)->__dev))
-#define alias_name(disk)	((disk)->alias ? (disk)->alias : \
-						 (disk)->disk_name)
 
 extern struct device_type part_type;
 extern struct kobject *block_depr;
@@ -60,7 +58,6 @@ enum {
 
 #define DISK_MAX_PARTS			256
 #define DISK_NAME_LEN			32
-#define ALIAS_LEN			256
 
 #include <linux/major.h>
 #include <linux/device.h>
@@ -166,7 +163,6 @@ struct gendisk {
                                          * disks that can't be partitioned. */
 
 	char disk_name[DISK_NAME_LEN];	/* name of major driver */
-	char *alias;			/* alias name of disk */
 	char *(*devnode)(struct gendisk *gd, mode_t *mode);
 
 	unsigned int events;		/* supported events */
Signature · 1 line
1.7.3.1
Acked-by on v1 1/1
#1 lore raw
Quoted 23 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

http://thread.gmane.org/gmane.linux.kernel/1155104
http://thread.gmane.org/gmane.linux.scsi/68632
http://thread.gmane.org/gmane.linux.scsi/69776

Quoted 6 lines
Cc Jens Axboe <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
Cc Nao Nishijima <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

Thanks for reverting this,

greg k-h

Acked-by
#2 lore raw
Quoted 24 lines · Greg KH
Quoted 23 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced.  Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed.  Revert it.

 http://thread.gmane.org/gmane.linux.kernel/1155104
 http://thread.gmane.org/gmane.linux.scsi/68632
 http://thread.gmane.org/gmane.linux.scsi/69776

Quoted 8 lines
Quoted 6 lines
Cc Jens Axboe <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
Cc Nao Nishijima <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

Thanks,
Kay

maintainer
#3 lore raw
In reply to Tejun Heo
Quoted 7 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It has the specific benefit that on snipped logs we don't get a mismatch between device name and actual device.

We already had this discussion at the kernel summit. The structured logging that might give us this facility in userspace isn't there yet (and may not be for a while), so while users cut and paste logs it's useful for the logs to show the device preferred name.

With just logs, we can't solve the cross reference problem, since the cross references appear at different points in the log files.

Quoted 8 lines

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core,

Which is why it went into gendisk rather than the driver core

Quoted 3 lines

block
layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

No, I can't agree with this ... if you propose a working alternative, I'm listening, but in the absence of one, I think the hack fills a gap we have in log analysis and tides us over until we have an agreed on proper solution (at which point, I'm perfectly happy to pull the hack back out).

James

#4 lore raw
Quoted 12 lines · James Bottomley
Quoted 7 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It has the specific benefit that on snipped logs we don't get a mismatch between device name and actual device.

Quoted 7 lines

We already had this discussion at the kernel summit. The structured logging that might give us this facility in userspace isn't there yet (and may not be for a while), so while users cut and paste logs it's useful for the logs to show the device preferred name.

With just logs, we can't solve the cross reference problem, since the cross references appear at different points in the log files.

It's not just a log issue, it's a userspace tool issue, which is not addressed here at all.

Actually I don't think this solves the log issue either, but that's a different point...

Quoted 15 lines
Quoted 8 lines

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core,

Which is why it went into gendisk rather than the driver core

Quoted 3 lines

block
layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

Quoted 5 lines

No, I can't agree with this ... if you propose a working alternative, I'm listening, but in the absence of one, I think the hack fills a gap we have in log analysis and tides us over until we have an agreed on proper solution (at which point, I'm perfectly happy to pull the hack back out).

Once it becomes a userspace api, it's almost impossible to ever remove, you know that.

And as you agree this is a hack, and it really doesn't provide the correct solution, why should it have been accepted?

Again, this is fixable in userspace, the author of the patch agrees with that, yet refuses to make the userspace changes despite having a few _years_ in which to so so.

Also, you should have gotten this through the block layer maintainer...

greg k-h

#5 lore raw
In reply to James Bottomley
Quoted 3 lines · James Bottomley

On Wed, 2011-11-09 at 08:25 -0800, Tejun Heo wrote:

No, I can't agree with this ... if you propose a working alternative,

The solution to this problem is to let udev log the known symlinks to the log stream at device discovery time. That way you can log _all_ kernel device messages to the currently know disk names. This works already even on old systems,

Quoted 1 line

I'm listening, but in the absence of one, I think the hack fills a gap

There is no gap.

Quoted 3 lines

we have in log analysis and tides us over until we have an agreed on proper solution (at which point, I'm perfectly happy to pull the hack back out).

Log analysis works just fine with the udev solution and provides the far better solution to this exact problem, so please revert it.

Such hacks are not supposed to get in, and its really hard to get them out again.

Thanks,
Kay

#6 lore raw
In reply to James Bottomley
Quoted 2 lines · James Bottomley
Quoted 1 line · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

[...]

Quoted 2 lines

No, I can't agree with this ... if you propose a working alternative, I'm listening, but in the absence of one, I think the hack fills a gap

"The hack fills a gap" -- the definitive guide to Operation System API designing by James Bottomley; ISBN-666-666.

;-)

If you open this Pandora's box then one day end-users will ask for the feature in all userspace utils. It's not about printk() only, it's about aliases for block devices. It's pretty invasive from long-term point of view...

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com
maintainer
#7 lore raw
In reply to Tejun Heo
Quoted 15 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

Quoted 7 lines

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

http://thread.gmane.org/gmane.linux.kernel/1155104
http://thread.gmane.org/gmane.linux.scsi/68632
http://thread.gmane.org/gmane.linux.scsi/69776

Ack on this, it seems both unneeded, unused, and a bad hack. We need to revert it before 3.2 rolls out, otherwise we are stuck with it.

Signature · 1 line
Jens Axboe
Acked-by on v1 1/1
#8 lore raw
In reply to Tejun Heo
Quoted 23 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

http://thread.gmane.org/gmane.linux.kernel/1155104
http://thread.gmane.org/gmane.linux.scsi/68632
http://thread.gmane.org/gmane.linux.scsi/69776

Quoted 8 lines
Cc Jens Axboe<[email protected]>
Cc "James E.J. Bottomley"<[email protected]>
Cc Nao Nishijima<[email protected]>
Cc Kay Sievers<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>
Cc Alan Cox<[email protected]>
Cc Al Viro<[email protected]>
#9 lore raw
In reply to Tejun Heo

Hi,

Finally, I understood why this patch is not acceptable and would like to solve the problem of the device name mismatch in *user space* using udev. So, could you please give me your comment about it?

I aim to unify the device name because the name shown in command output is different from it that users use for command execution.

So, I'd like to suggest that two new API should be added to udev.
- The API that returns any symlinks such as by-uuid, by id that users selected before if a device name is given as arguments
If the commands directly access to proc filesystems, they cannot get the symlinks. These commands should get the device name via this API.
- The API that replaces device name to symlink in text messages The dmesg and syslog messages include the raw device names such as sdX. They should be replaced with symlinks to unify the device names.
- udev support alias
Symlinks (by-uuid, by-id …) is too long, compared to before (e.g. sdX). I guess most of users need more friendly and short names.

Could you give me some advice or comments?
(I will post this idea udev mailing list with prototype code)

Thanks

Quoted 190 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

It is much worse than netif renames because it doesn't rename the actual device but just adds conveninence name which isn't used universally or enforced. Everything internal including device lookup and sysfs still uses the internal name and nothing prevents two devices from using conflicting alias - ie. sda can have sdb as its alias.

This has been nacked by people working on device driver core, block layer and kernel-userland interface and shouldn't have been upstreamed. Revert it.

http://thread.gmane.org/gmane.linux.kernel/1155104
http://thread.gmane.org/gmane.linux.scsi/68632
http://thread.gmane.org/gmane.linux.scsi/69776

Cc Jens Axboe <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
Cc Nao Nishijima <[email protected]>
Cc Kay Sievers <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Alan Cox <[email protected]>
Cc Al Viro <[email protected]>
---
 Documentation/ABI/testing/sysfs-block |   13 ------
 block/genhd.c                         |   71 ---------------------------------
 include/linux/genhd.h                 |    4 --
 3 files changed, 0 insertions(+), 88 deletions(-)
Documentation/ABI/testing/sysfs-block +0 -13
index 2b5d561..c1eb41c 100644
@@ -206,16 +206,3 @@ Description:
 		when a discarded area is read the discard_zeroes_data
 		parameter will be set to one. Otherwise it will be 0 and
 		the result of reading a discarded area is undefined.
-What:		/sys/block/<disk>/alias
-Date:		Aug 2011
-Contact:	Nao Nishijima <[email protected]>
-Description:
-		A raw device name of a disk does not always point a same disk
-		each boot-up time. Therefore, users have to use persistent
-		device names, which udev creates when the kernel finds a disk,
-		instead of raw device name. However, kernel doesn't show those
-		persistent names on its messages (e.g. dmesg).
-		This file can store an alias of the disk and it would be
-		appeared in kernel messages if it is set. A disk can have an
-		alias which length is up to 255bytes. Users can use alphabets,
-		numbers, "-" and "_" in alias name. This file is writeonce.
block/genhd.c +0 -71
index 9253839..02e9fca 100644
@@ -19,7 +19,6 @@
 #include <linux/mutex.h>
 #include <linux/idr.h>
 #include <linux/log2.h>
-#include <linux/ctype.h>
 
 #include "blk.h"
 
@@ -916,74 +915,6 @@ static int __init genhd_device_init(void)
 
 subsys_initcall(genhd_device_init);
 
-static ssize_t alias_show(struct device *dev,
-			       struct device_attribute *attr, char *buf)
-{
-	struct gendisk *disk = dev_to_disk(dev);
-	ssize_t ret = 0;
-
-	if (disk->alias)
-		ret = snprintf(buf, ALIAS_LEN, "%s\n", disk->alias);
-	return ret;
-}
-
-static ssize_t alias_store(struct device *dev, struct device_attribute *attr,
-			   const char *buf, size_t count)
-{
-	struct gendisk *disk = dev_to_disk(dev);
-	char *alias;
-	char *envp[] = { NULL, NULL };
-	unsigned char c;
-	int i;
-	ssize_t ret = count;
-
-	if (!count)
-		return -EINVAL;
-
-	if (count >= ALIAS_LEN) {
-		printk(KERN_ERR "alias: alias is too long\n");
-		return -EINVAL;
-	}
-
-	/* Validation check */
-	for (i = 0; i < count; i++) {
-		c = buf[i];
-		if (i == count - 1 && c == '\n')
-			break;
-		if (!isalnum(c) && c != '_' && c != '-') {
-			printk(KERN_ERR "alias: invalid alias\n");
-			return -EINVAL;
-		}
-	}
-
-	if (disk->alias) {
-		printk(KERN_INFO "alias: %s is already assigned (%s)\n",
-		       disk->disk_name, disk->alias);
-		return -EINVAL;
-	}
-
-	alias = kasprintf(GFP_KERNEL, "%s", buf);
-	if (!alias)
-		return -ENOMEM;
-
-	if (alias[count - 1] == '\n')
-		alias[count - 1] = '\0';
-
-	envp[0] = kasprintf(GFP_KERNEL, "ALIAS=%s", alias);
-	if (!envp[0]) {
-		kfree(alias);
-		return -ENOMEM;
-	}
-
-	disk->alias = alias;
-	printk(KERN_INFO "alias: assigned %s to %s\n", alias, disk->disk_name);
-
-	kobject_uevent_env(&dev->kobj, KOBJ_ADD, envp);
-
-	kfree(envp[0]);
-	return ret;
-}
-
 static ssize_t disk_range_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
@@ -1043,7 +974,6 @@ static ssize_t disk_discard_alignment_show(struct device *dev,
 	return sprintf(buf, "%d\n", queue_discard_alignment(disk->queue));
 }
 
-static DEVICE_ATTR(alias, S_IRUGO|S_IWUSR, alias_show, alias_store);
 static DEVICE_ATTR(range, S_IRUGO, disk_range_show, NULL);
 static DEVICE_ATTR(ext_range, S_IRUGO, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, S_IRUGO, disk_removable_show, NULL);
@@ -1066,7 +996,6 @@ static struct device_attribute dev_attr_fail_timeout =
 #endif
 
 static struct attribute *disk_attrs[] = {
-	&dev_attr_alias.attr,
 	&dev_attr_range.attr,
 	&dev_attr_ext_range.attr,
 	&dev_attr_removable.attr,
include/linux/genhd.h +0 -4
index 9de31bc..6d18f35 100644
@@ -21,8 +21,6 @@
 #define dev_to_part(device)	container_of((device), struct hd_struct, __dev)
 #define disk_to_dev(disk)	(&(disk)->part0.__dev)
 #define part_to_dev(part)	(&((part)->__dev))
-#define alias_name(disk)	((disk)->alias ? (disk)->alias : \
-						 (disk)->disk_name)
 
 extern struct device_type part_type;
 extern struct kobject *block_depr;
@@ -60,7 +58,6 @@ enum {
 
 #define DISK_MAX_PARTS			256
 #define DISK_NAME_LEN			32
-#define ALIAS_LEN			256
 
 #include <linux/major.h>
 #include <linux/device.h>
@@ -166,7 +163,6 @@ struct gendisk {
                                          * disks that can't be partitioned. */
 
 	char disk_name[DISK_NAME_LEN];	/* name of major driver */
-	char *alias;			/* alias name of disk */
 	char *(*devnode)(struct gendisk *gd, mode_t *mode);
 
 	unsigned int events;		/* supported events */
Signature · 8 lines
Nao NISHIJIMA
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., YOKOHAMA Research  Laboratory
Email: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#10 lore raw
Quoted 21 lines · Nao Nishijima

Hi,

Finally, I understood why this patch is not acceptable and would like to solve the problem of the device name mismatch in *user space* using udev. So, could you please give me your comment about it?

I aim to unify the device name because the name shown in command output is different from it that users use for command execution.

So, I'd like to suggest that two new API should be added to udev.
- The API that returns any symlinks such as by-uuid, by id that users selected before if a device name is given as arguments
If the commands directly access to proc filesystems, they cannot get the symlinks. These commands should get the device name via this API.
- The API that replaces device name to symlink in text messages The dmesg and syslog messages include the raw device names such as sdX. They should be replaced with symlinks to unify the device names.
- udev support alias
Symlinks (by-uuid, by-id …) is too long, compared to before (e.g. sdX). I guess most of users need more friendly and short names.

Quoted 2 lines

Could you give me some advice or comments?
(I will post this idea udev mailing list with prototype code)

Patches are always best to work off of, please post your changes to udev to the linux-hotplug list and we can take it from there.

thanks,

greg k-h

#11 lore raw
Quoted 22 lines · Greg KH
Quoted 21 lines · Nao Nishijima

Hi,

Finally, I understood why this patch is not acceptable and would like to solve the problem of the device name mismatch in *user space* using udev. So, could you please give me your comment about it?

I aim to unify the device name because the name shown in command output is different from it that users use for command execution.

So, I'd like to suggest that two new API should be added to udev.
- The API that returns any symlinks such as by-uuid, by id that users selected before if a device name is given as arguments
If the commands directly access to proc filesystems, they cannot get the symlinks. These commands should get the device name via this API.
- The API that replaces device name to symlink in text messages The dmesg and syslog messages include the raw device names such as sdX. They should be replaced with symlinks to unify the device names.
- udev support alias
Symlinks (by-uuid, by-id …) is too long, compared to before (e.g. sdX). I guess most of users need more friendly and short names.

Quoted 5 lines
Quoted 2 lines

Could you give me some advice or comments?
(I will post this idea udev mailing list with prototype code)

Patches are always best to work off of, please post your changes to udev to the linux-hotplug list and we can take it from there.

... and getting examples of acceptable code and APIs for that purpose might help the requester (and others) produce those patches.

Doug Gilbert

#12 lore raw
In reply to Tejun Heo
Quoted 8 lines · Tejun Heo

This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.

The commit introduced alias for block devices which is intended to be used during logging although actual usage hasn't been committed yet. This approach adds very limited benefit (raw log might be easier to follow) which can be trivially implemented in userland but has a lot of problems.

I have asked this before, but have been entirely ignored. So again, how do do you want to implement it in user space, if you are monitoring a system over a serial line, so all kernel messages go over that serial line to another system, without any local user space being involved?

Thanks,
Bernd