linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
@ 2011-11-09 16:25 Tejun Heo
  2011-11-09 16:53 ` Greg KH
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Tejun Heo @ 2011-11-09 16:25 UTC (permalink / raw)
  To: Jens Axboe, James E.J. Bottomley, Nao Nishijima, Kay Sievers,
	Greg Kroah-Hartman, Alan Cox, Al Viro
  Cc: linux-kernel, linux-scsi

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

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Nao Nishijima <nao.nishijima.xt@hitachi.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 Documentation/ABI/testing/sysfs-block |   13 ------
 block/genhd.c                         |   71 ---------------------------------
 include/linux/genhd.h                 |    4 --
 3 files changed, 0 insertions(+), 88 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
index 2b5d561..c1eb41c 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -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 <nao.nishijima.xt@hitachi.com>
-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.
diff --git a/block/genhd.c b/block/genhd.c
index 9253839..02e9fca 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -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,
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 9de31bc..6d18f35 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -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 */
-- 
1.7.3.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
@ 2011-11-09 16:53 ` Greg KH
  2011-11-09 17:19   ` Kay Sievers
  2011-11-09 17:30 ` James Bottomley
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2011-11-09 16:53 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jens Axboe, James E.J. Bottomley, Nao Nishijima, Kay Sievers,
	Alan Cox, Al Viro, linux-kernel, linux-scsi

On Wed, Nov 09, 2011 at 08:25:12AM -0800, Tejun Heo wrote:
> 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
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Nao Nishijima <nao.nishijima.xt@hitachi.com>
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Thanks for reverting this,

greg k-h


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:53 ` Greg KH
@ 2011-11-09 17:19   ` Kay Sievers
  0 siblings, 0 replies; 13+ messages in thread
From: Kay Sievers @ 2011-11-09 17:19 UTC (permalink / raw)
  To: Greg KH
  Cc: Tejun Heo, Jens Axboe, James E.J. Bottomley, Nao Nishijima,
	Alan Cox, Al Viro, linux-kernel, linux-scsi

On Wed, Nov 9, 2011 at 17:53, Greg KH <gregkh@suse.de> wrote:
> On Wed, Nov 09, 2011 at 08:25:12AM -0800, Tejun Heo wrote:
>> 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
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
>> Cc: Nao Nishijima <nao.nishijima.xt@hitachi.com>
>> Cc: Kay Sievers <kay.sievers@vrfy.org>
>> Cc: Greg Kroah-Hartman <gregkh@suse.de>
>
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

 Acked-by: Kay Sievers <kay.sievers@vrfy.org>

Thanks,
Kay

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
  2011-11-09 16:53 ` Greg KH
@ 2011-11-09 17:30 ` James Bottomley
  2011-11-09 17:36   ` Greg KH
                     ` (2 more replies)
  2011-11-09 18:54 ` Jens Axboe
                   ` (3 subsequent siblings)
  5 siblings, 3 replies; 13+ messages in thread
From: James Bottomley @ 2011-11-09 17:30 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jens Axboe, Nao Nishijima, Kay Sievers, Greg Kroah-Hartman,
	Alan Cox, Al Viro, linux-kernel, linux-scsi

On Wed, 2011-11-09 at 08:25 -0800, Tejun Heo wrote:
> 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.

> 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

> 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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 17:30 ` James Bottomley
@ 2011-11-09 17:36   ` Greg KH
  2011-11-09 17:38   ` Kay Sievers
  2011-11-09 19:51   ` Karel Zak
  2 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2011-11-09 17:36 UTC (permalink / raw)
  To: James Bottomley
  Cc: Tejun Heo, Jens Axboe, Nao Nishijima, Kay Sievers, Alan Cox,
	Al Viro, linux-kernel, linux-scsi

On Wed, Nov 09, 2011 at 11:30:00AM -0600, James Bottomley wrote:
> On Wed, 2011-11-09 at 08:25 -0800, Tejun Heo wrote:
> > 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.

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...

> > 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
> 
> > 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).

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 17:30 ` James Bottomley
  2011-11-09 17:36   ` Greg KH
@ 2011-11-09 17:38   ` Kay Sievers
  2011-11-09 19:51   ` Karel Zak
  2 siblings, 0 replies; 13+ messages in thread
From: Kay Sievers @ 2011-11-09 17:38 UTC (permalink / raw)
  To: James Bottomley
  Cc: Tejun Heo, Jens Axboe, Nao Nishijima, Greg Kroah-Hartman,
	Alan Cox, Al Viro, linux-kernel, linux-scsi

On Wed, Nov 9, 2011 at 18:30, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> 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,

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

There is no 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).

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
  2011-11-09 16:53 ` Greg KH
  2011-11-09 17:30 ` James Bottomley
@ 2011-11-09 18:54 ` Jens Axboe
  2011-11-10 21:21 ` Jeff Garzik
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2011-11-09 18:54 UTC (permalink / raw)
  To: Tejun Heo
  Cc: James E.J. Bottomley, Nao Nishijima, Kay Sievers,
	Greg Kroah-Hartman, Alan Cox, Al Viro, linux-kernel, linux-scsi

On 2011-11-09 17:25, Tejun Heo wrote:
> 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

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.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 17:30 ` James Bottomley
  2011-11-09 17:36   ` Greg KH
  2011-11-09 17:38   ` Kay Sievers
@ 2011-11-09 19:51   ` Karel Zak
  2 siblings, 0 replies; 13+ messages in thread
