Skip to contentSkip to conversation

ATA over Ethernet driver for 2.6.9 (with changes)

PATCH In review Ed L Cashin 4 participants 8 messages Dec 13, 2004 – Dec 14, 2004

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ATA over Ethernet driver for 2.6.9 (with changes)

I've implemented the changes suggested here for the AoE driver for 2.6.9. Here's a list of changes since December 6 when the 2.6.9 patch was first submitted. Suggestions made by more than one person are only listed once.

Greg KH suggestions:
* tell sysfs about our char drivers with class_simple
* don't use typedefs
* use list.h macros instead of our queue manipulation functions
* get rid of our getfields function
* remove stacked if statements (a Bell Labs-ism)
* comment ioctl: device node permissions should be restrictive
* return -ENOTTY for unsupported ioctls
* return apropos errors instead of -1 in char device operations
* remove aoeutils.c and use in-kernel functionality
* move example scripts into Documentation/aoe

Jan-Benedict Glaw suggestions:
* move Kconfig entry into parent dir
* use NULL instead of nil
* use ARRAY_SIZE instead of our nelems macro
* remove unused device path macros
* use C99 struct initializers
* use __init and __exit more

Arjan van de Ven suggestions:
* remove u.h
* use mempool allocation in make_request_fn
* use bdev->bd_openers instead of our own nopen

Pekka Enberg suggestions:
* name our header aoe.h
* only include needed headers each .c file
* split one enum according to meaning
* eliminate unnecessary casts from void * to typed pointer

Provide support for ATA over Ethernet devices

Documentation/aoe/aoe.txt +59 -0
diff -urNp linux-2.6.9/Documentation/aoe/aoe.txt linux-2.6.9-aoe/Documentation/aoe/aoe.txt
@@ -0,0 +1,59 @@
+The EtherDrive (R) HOWTO for users of 2.6 kernels is found at ...
+
+  http://www.coraid.com/support/linux/EtherDrive-2.6-HOWTO.html
+
+  It has many tips and hints!
+
+CREATING DEVICE NODES
+
+  Users of udev should find device nodes created automatically.  Two
+  scripts are provided in Documentation/aoe as examples of static
+  device node creation for using the aoe driver.
+
+    rm -rf /dev/etherd
+    sh Documentation/aoe/mkdevs /dev/etherd
+
+  ... or to make just one shelf's worth of block device nodes ...
+
+    sh Documentation/aoe/mkshelf /dev/etherd 0
+
+  There is also an autoload script that shows how to edit
+  /etc/modprobe.conf to ensure that the aoe module is loaded when
+  necessary.
+
+USING DEVICE NODES
+
+  "cat /dev/etherd/stat" shows the status of discovered AoE devices on
+  your LAN:
+
+	root@nai root# cat /dev/etherd/stat
+	/dev/etherd/e15.3       eth0    up
+	/dev/etherd/e6.2        eth3    up
+	/dev/etherd/e6.4        eth3    up
+	/dev/etherd/e6.3        eth3    up
+	/dev/etherd/e6.9        eth3    up
+	/dev/etherd/e6.5        eth3    up
+	/dev/etherd/e6.7        eth3    up
+	/dev/etherd/e6.6        eth3    up
+	/dev/etherd/e6.8        eth3    up
+	/dev/etherd/e6.0        eth3    up
+	/dev/etherd/e6.1        eth3    up
+
+  "cat /dev/etherd/err" blocks, waiting for error diagnostic output,
+  like any retransmitted packets.
+
+  "echo interfaces eth2 eth4 > /dev/etherd/ctl" tells the aoe driver
+  to limit ATA over Ethernet traffic to eth2 and eth4.  AoE traffic
+  from untrusted networks should be ignored as a matter of security.
+
+  "echo discover > /dev/etherd/ctl" tells the driver to find out what
+  AoE devices are available.
+
+  The block devices are named like this:
+
+	e{shelf}.{slot}
+	e{shelf}.{slot}p{part}
+
+  ... so that "e0.2" is the third blade from the left (slot 2) in the
+  first shelf (shelf address zero).  That's the whole disk.  The first
+  partition on that disk would be "e0.2p1".
Documentation/aoe/autoload +17 -0
diff -urNp linux-2.6.9/Documentation/aoe/autoload linux-2.6.9-aoe/Documentation/aoe/autoload
@@ -0,0 +1,17 @@
+#!/bin/sh
+# set aoe to autoload by installing the
+# aliases in /etc/modprobe.conf
+
+f=/etc/modprobe.conf
+
+if test ! -r $f || test ! -w $f; then
+	echo "cannot configure $f for module autoloading" 1>&2
+	exit 1
+fi
+
+grep major-152 $f >/dev/null
+if [ $? = 1 ]; then
+	echo alias block-major-152 aoe >> $f
+	echo alias char-major-152 aoe >> $f
+fi
+
Documentation/aoe/mkdevs +30 -0
diff -urNp linux-2.6.9/Documentation/aoe/mkdevs linux-2.6.9-aoe/Documentation/aoe/mkdevs
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+n_shelves=10
+
+if test "$#" != "1"; then
+	echo "Usage: mkdevs {dir}" 1>&2
+	exit 1
+fi
+dir=$1
+
+MAJOR=152
+
+echo "Creating AoE devnode files in $dir ..."
+
+set -e
+
+mkdir -p $dir
+
+rm -f $dir/ctl
+mknod -m 0200 $dir/ctl c $MAJOR 0
+rm -f $dir/stat
+mknod -m 0400 $dir/stat c $MAJOR 1
+rm -f $dir/err
+mknod -m 0400 $dir/err c $MAJOR 2
+
+i=0
+while test $i -lt $n_shelves; do
+	sh -xc "`dirname $0`/mkshelf $dir $i"
+	i=`expr $i + 1`
+done
Documentation/aoe/mkshelf +23 -0
diff -urNp linux-2.6.9/Documentation/aoe/mkshelf linux-2.6.9-aoe/Documentation/aoe/mkshelf
@@ -0,0 +1,23 @@
+#! /bin/sh
+
+if test "$#" != "2"; then
+	echo "Usage: mkshelf {dir} {shelfaddress}" 1>&2
+	exit 1
+fi
+dir=$1
+shelf=$2
+MAJOR=152
+
+set -e
+
+minor=`echo 10 \* $shelf \* 16 | bc`
+for slot in `seq 0 9`; do
+	for part in `seq 0 15`; do
+		name=e$shelf.$slot
+		test "$part" != "0" && name=${name}p$part
+		rm -f $dir/$name
+		mknod -m 0660 $dir/$name b $MAJOR $minor
+
+		minor=`expr $minor + 1`
+	done
+done
MAINTAINERS +6 -0
diff -urNp linux-2.6.9/MAINTAINERS linux-2.6.9-aoe/MAINTAINERS
@@ -329,6 +329,12 @@ L:	[email protected]
 W:	http://linux-atm.sourceforge.net
 S:	Maintained
 
