ATA over Ethernet driver for 2.6.10-rc3-bk11
PATCH 1/1 ATA over Ethernet driver for 2.6.10-rc3-bk11
I've implemented the second round of changes suggested here for the AoE driver. Here's a list of changes since December 13 when the revised 2.6.9 patch was submitted.
Greg KH suggestions:
* remove stat char device and put status info into sysfs
* split ctl char device into discover and interfaces
* use MODULE_VERSION
* remove unused macros from aoe.h
* make struct tags lowercase
Jens Axboe suggestions:
* use GFP_NOIO in make_request_fn on mempool alloc
Provide support for ATA over Ethernet devices
Documentation/aoe/aoe.txt +75 -0
@@ -0,0 +1,75 @@ +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.sh /dev/etherd + + ... or to make just one shelf's worth of block device nodes ... + + sh Documentation/aoe/mkshelf.sh /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/err" blocks, waiting for error diagnostic output, + like any retransmitted packets. + + "echo eth2 eth4 > /dev/etherd/interfaces" 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 > /dev/etherd/discover" 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". + +USING SYSFS + + Each aoe block device in /sys/block has the extra attributes of + state, mac, and netif. The state attribute is "up" when the device + is ready for I/O and "down" if detected but unusable. The + "down,closewait" state shows that the device is still open and + cannot come up again until it has been closed. + + The mac attribute is the ethernet address of the remote AoE device. + The netif attribute is the network interface on the localhost + through which we are communicating with the remote AoE device. + + There is a script in this directory that formats this information + in a convenient way. + + root@makki linux# sh Documentation/aoe/status.sh + device mac netif state + e6.0 0010040010c6 eth0 up + e6.1 001004001067 eth0 up + e6.2 001004001068 eth0 up + e6.3 001004001065 eth0 up + e6.4 001004001066 eth0 up + e6.5 0010040010c7 eth0 up + e6.6 0010040010c8 eth0 up + e6.7 0010040010c9 eth0 up + e6.8 0010040010ca eth0 up + e6.9 0010040010cb eth0 up + e9.0 001004000020 eth1 up + e9.5 001004000025 eth1 up + e9.9 001004000029 eth1 up +
Documentation/aoe/autoload.sh +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.sh +33 -0
@@ -0,0 +1,33 @@ +#!/bin/sh + +n_shelves=10 + +if test "$#" != "1"; then + echo "Usage: sh mkdevs.sh {dir}" 1>&2 + exit 1 +fi +dir=$1 + +MAJOR=152 + +echo "Creating AoE devnode files in $dir ..." + +set -e + +mkdir -p $dir + +# (Status info is in sysfs. See status.sh.) +# rm -f $dir/stat +# mknod -m 0400 $dir/stat c $MAJOR 1 +rm -f $dir/err +mknod -m 0400 $dir/err c $MAJOR 2 +rm -f $dir/discover +mknod -m 0200 $dir/discover c $MAJOR 3 +rm -f $dir/interfaces +mknod -m 0200 $dir/interfaces c $MAJOR 4 + +i=0 +while test $i -lt $n_shelves; do + sh -xc "sh `dirname $0`/mkshelf.sh $dir $i" + i=`expr $i + 1` +done
Documentation/aoe/mkshelf.sh +23 -0
@@ -0,0 +1,23 @@ +#! /bin/sh + +if test "$#" != "2"; then + echo "Usage: sh mkshelf.sh {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
Documentation/aoe/status.sh +15 -0
@@ -0,0 +1,15 @@ +# collate and present sysfs information about AoE storage + +set -e +format="%8s\t%12s\t%8s\t%8s\n" + +printf "$format" device mac netif state + +for d in `ls -d /sys/block/etherd* | grep -v p`; do + dev=`echo "$d" | sed 's/.*!//'` + printf "$format" \ + "$dev" \ + "`cat \"$d/mac\"`" \ + "`cat \"$d/netif\"`" \ + "`cat \"$d/state\"`" +done | sort
MAINTAINERS +6 -0
@@ -328,6 +328,12 @@ W: http://sourceforge.net/projects/acpi4 W: http://julien.lerouge.free.fr S: Maintained +ATA OVER ETHERNET DRIVER +P: Ed L. Cashin +M: [email protected] +W: http://www.coraid.com/support/linux +S: Supported + ATM P: Chas Williams M: [email protected]
drivers/Makefile +1 -0
@@ -42,6 +42,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
@@ -429,4 +429,12 @@ source "drivers/s390/block/Kconfig" source "drivers/block/Kconfig.iosched" +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 +163 -0
163 lines not on this page · raw message
drivers/block/aoe/aoeblk.c +251 -0
251 lines not on this page · raw message
drivers/block/aoe/aoechr.c +295 -0
295 lines not on this page · raw message
drivers/block/aoe/aoecmd.c +627 -0
627 lines not on this page · raw message
drivers/block/aoe/aoedev.c +194 -0
194 lines not on this page · raw message
drivers/block/aoe/aoemain.c +93 -0
93 lines not on this page · raw message
drivers/block/aoe/aoenet.c +173 -0
173 lines not on this page · raw message
Signature · 2 lines
-- Ed L Cashin <[email protected]>
Quoted 11 lines · Ed L Cashin
I've implemented the second round of changes suggested here for the AoE driver. Here's a list of changes since December 13 when the revised 2.6.9 patch was submitted.
Greg KH suggestions:
* remove stat char device and put status info into sysfs
* split ctl char device into discover and interfaces
* useMODULE_VERSION
* remove unused macros fromaoe.h
* make struct tags lowercase
Quoted 5 lines
Jens Axboe suggestions:
* useGFP_NOIOinmake_request_fnon mempool allocProvide support for ATA over Ethernet devices
Looks good. I've added this to my driver bk tree and it will show up in the next -mm release, and I'll forward it on to Linus after 2.6.10 is out.
Oh, I applied the following patch on top of yours, to get rid of the sparse warnings, and get rid of a kmalloc that could have been potentially pretty nasty in your write() function. It saved a bit of code too :)
thanks,
greg k-h
----------
AOE: fix up sparse warnings and get rid of a kmalloc in the aoe driver.
drivers/block/aoe/aoe.h +1 -1
@@ -158,6 +158,6 @@void aoenet_exit (void );void aoenet_xmit (struct sk_buff *);int is_aoe_netif (struct net_device *ifp); -int set_aoe_iflist (char *str); +int set_aoe_iflist (const char __user *str,size_t size);u64 mac_addr (char addr[6 ]);
drivers/block/aoe/aoeblk.c +2 -2
@@ -170,7 +170,7 @@if (cmd ==HDIO_GETGEO ) { d->geo.start =get_start_sect (inode->i_bdev); -if (!copy_to_user ((void *) arg, &d->geo,sizeof d->geo)) +if (!copy_to_user ((void __user *) arg, &d->geo,sizeof d->geo))return 0 ;return -EFAULT ; }
@@ -227,7 +227,7 @@printk (KERN_INFO "aoe: %012llx e%lu.%lu v%04x has %llu " "sectors\n" ,mac_addr (d->addr), d->aoemajor, d->aoeminor, - d->fw_ver, d->ssize); + d->fw_ver, (long long )d->ssize); }void __exit
drivers/block/aoe/aoechr.c +6 -22
@@ -51,9 +51,9 @@ }static int -interfaces (char *str) +interfaces (const char __user *str,size_t size) { -if (set_aoe_iflist (str)) { +if (set_aoe_iflist (str, size)) {printk (KERN_CRIT "%s: could not set interface list: %s\n" , __FUNCTION__,"too many interfaces" );
@@ -135,24 +135,10 @@ }static ssize_t -aoechr_write (struct file *filp,const char *buf,size_t cnt,loff_t *offp) +aoechr_write (struct file *filp,const char __user *buf,size_t cnt,loff_t *offp) { -char *str =kcalloc (1 , cnt+1 ,GFP_KERNEL ); -int ret; +int ret = -EINVAL ; -if (!str) { -printk (KERN_CRIT "aoe: aoechr_write: cannot allocate memory\n" ); -return -ENOMEM ; - } - - ret = -EFAULT ; -if (copy_from_user (str, buf, cnt)) { -printk (KERN_INFO "aoe: aoechr_write: copy from user failed\n" ); - goto out; - } - - str[cnt] ='\0' ; - ret = -EINVAL ;switch ((unsigned long ) filp->private_data) {default :printk (KERN_INFO "aoe: aoechr_write: can't write to that file.\n" );
@@ -161,13 +147,11 @@ ret =discover ();break ;case MINOR_INTERFACES : - ret =interfaces (str); + ret =interfaces (buf, cnt);break ; }if (ret ==0 ) ret = cnt; - out: -kfree (str);return ret; }
@@ -192,7 +176,7 @@ }static ssize_t -aoechr_read (struct file *filp,char *buf,size_t cnt,loff_t *off) +aoechr_read (struct file *filp,char __user *buf,size_t cnt,loff_t *off) {int n;char *mp;
drivers/block/aoe/aoenet.c +7 -5
@@ -53,14 +53,16 @@ }int -set_aoe_iflist (char *str) +set_aoe_iflist (const char __user *user_str,size_t size) { -int len =strlen (str); - -if (len >=IFLISTSZ ) +if (size >=IFLISTSZ )return -EINVAL ; -strcpy (aoe_iflist, str); +if (copy_from_user (aoe_iflist, user_str, size)) { +printk (KERN_INFO "aoe: %s: copy from user failed\n" , __FUNCTION__); +return -EFAULT ; + } + aoe_iflist[size] =0x00 ;return 0 ; }
Quoted 1 line · Ed L Cashin
+
ETH_P_AOE=0x88a2,
include/linux/if_ether.h already defines this as ETH_P_EDP2=0x88A2; use that.
drivers/block/aoe/aoecmd.c line 93
Quoted 4 lines
drivers/block/aoe/aoecmd.c +4
-0+static int +aoehdr_atainit (struct aoedev *d,struct aoe_hdr *h) +{ +u16 type =__constant_cpu_to_be16 (ETH_P_AOE );
How about __constant_htons()?
> +aoecmd_cfg(ushort aoemajor, unsigned char aoeminor)
drivers/block/aoe/aoecmd.c line 487
Quoted 6 lines
drivers/block/aoe/aoecmd.c +6
-0+{ +struct aoe_hdr *h; +struct aoe_cfghdr *ch; +struct sk_buff *skb, *sl; +struct net_device *ifp; +u16 aoe_type =__constant_cpu_to_be16 (ETH_P_AOE );
Ditto
Quoted 11 lines
drivers/block/aoe/aoenet.c +11
-0+enum { +IFLISTSZ =1024 , +}; + +static char aoe_iflist[IFLISTSZ ]; + +int +is_aoe_netif (struct net_device *ifp) +{ + registerchar *p, *q; + registerint len;
drivers/block/aoe/aoenet.c
Quoted 10 lines
drivers/block/aoe/aoenet.c +10
-0+ +if (aoe_iflist[0 ] =='\0' ) +return 1 ; + +for (p = aoe_iflist; *p; p = q +strspn (q,WHITESPACE )) { + q = p +strcspn (p,WHITESPACE ); +if (q != p) + len = q - p; +else + len =strlen (p);/* last token in aoe_iflist
*/
Review
Quoted 2 lines
(quoted diff) +2
-0+ + if (strlen(ifp->name) == len && !strncmp(ifp->name, p,
len))
drivers/block/aoe/aoenet.c line 53
Quoted 7 lines
drivers/block/aoe/aoenet.c +7
-0+return 1 ; +if (q == p) +break ; + } + +return 0 ; +}
This is getting called for every skb received. Do you really want to walk a string array looking for "eth0" or "eth1" for every receive packet? Maybe making the locals "register" helps speed things up. :-)
Seriously, can you achieve the same by registering the packet type handler for each dev and let netif_receive_skb() do the check for skb->dev == packet_type->dev?
drivers/block/aoe/aoenet.c line 86
Quoted 8 lines
drivers/block/aoe/aoenet.c +8
-0+skb_check(struct sk_buff *skb) +{ +if (skb_is_nonlinear (skb)) +if ((skb =skb_share_check (skb,GFP_ATOMIC ))) +if (skb_linearize (skb,GFP_ATOMIC ) <0 ) { +dev_kfree_skb (skb); +return NULL ; + }
indentation of if's
Quoted 2 lines
+ * (1) i have no idea if this is redundant, but i can't figure why
+ * the ifp is passed in if it is.
This isn't necessary - see deliver_skb()
Quoted 1 line
+
skb->len+=ETH_HLEN; /* (2) */
You want to do a skb_push(skb, ETH_HLEN) here instead to keep skb->len and skb->data corresponding (basically to undo the skb->pull() in eth_type_trans()). skb->mac.raw will still be correct.
Quoted 1 line
+ n =
__be32_to_cpu(*((u32 *) h->tag));
Use ntohl() variant since this is network header field.
drivers/block/aoe/aoenet.c
Quoted 5 lines
drivers/block/aoe/aoenet.c +5
-0+ if (h->verfl & AOEFL_ERR) { + n = h->err; + if (n > NECODES) + n = 0; + printk(KERN_CRIT "aoe: aoenet_rcv: error packet from
%d.%d; "
drivers/block/aoe/aoenet.c line 139
Quoted 5 lines
drivers/block/aoe/aoenet.c +5
-0+"ecode=%d '%s'\n" , +__be16_to_cpu (*((u16 *) h->major)), h->minor, + h->err, aoe_errlist[n]); + goto exit; + }
Is there a better way to handle errors than flooding the log?
Quoted 1 line
+
dev_kfree_skb(skb);
kfree_skb() should be enough.
-scott
Quoted 6 lines · Scott Feldman
Quoted 1 line · Ed L Cashin
+
ETH_P_AOE=0x88a2,
include/linux/if_ether.halready defines this as ETH_P_EDP2=0x88A2; use that.
If my aging memory serves me, EDP2 == AOE.
Jeff
PATCH 1/1 ETH_P_AOE (was Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11)
In reply to Scott FeldmanQuoted 6 lines · Scott Feldman
Quoted 1 line · Ed L Cashin
+
ETH_P_AOE=0x88a2,
include/linux/if_ether.halready defines this as ETH_P_EDP2=0x88A2; use that.
Ah, that's old. It's not just "EtherDrive protocol" but the ATA over Ethernet protocol.
Here's a patch for 2.6.10-rc3-bk11 that goes on top of the aoe patch in this thread.
drivers/block/aoe/aoe.h +0 -1
@@ -26,7 +26,6 @@ enum {AOECCMD_FSET , AOE_HVER = 0x10, - ETH_P_AOE = 0x88a2, }; struct aoe_hdr {
include/linux/if_ether.h +1 -1
@@ -69,7 +69,7 @@#define ETH_P_ATMFATE 0x8884/* Frame-based ATM Transport * over Ethernet */ -#define ETH_P_EDP2 0x88A2/* Coraid EDP2 */ +#define ETH_P_AOE 0x88A2/* ATA over Ethernet */ /* * Non DIX types. Won't clash for 1500 types.
Signature · 2 lines
-- Ed L Cashin <[email protected]>
Greg KH
AppliedRe: [PATCH] ETH_P_AOE (was Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11)
Quoted 6 lines · Ed L Cashin
Quoted 4 lines · Scott Feldman
Quoted 1 line · Ed L Cashin
+
ETH_P_AOE=0x88a2,
Quoted 8 lines
Quoted 2 lines
include/linux/if_ether.halready defines this as ETH_P_EDP2=0x88A2; use that.Ah, that's old. It's not just "EtherDrive protocol" but the ATA over Ethernet protocol.
Here's a patch for 2.6.10-rc3-bk11 that goes on top of the aoe patch in this thread.
Applied to my trees, thanks.
greg k-h
Quoted 7 lines · Scott Feldman
Quoted 1 line · Ed L Cashin
+
ETH_P_AOE=0x88a2,
include/linux/if_ether.halready defines this as ETH_P_EDP2=0x88A2; use that.
Quoted 6 lines
drivers/block/aoe/aoecmd.c line 93
Quoted 4 lines
drivers/block/aoe/aoecmd.c +4
-0+static int +aoehdr_atainit (struct aoedev *d,struct aoe_hdr *h) +{ +u16 type =__constant_cpu_to_be16 (ETH_P_AOE );How about
__constant_htons()?
Hi. Andi Kleene and you both ask why we're using the __cpu_to_be16 kind of byte-swappers.
I think it comes down to a semantics thing, and it's probably controversial. We are using fixed-size integers, so we specify the size of the integers. A short happens to be 16-bits wide on most architectures, but it needn't be, esp. in the future, so it's more clear to say what we mean.
Signature · 1 line
Ed L Cashin <[email protected]>
Scott Feldman <[email protected]> writes:
...
Quoted 33 lines
Quoted 7 lines
drivers/block/aoe/aoenet.c +7
-0+static char aoe_iflist[IFLISTSZ ]; + +int +is_aoe_netif (struct net_device *ifp) +{ + registerchar *p, *q; + registerint len;drivers/block/aoe/aoenet.c
Quoted 10 lines
drivers/block/aoe/aoenet.c +10
-0+ +if (aoe_iflist[0 ] =='\0' ) +return 1 ; + +for (p = aoe_iflist; *p; p = q +strspn (q,WHITESPACE )) { + q = p +strcspn (p,WHITESPACE ); +if (q != p) + len = q - p; +else + len =strlen (p);/* last token in aoe_iflist */
Review
Quoted 2 lines
(quoted diff) +2
-0+ + if (strlen(ifp->name) == len && !strncmp(ifp->name, p,len))
drivers/block/aoe/aoenet.c line 53
Quoted 7 lines
drivers/block/aoe/aoenet.c +7
-0+return 1 ; +if (q == p) +break ; + } + +return 0 ; +}This is getting called for every skb received. Do you really want to walk a string array looking for "eth0" or "eth1" for every receive packet? Maybe making the locals "register" helps speed things up. :-)
Quoted 3 lines
Seriously, can you achieve the same by registering the packet type handler for each dev and let
netif_receive_skb()do the check forskb->dev==packet_type->dev?
We'd like to keep state in the driver in order to be able to control both outgoing and incoming traffic. I'm not sure this check will become a performance problem, but anyone with who can measure it is welcome to speak up. I suspect that optimizing this now would be a case of premature optimization.
Quoted 4 lines
Quoted 2 lines
+ * (1) i have no idea if this is redundant, but i can't figure why
+ * the ifp is passed in if it is.This isn't necessary - see
deliver_skb()
Thanks. That looks like the only place our packet handler is called.
Quoted 5 lines
Quoted 1 line
+
skb->len+=ETH_HLEN; /* (2) */You want to do a
skb_push(skb, ETH_HLEN)here instead to keepskb->lenandskb->datacorresponding (basically to undo theskb->pull()ineth_type_trans()).skb->mac.rawwill still be correct.
Thanks.
...
Quoted 5 lines
drivers/block/aoe/aoenet.c
Quoted 5 lines
drivers/block/aoe/aoenet.c +5
-0+ if (h->verfl & AOEFL_ERR) { + n = h->err; + if (n > NECODES) + n = 0; + printk(KERN_CRIT "aoe: aoenet_rcv: error packet from
Quoted 8 lines
%d.%d; "
drivers/block/aoe/aoenet.c line 139
Quoted 5 lines
drivers/block/aoe/aoenet.c +5
-0+"ecode=%d '%s'\n" , +__be16_to_cpu (*((u16 *) h->major)), h->minor, + h->err, aoe_errlist[n]); + goto exit; + }Is there a better way to handle errors than flooding the log?
These errors haven't happened much, but if they did, I'd want everybody to know. Would you suggest a per-device counter to limit how many times this message gets printed?
Quoted 3 lines
Quoted 1 line
+
dev_kfree_skb(skb);
kfree_skb()should be enough.
Hmm. It's just a macro for kfree_skb, but I thought that dev_kfree_skb is the more correct one for us to call. I mean, if dev_kfree_skb isn't supposed to be called here, then what's it for?
Signature · 1 line
Ed L Cashin <[email protected]>
Quoted 6 lines · Ed L Cashin
Quoted 1 line · Scott Feldman
Is there a better way to handle errors than flooding the log?
These errors haven't happened much, but if they did, I'd want everybody to know. Would you suggest a per-device counter to limit how many times this message gets printed?
And a sensible KERN_ level like KERN_WARNING. KERN_CRIT is used when something catastrophic just occurred and the box is as good as defunct. Take a look at "net_ratelimit()"
Alan