From: Karel Zak @ 2011-11-09 19:51 UTC (permalink / raw)
  To: James Bottomley
  Cc: Tejun Heo, Jens Axboe, Nao Nishijima, Kay Sievers,
	Greg Kroah-Hartman, Alan Cox, Al Viro, linux-kernel, linux-scsi

On Wed, Nov 09, 2011 at 11:30:00AM -0600, James Bottomley wrote:
> On Wed, 2011-11-09 at 08:25 -0800, Tejun Heo wrote:
> > This reverts commit a72c5e5eb738033938ab30d6a634b74d1d060f10.
[...]
> 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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
                   ` (2 preceding siblings ...)
  2011-11-09 18:54 ` Jens Axboe
@ 2011-11-10 21:21 ` Jeff Garzik
  2011-11-11 14:50 ` Nao Nishijima
  2011-11-24 12:21 ` Bernd Schubert
  5 siblings, 0 replies; 13+ messages in thread
From: Jeff Garzik @ 2011-11-10 21:21 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jens Axboe, James E.J. Bottomley, Nao Nishijima, Kay Sievers,
	Greg Kroah-Hartman, Alan Cox, Al Viro, linux-kernel, linux-scsi

On 11/09/2011 11:25 AM, Tejun Heo wrote:
> 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
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Jens Axboe<axboe@kernel.dk>
> Cc: "James E.J. Bottomley"<James.Bottomley@HansenPartnership.com>
> Cc: Nao Nishijima<nao.nishijima.xt@hitachi.com>
> Cc: Kay Sievers<kay.sievers@vrfy.org>
> Cc: Greg Kroah-Hartman<gregkh@suse.de>
> Cc: Alan Cox<alan@linux.intel.com>
> Cc: Al Viro<viro@zeniv.linux.org.uk>

Acked-by: Jeff Garzik <jgarzik@redhat.com>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
                   ` (3 preceding siblings ...)
  2011-11-10 21:21 ` Jeff Garzik
@ 2011-11-11 14:50 ` Nao Nishijima
  2011-11-11 17:36   ` Greg KH
  2011-11-24 12:21 ` Bernd Schubert
  5 siblings, 1 reply; 13+ messages in thread
From: Nao Nishijima @ 2011-11-11 14:50 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jens Axboe, James E.J. Bottomley, Kay Sievers,
	Greg Kroah-Hartman, Alan Cox, Al Viro, linux-kernel, linux-scsi

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

(2011/11/10 1:25), Tejun Heo wrote:
> 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
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Nao Nishijima <nao.nishijima.xt@hitachi.com>
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  Documentation/ABI/testing/sysfs-block |   13 ------
>  block/genhd.c                         |   71 ---------------------------------
>  include/linux/genhd.h                 |    4 --
>  3 files changed, 0 insertions(+), 88 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
> index 2b5d561..c1eb41c 100644
> --- a/Documentation/ABI/testing/sysfs-block
> +++ b/Documentation/ABI/testing/sysfs-block
> @@ -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 <nao.nishijima.xt@hitachi.com>
> -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.
> diff --git a/block/genhd.c b/block/genhd.c
> index 9253839..02e9fca 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -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,
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 9de31bc..6d18f35 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -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 */


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-11 14:50 ` Nao Nishijima
@ 2011-11-11 17:36   ` Greg KH
  2011-11-11 19:09     ` Douglas Gilbert
  0 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2011-11-11 17:36 UTC (permalink / raw)
  To: Nao Nishijima
  Cc: Tejun Heo, Jens Axboe, James E.J. Bottomley, Kay Sievers,
	Alan Cox, Al Viro, linux-kernel, linux-scsi

On Fri, Nov 11, 2011 at 11:50:06PM +0900, Nao Nishijima wrote:
> 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)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-11 17:36   ` Greg KH
@ 2011-11-11 19:09     ` Douglas Gilbert
  0 siblings, 0 replies; 13+ messages in thread
From: Douglas Gilbert @ 2011-11-11 19:09 UTC (permalink / raw)
  To: Greg KH
  Cc: Nao Nishijima, Tejun Heo, Jens Axboe, James E.J. Bottomley,
	Kay Sievers, Alan Cox, Al Viro, linux-kernel, linux-scsi

On 11-11-11 12:36 PM, Greg KH wrote:
> On Fri, Nov 11, 2011 at 11:50:06PM +0900, Nao Nishijima wrote:
>> 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)
>
> 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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk"
  2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
                   ` (4 preceding siblings ...)
  2011-11-11 14:50 ` Nao Nishijima
@ 2011-11-24 12:21 ` Bernd Schubert
  5 siblings, 0 replies; 13+ messages in thread
From: Bernd Schubert @ 2011-11-24 12:21 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux-scsi, linux-kernel

On 11/09/2011 05:25 PM, Tejun Heo wrote:
> 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-11-24 12:22 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-09 16:25 [PATCH] block: Revert "[SCSI] genhd: add a new attribute "alias" in gendisk" Tejun Heo
2011-11-09 16:53 ` Greg KH
2011-11-09 17:19   ` Kay Sievers
2011-11-09 17:30 ` James Bottomley
2011-11-09 17:36   ` Greg KH
2011-11-09 17:38   ` Kay Sievers
2011-11-09 19:51   ` Karel Zak
2011-11-09 18:54 ` Jens Axboe
2011-11-10 21:21 ` Jeff Garzik
2011-11-11 14:50 ` Nao Nishijima
2011-11-11 17:36   ` Greg KH
2011-11-11 19:09     ` Douglas Gilbert
2011-11-24 12:21 ` Bernd Schubert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).