+ATA OVER ETHERNET DRIVER
+P:	Ed L. Cashin
+M:	[email protected]
+W:	http://www.coraid.com/support/linux
+S:	Supported
+
 ATMEL WIRELESS DRIVER
 P:	Simon Kelley
 M:	[email protected]
drivers/Makefile +1 -0
diff -urNp linux-2.6.9/drivers/Makefile linux-2.6.9-aoe/drivers/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_DIO)		+= dio/
 obj-$(CONFIG_SBUS)		+= sbus/
 obj-$(CONFIG_ZORRO)		+= zorro/
 obj-$(CONFIG_MAC)		+= macintosh/
+obj-$(CONFIG_ATA_OVER_ETH)	+= block/aoe/
 obj-$(CONFIG_PARIDE) 		+= block/paride/
 obj-$(CONFIG_TC)		+= tc/
 obj-$(CONFIG_USB)		+= usb/
drivers/block/Kconfig +8 -0
diff -urNp linux-2.6.9/drivers/block/Kconfig linux-2.6.9-aoe/drivers/block/Kconfig
@@ -358,4 +358,12 @@ config LBD
 
 source "drivers/s390/block/Kconfig"
 
+config ATA_OVER_ETH
+	tristate "ATA over Ethernet support"
+	depends on NET
+	default m
+	help
+	This driver provides Support for ATA over Ethernet block
+	devices like the Coraid EtherDrive (R) Storage Blade.
+
 endmenu
drivers/block/aoe/Makefile +6 -0
diff -urNp linux-2.6.9/drivers/block/aoe/Makefile linux-2.6.9-aoe/drivers/block/aoe/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for ATA over Ethernet
+#
+
+obj-$(CONFIG_ATA_OVER_ETH)	+= aoe.o
+aoe-objs := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o
drivers/block/aoe/aoe.h +164 -0

164 lines not on this page · raw message

drivers/block/aoe/aoeblk.c +190 -0

190 lines not on this page · raw message

