Skip to contentSkip to conversation

aoe [1/12]: remove too-low cap on minor number

PATCH New Greg KH 7 participants 30 messages Mar 17, 2005 – Apr 8, 2005 1 message not in the archive

Subsystems (5)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Greg KH

author
#1 lore raw

I've applied 11 of these 12 patches (the one from Randy was already included) to my trees.

thanks,

greg k-h

#2 lore raw
Quoted 2 lines · Greg KH

I've applied 11 of these 12 patches (the one from Randy was already included) to my trees.

Those haven't gone to the lkml yet. I'll post them through gmane. (The original postings never made it to the list, and I can't get in touch with the lkml admins, but gmane should work.)

Signature · 1 line
  Ed L Cashin <[email protected]>
#3 lore raw

PATCH 1/1 aoe [4/12]: handle distros that have a udev rules file instead of dir

In reply to Greg KH

handle distros that have a udev rules file instead of dir

Documentation/aoe/udev-install.sh +5 -1
diff -uprN a/Documentation/aoe/udev-install.sh b/Documentation/aoe/udev-install.sh
@@ -23,4 +23,8 @@ fi
 # /etc/udev/rules.d
 #
 rules_d="`sed -n '/^udev_rules=/{ s!udev_rules=!!; s!\"!!g; p; }' $conf`"
-test "$rules_d" && sh -xc "cp `dirname $0`/udev.txt $rules_d/60-aoe.rules"
+if test -z "$rules_d" || test ! -d "$rules_d"; then
+	echo "$me Error: cannot find udev rules directory" 1>&2
+	exit 1
+fi
+sh -xc "cp `dirname $0`/udev.txt $rules_d/60-aoe.rules"
Signature · 1 line
  Ed L. Cashin <[email protected]>
#4 lore raw

PATCH 1/1 aoe [1/12]: remove too-low cap on minor number

In reply to Greg KH

remove too-low cap on minor number

drivers/block/aoe/aoe.h +5 -0
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -2,9 +2,14 @@
 #define VERSION "5"
 #define AOE_MAJOR 152
 #define DEVICE_NAME "aoe"
+
+/* set AOE_PARTITIONS to 1 to use whole-disks only
+ * default is 16, which is 15 partitions plus the whole disk
+ */
 #ifndef AOE_PARTITIONS
 #define AOE_PARTITIONS 16
 #endif
+
 #define SYSMINOR(aoemajor, aoeminor) ((aoemajor) * 10 + (aoeminor))
 #define AOEMAJOR(sysminor) ((sysminor) / 10)
 #define AOEMINOR(sysminor) ((sysminor) % 10)
drivers/block/aoe/aoecmd.c +5 -4
diff -uprN a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
@@ -577,7 +577,7 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
 	struct aoe_cfghdr *ch;
 	ulong flags, bufcnt, sysminor, aoemajor;
 	struct sk_buff *sl;
-	enum { MAXFRAMES = 8, MAXSYSMINOR = 255 };
+	enum { MAXFRAMES = 8 };
 
 	h = (struct aoe_hdr *) skb->mac.raw;
 	ch = (struct aoe_cfghdr *) (h+1);
@@ -594,9 +594,10 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
 	}
 
 	sysminor = SYSMINOR(aoemajor, h->minor);
-	if (sysminor > MAXSYSMINOR) {
-		printk(KERN_INFO "aoe: aoecmd_cfg_rsp: sysminor %ld too "
-			"large\n", sysminor);
+	if (sysminor * AOE_PARTITIONS + AOE_PARTITIONS > MINORMASK) {
+		printk(KERN_INFO
+			"aoe: e%ld.%d: minor number too large\n", 
+			aoemajor, (int) h->minor);
 		return;
 	}
 
Signature · 1 line
  Ed L. Cashin <[email protected]>
#5 lore raw

PATCH 1/1 aoe [2/12]: allow multiple aoe devices with same MAC addr

In reply to Greg KH

allow multiple aoe devices with same MAC addr

drivers/block/aoe/aoe.h +1 -1
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -156,7 +156,7 @@ void aoecmd_cfg_rsp(struct sk_buff *);
 
 int aoedev_init(void);
 void aoedev_exit(void);
-struct aoedev *aoedev_bymac(unsigned char *);
+struct aoedev *aoedev_by_aoeaddr(int maj, int min);
 void aoedev_downdev(struct aoedev *d);
 struct aoedev *aoedev_set(ulong, unsigned char *, struct net_device *, ulong);
 int aoedev_busy(void);
drivers/block/aoe/aoecmd.c +5 -4
diff -uprN a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
@@ -380,14 +380,15 @@ aoecmd_ata_rsp(struct sk_buff *skb)
 	register long n;
 	ulong flags;
 	char ebuf[128];
-	
+	u16 aoemajor;
+
 	hin = (struct aoe_hdr *) skb->mac.raw;
-	d = aoedev_bymac(hin->src);
+	aoemajor = __be16_to_cpu(*((u16 *) hin->major));
+	d = aoedev_by_aoeaddr(aoemajor, hin->minor);
 	if (d == NULL) {
 		snprintf(ebuf, sizeof ebuf, "aoecmd_ata_rsp: ata response "
 			"for unknown device %d.%d\n",
-			 __be16_to_cpu(*((u16 *) hin->major)),
-			hin->minor);
+			 aoemajor, hin->minor);
 		aoechr_error(ebuf);
 		return;
 	}
