ATA over Ethernet driver for 2.6.9 (with changes)
Subsystems (3)
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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]>
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
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.clinux-2.6.9-aoe/drivers/block/aoe/aoecmd.cdrivers/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));
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.hlinux-2.6.9-aoe/drivers/block/aoe/aoe.hdrivers/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
Quoted 8 lines · Jens Axboe
Quoted 1 line · Ed L Cashin
* use mempool allocation in
make_request_fnIt's not good enough, if cannot use a higher allocation priority that
GFP_NOIOhere - 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]>
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_FAILandDEVFL_UPare 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_simplestuff looked sane, nice job.
Thanks! It was pretty easy.
Signature · 1 line
Ed L Cashin <[email protected]>
(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_fnIt's not good enough, if cannot use a higher allocation priority that
GFP_NOIOhere - 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_KERNELwe may still deadlock on line 199 of the snippet below (frommm/mempool.c)? That alloc pointer points tomempool_alloc_slab, which gets called with__GFP_WAITturned off. Thekmem_cacheallocator doesn't get called with the allocation priority we specify in ourmake_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_freeis 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_KERNELtoGFP_IOin ourmake_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
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]>