drivers/block/aoe/aoechr.c +319 -0

319 lines not on this page · raw message

drivers/block/aoe/aoecmd.c +626 -0

626 lines not on this page · raw message

drivers/block/aoe/aoedev.c +237 -0

237 lines not on this page · raw message

drivers/block/aoe/aoemain.c +89 -0

89 lines not on this page · raw message

drivers/block/aoe/aoenet.c +172 -0

172 lines not on this page · raw message

Signature · 2 lines
-- 
  Ed L Cashin <[email protected]>
#1 lore raw
Quoted 1 line · Ed L Cashin

* use mempool allocation in make_request_fn

It's not good enough, if cannot use a higher allocation priority that GFP_NOIO here - basically guarantee that your allocation will not block on further io. Currently you have the very same deadlock as before, the mempool does not help you since you call into the allocator and deadlock before ever blocking on the mempool.

Signature · 1 line
Jens Axboe
#2 lore raw
Quoted 8 lines · Jens Axboe
Quoted 1 line · Ed L Cashin

* use mempool allocation in make_request_fn

It's not good enough, if cannot use a higher allocation priority that GFP_NOIO here - basically guarantee that your allocation will not block on further io. Currently you have the very same deadlock as before, the mempool does not help you since you call into the allocator and deadlock before ever blocking on the mempool.