drivers/block/aoe/aoedev.c +2 -3
diff -uprN a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
@@ -13,7 +13,7 @@ static struct aoedev *devlist;
 static spinlock_t devlist_lock;
 
 struct aoedev *
-aoedev_bymac(unsigned char *macaddr)
+aoedev_by_aoeaddr(int maj, int min)
 {
 	struct aoedev *d;
 	ulong flags;
@@ -21,7 +21,7 @@ aoedev_bymac(unsigned char *macaddr)
 	spin_lock_irqsave(&devlist_lock, flags);
 
 	for (d=devlist; d; d=d->next)
-		if (!memcmp(d->addr, macaddr, 6))
+		if (d->aoemajor == maj && d->aoeminor == min)
 			break;
 
 	spin_unlock_irqrestore(&devlist_lock, flags);
@@ -125,7 +125,6 @@ aoedev_set(ulong sysminor, unsigned char
 	d->ifp = ifp;
 
 	if (d->sysminor != sysminor
-	|| memcmp(d->addr, addr, sizeof d->addr)
 	|| (d->flags & DEVFL_UP) == 0) {
 		aoedev_downdev(d); /* flushes outstanding frames */
 		memcpy(d->addr, addr, sizeof d->addr);
Signature · 1 line
  Ed L. Cashin <[email protected]>
#6 lore raw

PATCH 1/1 aoe [3/12]: update driver version to 6

In reply to Greg KH

update driver version to 6

drivers/block/aoe/aoe.h +1 -1
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -1,5 +1,5 @@
 /* Copyright (c) 2004 Coraid, Inc.  See COPYING for GPL terms. */
-#define VERSION "5"
+#define VERSION "6"
 #define AOE_MAJOR 152
 #define DEVICE_NAME "aoe"
 
Signature · 1 line
  Ed L. Cashin <[email protected]>
#7 lore raw

PATCH 1/1 aoe [4/12]: handle distros that have a udev rules file instead of dir

In reply to Greg KH

handle distros that have a udev rules file instead of dir

Documentation/aoe/udev-install.sh +5 -1
diff -uprN a/Documentation/aoe/udev-install.sh b/Documentation/aoe/udev-install.sh
@@ -23,4 +23,8 @@ fi
 # /etc/udev/rules.d
 #
 rules_d="`sed -n '/^udev_rules=/{ s!udev_rules=!!; s!\"!!g; p; }' $conf`"
-test "$rules_d" && sh -xc "cp `dirname $0`/udev.txt $rules_d/60-aoe.rules"
+if test -z "$rules_d" || test ! -d "$rules_d"; then
+	echo "$me Error: cannot find udev rules directory" 1>&2
+	exit 1
+fi
+sh -xc "cp `dirname $0`/udev.txt $rules_d/60-aoe.rules"
Signature · 1 line
  Ed L. Cashin <[email protected]>
#8 lore raw

PATCH 1/1 aoe [6/12]: Alexey Dobriyan sparse cleanup

In reply to Greg KH

Alexey Dobriyan sparse cleanup

drivers/block/aoe/aoe.h +5 -5
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -39,13 +39,13 @@ enum {
 struct aoe_hdr {
 	unsigned char dst[6];
 	unsigned char src[6];
-	unsigned char type[2];
+	__be16 type;
 	unsigned char verfl;
 	unsigned char err;
-	unsigned char major[2];
+	__be16 major;
 	unsigned char minor;
 	unsigned char cmd;
-	unsigned char tag[4];
+	__be32 tag;
 };
 
 struct aoe_atahdr {
@@ -63,8 +63,8 @@ struct aoe_atahdr {
 };
 
 struct aoe_cfghdr {
-	unsigned char bufcnt[2];
-	unsigned char fwver[2];
+	__be16 bufcnt;
+	__be16 fwver;
 	unsigned char res;
 	unsigned char aoeccmd;
 	unsigned char cslen[2];
drivers/block/aoe/aoecmd.c +23 -29

159 lines not on this page · raw message

drivers/block/aoe/aoenet.c +4 -4
diff -uprN a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
@@ -69,7 +69,7 @@ set_aoe_iflist(const char __user *user_s
 u64
 mac_addr(char addr[6])
 {
-	u64 n = 0;
+	__be64 n = 0;
 	char *p = (char *) &n;
 
 	memcpy(p + 2, addr, 6);	/* (sizeof addr != 6) */
@@ -108,7 +108,7 @@ static int
 aoenet_rcv(struct sk_buff *skb, struct net_device *ifp, struct packet_type *pt)
 {
 	struct aoe_hdr *h;
-	ulong n;
+	u32 n;
 
 	skb = skb_check(skb);
 	if (!skb)
@@ -121,7 +121,7 @@ aoenet_rcv(struct sk_buff *skb, struct n
 	skb_push(skb, ETH_HLEN);	/* (1) */
 
 	h = (struct aoe_hdr *) skb->mac.raw;
-	n = __be32_to_cpu(*((u32 *) h->tag));
+	n = be32_to_cpu(h->tag);
 	if ((h->verfl & AOEFL_RSP) == 0 || (n & 1<<31))
 		goto exit;
 
@@ -132,7 +132,7 @@ aoenet_rcv(struct sk_buff *skb, struct n
 		if (net_ratelimit())
 			printk(KERN_ERR "aoe: aoenet_rcv: error packet from %d.%d; "
 			       "ecode=%d '%s'\n",
-			       __be16_to_cpu(*((u16 *) h->major)), h->minor, 
+			       be16_to_cpu(h->major), h->minor, 
 			       h->err, aoe_errlist[n]);
 		goto exit;
 	}
Signature · 1 line
  Ed L. Cashin <[email protected]>
#9 lore raw

PATCH 1/1 aoe [5/12]: don't try to free null bufpool

In reply to Greg KH

don't try to free null bufpool

drivers/block/aoe/aoedev.c +2 -1
diff -uprN a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
@@ -146,7 +146,8 @@ aoedev_freedev(struct aoedev *d)
 		put_disk(d->gd);
 	}
 	kfree(d->frames);
-	mempool_destroy(d->bufpool);
+	if (d->bufpool)
+		mempool_destroy(d->bufpool);
 	kfree(d);
 }
 
Signature · 1 line
  Ed L. Cashin <[email protected]>
#10 lore raw

Re: [PATCH 2.6.11] aoe [5/12]: don't try to free null bufpool

Quoted 1 line · On Thu, 2005-03-24 at 07:17 -0800, [email protected] wrote:

don't try to free null bufpool

in linux there is a "rule" that all memory free routines are supposed to also accept NULL as argument, so I think this patch is not needed (and even wrong)

#11 lore raw

Re: [PATCH 2.6.11] aoe [5/12]: don't try to free null bufpool

Quoted 7 lines · Arjan van de Ven
Quoted 1 line · On Thu, 2005-03-24 at 07:17 -0800, [email protected] wrote:

don't try to free null bufpool

in linux there is a "rule" that all memory free routines are supposed to also accept NULL as argument, so I think this patch is not needed (and even wrong)

Hmm. The mm/mempool.c:mempool_destroy function immediately dereferences the pointer passed to it:

void mempool_destroy(mempool_t *pool)

{
	if (pool->curr_nr != pool->min_nr)
		BUG();		/* There were outstanding elements */
	free_pool(pool);
}

... so I'm not sure mempool_destroy fits the rule. Are you suggesting that the patch should instead modify mempool_destroy?

Signature · 1 line
  Ed L Cashin <[email protected]>
#12 lore raw

Re: [PATCH 2.6.11] aoe [5/12]: don't try to free null bufpool

Quoted 14 lines · On Thu, 2005-03-24 at 09:04 -0800, [email protected] wrote:
Quoted 7 lines · Arjan van de Ven
Quoted 1 line · On Thu, 2005-03-24 at 07:17 -0800, [email protected] wrote:

don't try to free null bufpool

in linux there is a "rule" that all memory free routines are supposed to also accept NULL as argument, so I think this patch is not needed (and even wrong)

Hmm. The mm/mempool.c:mempool_destroy function immediately dereferences the pointer passed to it:

void mempool_destroy(mempool_t *pool)

Quoted 8 lines
{
	if (pool->curr_nr != pool->min_nr)
		BUG();		/* There were outstanding elements */
	free_pool(pool);
}

... so I'm not sure mempool_destroy fits the rule. Are you suggesting that the patch should instead modify mempool_destroy?

hmm perhaps... Jens?

#13 lore raw

Re: [PATCH 2.6.11] aoe [5/12]: don't try to free null bufpool

Quoted 21 lines · Arjan van de Ven
Quoted 20 lines · On Thu, 2005-03-24 at 09:04 -0800, [email protected] wrote:
Quoted 7 lines · Arjan van de Ven
Quoted 1 line · On Thu, 2005-03-24 at 07:17 -0800, [email protected] wrote:

don't try to free null bufpool

in linux there is a "rule" that all memory free routines are supposed to also accept NULL as argument, so I think this patch is not needed (and even wrong)

Hmm. The mm/mempool.c:mempool_destroy function immediately dereferences the pointer passed to it:

void mempool_destroy(mempool_t *pool)

{
	if (pool->curr_nr != pool->min_nr)
		BUG();		/* There were outstanding elements */
	free_pool(pool);
}
Quoted 5 lines
Quoted 2 lines

... so I'm not sure mempool_destroy fits the rule. Are you suggesting that the patch should instead modify mempool_destroy?

hmm perhaps... Jens?

Having mempool_destroy() be the one that checks seems safer, then callers won't forget to check - easier to just check in one place.
If that's what you want, then here's a patch. If this is acceptable I can create another one that removes the (then pointless) NULL checks from all callers - let me know if that's wanted.

mm/mempool.c +2 -0
@@ -176,6 +176,8 @@ EXPORT_SYMBOL(mempool_resize);
  */
 void mempool_destroy(mempool_t *pool)
 {
+	if (!pool)
+		return;
 	if (pool->curr_nr != pool->min_nr)
 		BUG();		/* There were outstanding elements */
 	free_pool(pool);
#14 lore raw

Re: [PATCH 2.6.11] aoe [5/12]: don't try to free null bufpool

In reply to Arjan van de Ven
Quoted 21 lines · Arjan van de Ven
Quoted 20 lines · On Thu, 2005-03-24 at 09:04 -0800, [email protected] wrote:
Quoted 7 lines · Arjan van de Ven
Quoted 1 line · On Thu, 2005-03-24 at 07:17 -0800, [email protected] wrote:

don't try to free null bufpool

in linux there is a "rule" that all memory free routines are supposed to also accept NULL as argument, so I think this patch is not needed (and even wrong)

Hmm. The mm/mempool.c:mempool_destroy function immediately dereferences the pointer passed to it:

void mempool_destroy(mempool_t *pool)

{
	if (pool->curr_nr != pool->min_nr)
		BUG();		/* There were outstanding elements */
	free_pool(pool);
}
Quoted 4 lines
Quoted 2 lines

... so I'm not sure mempool_destroy fits the rule. Are you suggesting that the patch should instead modify mempool_destroy?

hmm perhaps... Jens?

Not really my call, but I agree we should make mempool_destroy() resilient against !pool to follow the path of least surprise.

Signature · 1 line
Jens Axboe
#15 lore raw

PATCH 1/1 aoe [8/12]: document env var for specifying number of partitions per dev

In reply to Greg KH

document env var for specifying number of partitions per dev

Documentation/aoe/mkdevs.sh +1 -0
diff -uprN a/Documentation/aoe/mkdevs.sh b/Documentation/aoe/mkdevs.sh
@@ -5,6 +5,7 @@ n_partitions=${n_partitions:-16}
 
 if test "$#" != "1"; then
 	echo "Usage: sh `basename $0` {dir}" 1>&2
+	echo "       n_partitions=16 sh `basename $0` {dir}" 1>&2
 	exit 1
 fi
 dir=$1
Documentation/aoe/mkshelf.sh +1 -0
diff -uprN a/Documentation/aoe/mkshelf.sh b/Documentation/aoe/mkshelf.sh
@@ -2,6 +2,7 @@
 
 if test "$#" != "2"; then
 	echo "Usage: sh `basename $0` {dir} {shelfaddress}" 1>&2
+	echo "       n_partitions=16 sh `basename $0` {dir} {shelfaddress}" 1>&2
 	exit 1
 fi
 n_partitions=${n_partitions:-16}
Signature · 1 line
  Ed L. Cashin <[email protected]>
#16 lore raw

PATCH 1/1 aoe [9/12]: add note about the need for deadlock-free sk_buff allocation

In reply to Greg KH

add note about the need for deadlock-free sk_buff allocation

Documentation/aoe/todo.txt +14 -0
diff -uprN a/Documentation/aoe/todo.txt b/Documentation/aoe/todo.txt
@@ -0,0 +1,14 @@
+There is a potential for deadlock when allocating a struct sk_buff for
+data that needs to be written out to aoe storage.  If the data is
+being written from a dirty page in order to free that page, and if
+there are no other pages available, then deadlock may occur when a
+free page is needed for the sk_buff allocation.  This situation has
+not been observed, but it would be nice to eliminate any potential for
+deadlock under memory pressure.
+
+Because ATA over Ethernet is not fragmented by the kernel's IP code,
+the destructore member of the struct sk_buff is available to the aoe
+driver.  By using a mempool for allocating all but the first few
+sk_buffs, and by registering a destructor, we should be able to
+efficiently allocate sk_buffs without introducing any potential for
+deadlock.
Signature · 1 line
  Ed L. Cashin <[email protected]>
#17 lore raw

PATCH 1/1 aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

In reply to Greg KH

support configuration of AOE_PARTITIONS from Kconfig

drivers/block/Kconfig +15 -0
diff -uprN a/drivers/block/Kconfig b/drivers/block/Kconfig
@@ -506,4 +506,19 @@ config ATA_OVER_ETH
 	This driver provides Support for ATA over Ethernet block
 	devices like the Coraid EtherDrive (R) Storage Blade.
 
+config AOE_PARTITIONS
+	int "Partitions per AoE device" if ATA_OVER_ETH
+	default "16"
+	help
+	  The default is to support 16 partitions per aoe device. Some
+	  systems lack good support for devices with large minor
+	  numbers.
+
+	  Such systems will be able to use more aoe disks when
+	  AOE_PARTITIONS is set to one, but you won't be able to
+	  partition the disks, and you must make sure your device
+	  nodes are created to work with the value you select.
+
+	  If unsure, use 16.
+
 endmenu
drivers/block/aoe/aoe.h +3 -2
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -6,8 +6,9 @@
 /* set AOE_PARTITIONS to 1 to use whole-disks only
  * default is 16, which is 15 partitions plus the whole disk
  */
-#ifndef AOE_PARTITIONS
-#define AOE_PARTITIONS 16
+#define AOE_PARTITIONS CONFIG_AOE_PARTITIONS
+#if AOE_PARTITIONS < 1
+#error AOE_PARTITIONS less than one
 #endif
 
 #define SYSMINOR(aoemajor, aoeminor) ((aoemajor) * 10 + (aoeminor))
Signature · 1 line
  Ed L. Cashin <[email protected]>
#18 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 17 lines · On Thu, Mar 24, 2005 at 07:21:28AM -0800, [email protected] wrote:

support configuration of AOE_PARTITIONS from Kconfig

diff -uprN a/drivers/block/Kconfig b/drivers/block/Kconfig

drivers/block/Kconfig +5 -0
@@ -506,4 +506,19 @@ config ATA_OVER_ETH
 	This driver provides Support for ATA over Ethernet block
 	devices like the Coraid EtherDrive (R) Storage Blade.
 
+config AOE_PARTITIONS
+	int "Partitions per AoE device" if ATA_OVER_ETH
+	default "16"
+	help
+	  The default is to support 16 partitions per aoe device. Some

drivers/block/Kconfig config ATA_OVER_ETH · line 523

Quoted 10 lines
drivers/block/Kconfig +10 -0
+	  systems lack good support for devices with large minor
+	  numbers.
+
+	  Such systems will be able to use more aoe disks when
+	  AOE_PARTITIONS is set to one, but you won't be able to
+	  partition the disks, and you must make sure your device
+	  nodes are created to work with the value you select.
+
+	  If unsure, use 16.
+

NACK. this changes devices nodes based on a compile-time option. Just tell people to update their userland to a 2.6-copatible version.

#19 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 22 lines · Christoph Hellwig
Quoted 21 lines · On Thu, Mar 24, 2005 at 07:21:28AM -0800, [email protected] wrote:

support configuration of AOE_PARTITIONS from Kconfig

diff -uprN a/drivers/block/Kconfig b/drivers/block/Kconfig

drivers/block/Kconfig +9 -0
@@ -506,4 +506,19 @@ config ATA_OVER_ETH
 	This driver provides Support for ATA over Ethernet block
 	devices like the Coraid EtherDrive (R) Storage Blade.
 
+config AOE_PARTITIONS
+	int "Partitions per AoE device" if ATA_OVER_ETH
+	default "16"
+	help
+	  The default is to support 16 partitions per aoe device. Some
+	  systems lack good support for devices with large minor
+	  numbers.
+
+	  Such systems will be able to use more aoe disks when
Quoted 8 lines

drivers/block/Kconfig config ATA_OVER_ETH · line 523

Quoted 6 lines
drivers/block/Kconfig +6 -0
+	  AOE_PARTITIONS is set to one, but you won't be able to
+	  partition the disks, and you must make sure your device
+	  nodes are created to work with the value you select.
+
+	  If unsure, use 16.
+

NACK. this changes devices nodes based on a compile-time option.

I'm not sure I follow. This configuration option sets the number of partitions per device in the driver. It doesn't create device nodes.

If the user has udev, then the device nodes are created correctly (on Fedora Core 3), so that if the driver is configured with 1 partition per device, the minor numbers for the disks are low.

The folks I've talked to who aren't using udev but want one partition per device already know that they have to re-create their device files.

Quoted 2 lines

Just tell people to update their userland to a 2.6-copatible version.

Even if the glibc, coreutils, etc., get it right, some programs try to parse the device node bits themselves and fail to find all the minor number bits.

Making this configurable makes it possible for a debian sarge user or a Slackware 10 user to run a 2.6.11 kernel and use up to 256 disks. Even a Fedora Core 3 user has an mdadm that balks at minor numbers like 1120. By using one partition per device, I can use FC3's mdadm and have everything work.

The AoE users started doing this themselves, but this configuration option allows non-C-programmers to do the same. It's helpful during the transitional period and should be removed when the userland software that people are actually running has caught up.

Signature · 1 line
  Ed L Cashin <[email protected]>
#20 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 6 lines · Ed L Cashin
Quoted 2 lines

NACK. this changes devices nodes based on a compile-time option.

I'm not sure I follow. This configuration option sets the number of partitions per device in the driver. It doesn't create device nodes.

Quoted 7 lines

If the user has udev, then the device nodes are created correctly (on Fedora Core 3), so that if the driver is configured with 1 partition per device, the minor numbers for the disks are low.

The folks I've talked to who aren't using udev but want one partition per device already know that they have to re-create their device files.

It changes a kernel ABI, so people that have different config options set can't use the same userland. It's a really big no-go.

#21 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 11 lines · Christoph Hellwig
Quoted 10 lines · Ed L Cashin
Quoted 2 lines

NACK. this changes devices nodes based on a compile-time option.

I'm not sure I follow. This configuration option sets the number of partitions per device in the driver. It doesn't create device nodes.

If the user has udev, then the device nodes are created correctly (on Fedora Core 3), so that if the driver is configured with 1 partition per device, the minor numbers for the disks are low.

Quoted 6 lines
Quoted 3 lines

The folks I've talked to who aren't using udev but want one partition per device already know that they have to re-create their device files.

It changes a kernel ABI, so people that have different config options set can't use the same userland. It's a really big no-go.

I don't know if it matters now that we have udev. When udev manages the device nodes it all just works, so there's practically not much of an issue. The UUIDs in Software RAID and LVM make this even less of an issue. Besides, if they aren't using udev and are using a custom kernel *and* using this configure option, then they're changing the ABI on purpose for a practical benefit.

If you're saying that it's bad in principal, then that's another story. If that's what you mean, then it's a Linux policy issue, and to follow convention I'd think that we'd need another major number. That would be like the partitionable md devices, etc.

To me, the latter seems like the uglier way to go, and it would be more permanent, while the configure option seems ugly but convenient for this transitional stage, and could go away when high minor numbers are well supported on the systems currently in use.

Signature · 1 line
  Ed L Cashin <[email protected]>
#22 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 2 lines · Ed L Cashin

I don't know if it matters now that we have udev. When udev manages the device nodes it all just works,

But most peopel still don't use udev.

Quoted 4 lines

If you're saying that it's bad in principal, then that's another story. If that's what you mean, then it's a Linux policy issue, and to follow convention I'd think that we'd need another major number. That would be like the partitionable md devices, etc.

Yes, it's a policy issue. We don't do this weird config option anywhere else.

#23 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Ed L Cashin

I don't know if it matters now that we have udev. When udev manages the device nodes it all just works,

But most peopel still don't use udev.

Quoted 7 lines
Quoted 4 lines

If you're saying that it's bad in principal, then that's another story. If that's what you mean, then it's a Linux policy issue, and to follow convention I'd think that we'd need another major number. That would be like the partitionable md devices, etc.

Yes, it's a policy issue. We don't do this weird config option anywhere else.

A couple support calls later, I think I've come around to your point of view. This patch isn't needed and may cause confusion.

Few aoe users really use partitions on their aoe disks, so I can make the aoe driver have one minor number per disk as the default to avoid the most common problems people encounter.

Then, aoe users who really need to partition their network disks can use the partitionable md driver to "wrap" the aoe disk, like this:

  mdadm -B -l linear --force -n 1 --auto=mdp /dev/md_p0 /dev/etherd/e7.0
  fdisk /dev/md_p0
Signature · 1 line
  Ed L Cashin <[email protected]>
author
#24 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 23 lines · Ed L Cashin
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Ed L Cashin

I don't know if it matters now that we have udev. When udev manages the device nodes it all just works,

But most peopel still don't use udev.

Quoted 7 lines
Quoted 4 lines

If you're saying that it's bad in principal, then that's another story. If that's what you mean, then it's a Linux policy issue, and to follow convention I'd think that we'd need another major number. That would be like the partitionable md devices, etc.

Yes, it's a policy issue. We don't do this weird config option anywhere else.

A couple support calls later, I think I've come around to your point of view. This patch isn't needed and may cause confusion.

Few aoe users really use partitions on their aoe disks, so I can make the aoe driver have one minor number per disk as the default to avoid the most common problems people encounter.

Quoted 5 lines

Then, aoe users who really need to partition their network disks can use the partitionable md driver to "wrap" the aoe disk, like this:

  mdadm -B -l linear --force -n 1 --auto=mdp /dev/md_p0 /dev/etherd/e7.0
  fdisk /dev/md_p0

So, which one of the aoe patches listed at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/
do you want me to drop? This one:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/aoe-AOE_PARTITIONS.patch
?
Or some other one too?

thanks,

greg k-h

#25 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Greg KH <[email protected]> writes:

...

Quoted 6 lines

So, which one of the aoe patches listed at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/
do you want me to drop? This one:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/aoe-AOE_PARTITIONS.patch
?
Or some other one too?

Just aoe-AOE_PARTITIONS.patch, the seventh of the twelve, should be dropped.

Then later I'll send a batch of patches that will include a change to make aoe disks non-partitionable by default.

Signature · 1 line
  Ed L Cashin <[email protected]>
author
#26 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 10 lines · Ed L Cashin

Greg KH <[email protected]> writes:

...

Quoted 6 lines

So, which one of the aoe patches listed at:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/
do you want me to drop? This one:
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver/aoe-AOE_PARTITIONS.patch
?
Or some other one too?

Quoted 2 lines

Just aoe-AOE_PARTITIONS.patch, the seventh of the twelve, should be dropped.

Ok, dropped.

Quoted 2 lines

Then later I'll send a batch of patches that will include a change to make aoe disks non-partitionable by default.

That's fine. Mind if I forward the other aoe patches in that directory to Linus soon?

thanks,

greg k-h

#27 lore raw

Re: [PATCH 2.6.11] aoe [7/12]: support configuration of AOE_PARTITIONS from Kconfig

Quoted 1 line · Greg KH

On Thu, Apr 07, 2005 at 02:56:39PM -0400, Ed L Cashin wrote:

...

Quoted 10 lines
Quoted 2 lines

Just aoe-AOE_PARTITIONS.patch, the seventh of the twelve, should be dropped.

Ok, dropped.

Quoted 2 lines

Then later I'll send a batch of patches that will include a change to make aoe disks non-partitionable by default.

That's fine. Mind if I forward the other aoe patches in that directory to Linus soon?

Go ahead. I have a new batch of patches to send, but it looks like I might not get to it for a few days.

Signature · 1 line
  Ed L Cashin <[email protected]>
#28 lore raw

PATCH 1/1 aoe [10/12]: Randy Dunlap: avoid warnings on sparc64

In reply to Greg KH

Andrew Morton has already merged this patch.

Randy Dunlap: avoid warnings on sparc64

drivers/block/aoe/aoeblk.c +4 -2
diff -uprN a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
@@ -28,7 +28,8 @@ static ssize_t aoedisk_show_mac(struct g
 {
 	struct aoedev *d = disk->private_data;
 
-	return snprintf(page, PAGE_SIZE, "%012llx\n", mac_addr(d->addr));
+	return snprintf(page, PAGE_SIZE, "%012llx\n",
+			(unsigned long long)mac_addr(d->addr));
 }
 static ssize_t aoedisk_show_netif(struct gendisk * disk, char *page)
 {
@@ -241,7 +242,8 @@ aoeblk_gdalloc(void *vp)
 	aoedisk_add_sysfs(d);
 	
 	printk(KERN_INFO "aoe: %012llx e%lu.%lu v%04x has %llu "
-		"sectors\n", mac_addr(d->addr), d->aoemajor, d->aoeminor,
+		"sectors\n", (unsigned long long)mac_addr(d->addr),
+		d->aoemajor, d->aoeminor,
 		d->fw_ver, (long long)d->ssize);
 }
 
Signature · 1 line
  Ed L. Cashin <[email protected]>
#29 lore raw

PATCH 1/1 aoe [12/12]: send outgoing packets in order

In reply to Greg KH

I can't use list.h, since sk_buff doesn't have a list_head but instead has two struct sk_buff pointers, and I want to avoid any extra memory allocation.

send outgoing packets in order

drivers/block/aoe/aoe.h +2 -1
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -132,7 +132,8 @@ struct aoedev {
 	struct timer_list timer;
 	spinlock_t lock;
 	struct net_device *ifp;	/* interface ed is attached to */
-	struct sk_buff *skblist;/* packets needing to be sent */
+	struct sk_buff *sendq_hd; /* packets needing to be sent, list head */
+	struct sk_buff *sendq_tl;
 	mempool_t *bufpool;	/* for deadlock-free Buf allocation */
 	struct list_head bufq;	/* queue of bios to work on */
 	struct buf *inprocess;	/* the one we're currently working on */
drivers/block/aoe/aoeblk.c +2 -2
diff -uprN a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
@@ -147,8 +147,8 @@ aoeblk_make_request(request_queue_t *q, 
 	list_add_tail(&buf->bufs, &d->bufq);
 	aoecmd_work(d);
 
-	sl = d->skblist;
-	d->skblist = NULL;
+	sl = d->sendq_hd;
+	d->sendq_hd = d->sendq_tl = NULL;
 
 	spin_unlock_irqrestore(&d->lock, flags);
 
drivers/block/aoe/aoecmd.c +17 -9
diff -uprN a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
@@ -178,8 +178,12 @@ aoecmd_ata_rw(struct aoedev *d, struct f
 
 	skb = skb_prepare(d, f);
 	if (skb) {
-		skb->next = d->skblist;
-		d->skblist = skb;
+		skb->next = NULL;
+		if (d->sendq_hd)
+			d->sendq_tl->next = skb;
+		else
+			d->sendq_hd = skb;
+		d->sendq_tl = skb;
 	}
 }
 
@@ -227,8 +231,12 @@ rexmit(struct aoedev *d, struct frame *f
 
 	skb = skb_prepare(d, f);
 	if (skb) {
-		skb->next = d->skblist;
-		d->skblist = skb;
+		skb->next = NULL;
+		if (d->sendq_hd)
+			d->sendq_tl->next = skb;
+		else
+			d->sendq_hd = skb;
+		d->sendq_tl = skb;
 	}
 }
 
@@ -280,8 +288,8 @@ tdie:		spin_unlock_irqrestore(&d->lock, 
 		}
 	}
 
-	sl = d->skblist;
-	d->skblist = NULL;
+	sl = d->sendq_hd;
+	d->sendq_hd = d->sendq_tl = NULL;
 	if (sl) {
 		n = d->rttavg <<= 1;
 		if (n > MAXTIMER)
@@ -481,8 +489,8 @@ aoecmd_ata_rsp(struct sk_buff *skb)
 
 	aoecmd_work(d);
 
-	sl = d->skblist;
-	d->skblist = NULL;
+	sl = d->sendq_hd;
+	d->sendq_hd = d->sendq_tl = NULL;
 
 	spin_unlock_irqrestore(&d->lock, flags);
 
@@ -531,7 +539,7 @@ aoecmd_cfg(ushort aoemajor, unsigned cha
  
 /*
  * Since we only call this in one place (and it only prepares one frame)
- * we just return the skb.  Usually we'd chain it up to the d->skblist.
+ * we just return the skb.  Usually we'd chain it up to the aoedev sendq.
  */
 static struct sk_buff *
 aoecmd_ata_id(struct aoedev *d)
Signature · 1 line
  Ed L. Cashin <[email protected]>
#30 lore raw

PATCH 1/1 aoe [11/12]: add support for disk statistics

In reply to Greg KH

add support for disk statistics

drivers/block/aoe/aoe.h +1 -0
diff -uprN a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
@@ -91,6 +91,7 @@ enum {
 
 struct buf {
 	struct list_head bufs;
+	ulong start_time;	/* for disk stats */
 	ulong flags;
 	ulong nframesout;
 	char *bufaddr;
drivers/block/aoe/aoeblk.c +1 -0
diff -uprN a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
@@ -125,6 +125,7 @@ aoeblk_make_request(request_queue_t *q, 
 	}
 	memset(buf, 0, sizeof(*buf));
 	INIT_LIST_HEAD(&buf->bufs);
+	buf->start_time = jiffies;
 	buf->bio = bio;
 	buf->resid = bio->bi_size;
 	buf->sector = bio->bi_sector;
drivers/block/aoe/aoecmd.c +14 -0
diff -uprN a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
@@ -456,6 +456,20 @@ aoecmd_ata_rsp(struct sk_buff *skb)
 	if (buf) {
 		buf->nframesout -= 1;
 		if (buf->nframesout == 0 && buf->resid == 0) {
+			unsigned long duration = jiffies - buf->start_time;
+			unsigned long n_sect = buf->bio->bi_size >> 9;
+			struct gendisk *disk = d->gd;
+
+			if (bio_data_dir(buf->bio) == WRITE) {
+				disk_stat_inc(disk, writes);
+				disk_stat_add(disk, write_ticks, duration);
+				disk_stat_add(disk, write_sectors, n_sect);
+			} else {
+				disk_stat_inc(disk, reads);
+				disk_stat_add(disk, read_ticks, duration);
+				disk_stat_add(disk, read_sectors, n_sect);
+			}
+			disk_stat_add(disk, io_ticks, duration);
 			n = (buf->flags & BUFFL_FAIL) ? -EIO : 0;
 			bio_endio(buf->bio, buf->bio->bi_size, n);
 			mempool_free(buf, d->bufpool);
Signature · 1 line
  Ed L. Cashin <[email protected]>