Do you mean that with GFP_KERNEL we may still deadlock on line 199 of the snippet below (from mm/mempool.c)? That alloc pointer points to mempool_alloc_slab, which gets called with __GFP_WAIT turned off. The kmem_cache allocator doesn't get called with the allocation priority we specify in our make_request_fn, so we won't block there.

   190	void * mempool_alloc(mempool_t *pool, int gfp_mask)
   191	{
   192		void *element;
   193		unsigned long flags;
   194		DEFINE_WAIT(wait);
   195		int gfp_nowait = gfp_mask & ~(__GFP_WAIT | __GFP_IO);
   196	
   197		might_sleep_if(gfp_mask & __GFP_WAIT);
   198	repeat_alloc:
   199		element = pool->alloc(gfp_nowait|__GFP_NOWARN, pool->pool_data);
   200		if (likely(element != NULL))
   201			return element;
   202	

If we block later on the pool, that's because there are 16 objects in use, which means that mempool_free is going to get called 16 times as I/O completes, so I/O is throttled and forward progress is guaranteed. Otherwise, how does the mempool mechanism help in preventing deadlock?

It looks like we can simply change GFP_KERNEL to GFP_IO in our make_request_fn, but I'd also like to understand why that's necessary when there's a dedicated pre-allocated pool per aoe device.

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

(don't trim cc lists, please)

Quoted 17 lines · Ed L Cashin
Quoted 8 lines · Jens Axboe
Quoted 1 line · Ed L Cashin

* use mempool allocation in make_request_fn

It's not good enough, if cannot use a higher allocation priority that GFP_NOIO here - basically guarantee that your allocation will not block on further io. Currently you have the very same deadlock as before, the mempool does not help you since you call into the allocator and deadlock before ever blocking on the mempool.

Do you mean that with GFP_KERNEL we may still deadlock on line 199 of the snippet below (from mm/mempool.c)? That alloc pointer points to mempool_alloc_slab, which gets called with __GFP_WAIT turned off. The kmem_cache allocator doesn't get called with the allocation priority we specify in our make_request_fn, so we won't block there.

Quoted 13 lines
   190	void * mempool_alloc(mempool_t *pool, int gfp_mask)
   191	{
   192		void *element;
   193		unsigned long flags;
   194		DEFINE_WAIT(wait);
   195		int gfp_nowait = gfp_mask & ~(__GFP_WAIT | __GFP_IO);
   196	
   197		might_sleep_if(gfp_mask & __GFP_WAIT);
   198	repeat_alloc:
   199		element = pool->alloc(gfp_nowait|__GFP_NOWARN, pool->pool_data);
   200		if (likely(element != NULL))
   201			return element;
   202	

No, line 199 is safe. But you risk deadlocking at line 210 because you call the page allocator with GFP_KERNEL at that point (which includes __GFP_IO and __GFP_FS).

Quoted 4 lines

If we block later on the pool, that's because there are 16 objects in use, which means that mempool_free is going to get called 16 times as I/O completes, so I/O is throttled and forward progress is guaranteed. Otherwise, how does the mempool mechanism help in preventing deadlock?

Doesn't matter, since you are already deadlocked on the ->alloc call. If you have IO set in the gfp_mask, you could reenter your own driver, which will call ->alloc, which will reenter... And then you hang.

Quoted 3 lines

It looks like we can simply change GFP_KERNEL to GFP_IO in our make_request_fn, but I'd also like to understand why that's necessary when there's a dedicated pre-allocated pool per aoe device.

No, you need to use GFP_NOIO. You can block on the mempool itself, because the objects you allocate from there have a finite life time. They will be freed once some io completes. But you cannot block on other IO, because that might be up to you do to.

So the safe mask simply __GFP_WAIT - it allows blocking waiting for your mempool to refill, nothing more.

Signature · 1 line
Jens Axboe
#4 lore raw
In reply to Ed L Cashin

On Mon, 2004-12-13 11:04:51 -0500, Ed L Cashin <[email protected]> wrote in message <[email protected]>:
[...]

Impressive list of changes. I'm thinking about implementing a userland server for AoE. Is there a formal protocol specification available? Though, I'd use the block driver's sources to reverse engineer it, but for interoperability purposes, it would probably be better to start off a specification than an implementation.

. o O (...and I'd love to get my hands on a real hardware device)

Quoted 6 lines

diff -urNp linux-2.6.9/drivers/block/aoe/aoecmd.c linux-2.6.9-aoe/drivers/block/aoe/aoecmd.c

drivers/block/aoe/aoecmd.c +3 -0
+static void
+aoecmd_ata_rw(struct Aoedev *d, struct Frame *f)
+{

[...]

drivers/block/aoe/aoecmd.c line 153

Quoted 8 lines
drivers/block/aoe/aoecmd.c +8 -0
+	if (d->flags & DEVFL_EXT) {
+		ah->aflags |= AOEAFL_EXT;
+		ah->lba4 = sector >>= 8;
+		ah->lba5 = sector >>= 8;
+	} else {
+		extbit = 0;
+		ah->lba3 &= 0x0f;
+		ah->lba3 |= 0xe0;	/* LBA bit + obsolete 0xa0 */

This comment doesn't match it's code.

MfG, JBG

Signature · 4 lines
Jan-Benedict Glaw       [email protected]    . +49-172-7608481             _ O _
"Eine Freie Meinung in  einem Freien Kopf    | Gegen Zensur | Gegen Krieg  _ _ O
 fuer einen Freien Staat voll Freier Bürger" | im Internet! |   im Irak!   O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));
#5 lore raw
Quoted 6 lines · Jan-Benedict Glaw

On Mon, 2004-12-13 11:04:51 -0500, Ed L Cashin <[email protected]> wrote in message <[email protected]>:
[...]

Impressive list of changes. I'm thinking about implementing a userland server for AoE. Is there a formal protocol specification available?

Yes. It's here:

http://www.coraid.com/documents/AoEr8.txt
http://www.coraid.com/documents/AoEr8.pdf

Quoted 5 lines

Though, I'd use the block driver's sources to reverse engineer it, but for interoperability purposes, it would probably be better to start off a specification than an implementation.

. o O (...and I'd love to get my hands on a real hardware device)

There are some eval kits that can be ordered on our web site.

Signature · 1 line
  Ed L Cashin <[email protected]>
#6 lore raw
In reply to Ed L Cashin
Quoted 7 lines · Ed L Cashin
Documentation/aoe/aoe.txt +7 -0
+USING DEVICE NODES
+
+  "cat /dev/etherd/stat" shows the status of discovered AoE devices on
+  your LAN:
+
+	root@nai root# cat /dev/etherd/stat
+	/dev/etherd/e15.3       eth0    up

Documentation/aoe/aoe.txt line 40

Quoted 10 lines
Documentation/aoe/aoe.txt +10 -0
+	/dev/etherd/e6.2        eth3    up
+	/dev/etherd/e6.4        eth3    up
+	/dev/etherd/e6.3        eth3    up
+	/dev/etherd/e6.9        eth3    up
+	/dev/etherd/e6.5        eth3    up
+	/dev/etherd/e6.7        eth3    up
+	/dev/etherd/e6.6        eth3    up
+	/dev/etherd/e6.8        eth3    up
+	/dev/etherd/e6.0        eth3    up
+	/dev/etherd/e6.1        eth3    up

Again, can't you move this out into sysfs in the block directory for the different partitions? Or do you have userspace programs that are expecting the stat device node to be present and export this information?

Documentation/aoe/aoe.txt line 50

Quoted 9 lines
Documentation/aoe/aoe.txt +9 -0
+  "cat /dev/etherd/err" blocks, waiting for error diagnostic output,
+  like any retransmitted packets.
+
+  "echo interfaces eth2 eth4 > /dev/etherd/ctl" tells the aoe driver
+  to limit ATA over Ethernet traffic to eth2 and eth4.  AoE traffic
+  from untrusted networks should be ignored as a matter of security.
+
+  "echo discover > /dev/etherd/ctl" tells the driver to find out what
+  AoE devices are available.

Can't you split the ctl device node up into 2 different ones? One for the interface command, and one for the discover command? Any future commands would use additional device nodes, you have a whole major reserved, might as well take advantage of it :)

That way, you can reduce the ammount of parsing logic you have to do in your driver.

drivers/block/aoe/aoe.h line 1

Quoted 5 lines

diff -urNp linux-2.6.9/drivers/block/aoe/aoe.h linux-2.6.9-aoe/drivers/block/aoe/aoe.h

drivers/block/aoe/aoe.h +1 -0
@@ -0,0 +1,164 @@
+#define VER 3

This should be used in a MODULE_VERSION() declaration, instead of a printk() line.

drivers/block/aoe/aoe.h line 3

Quoted 2 lines
drivers/block/aoe/aoe.h +2 -0
+#define AOE_MAJOR 152
+#define MAX_ARGS 16

This is not used anywhere.

drivers/block/aoe/aoe.h line 5

Quoted 2 lines
drivers/block/aoe/aoe.h +2 -0
+#define DEVICE_NAME "aoe"
+#define DEVICE_NO_RANDOM

This is not used anywhere.

drivers/block/aoe/aoe.h line 33

Quoted 1 line
drivers/block/aoe/aoe.h +1 -0
+struct Aoehdr {

Ah, so close. You dropped all of the typedefs, thank you. But what's with the capital letter in the structure name? Just make it "aoehdr" or "aoe_header" or something that doesn't have capital letters in it.

Same thing for all of your structure names. A simple sed script should fix them all up for you :)

drivers/block/aoe/aoe.h line 76

Quoted 10 lines
drivers/block/aoe/aoe.h +10 -0
+enum {
+	DEVFL_UP = 1,	/* device is installed in system and ready for AoE->ATA commands */
+	DEVFL_TKILL = (1<<1),	/* flag for timer to know when to kill self */
+	DEVFL_EXT = (1<<2),	/* device accepts lba48 commands */
+	DEVFL_CLOSEWAIT = (1<<3), /* device is waiting for all closes to revalidate */
+	DEVFL_WC_UPDATE = (1<<4), /* this device needs to update write cache status */
+	DEVFL_WORKON = (1<<4),
+
+	BUFFL_FAIL = 1,
+};

Any reason why BUFFL_FAIL and DEVFL_UP are the same value? It looks like they can be used in the same variable right?

The class_simple stuff looked sane, nice job.

thanks,

greg k-h

#7 lore raw

Greg KH <[email protected]> writes:

...

Quoted 5 lines
Quoted 5 lines
drivers/block/aoe/aoe.h +5 -0
+enum {
+	DEVFL_UP = 1,	/* device is installed in system and ready for AoE->ATA commands */
+	DEVFL_TKILL = (1<<1),	/* flag for timer to know when to kill self */
+	DEVFL_EXT = (1<<2),	/* device accepts lba48 commands */
+	DEVFL_CLOSEWAIT = (1<<3), /* device is waiting for all closes to revalidate */
Quoted 8 lines

drivers/block/aoe/aoe.h line 76

Quoted 5 lines
drivers/block/aoe/aoe.h +5 -0
+	DEVFL_WC_UPDATE = (1<<4), /* this device needs to update write cache status */
+	DEVFL_WORKON = (1<<4),
+
+	BUFFL_FAIL = 1,
+};

Any reason why BUFFL_FAIL and DEVFL_UP are the same value? It looks like they can be used in the same variable right?

The struct Buf and the struct Aoedev both have a flags member. BUFFL_* is for the flags member of the Buf, while DEVFL_* are bits for the flags member of the struct Aoedev.

Although they both happen to be 1, they are conceptually unrelated, so they have different names. They should not be used in the same variable. BUFFL_ for the flags of a struct Buf, and DEVFL_ for the flags of a struct Aoedev.

Quoted 2 lines

The class_simple stuff looked sane, nice job.

Thanks! It was pretty easy.

Signature · 1 line
  Ed L Cashin <[email protected]>