ATA over Ethernet driver for 2.6.9
Subsystems (3)
PATCH 1/1 ATA over Ethernet driver for 2.6.9
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
AoE devices on the LAN are accessable as block devices and can be used with filesystems, Software RAID, LVM, etc.
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
This patch is released under the terms of the GPL.
(We also have an AoE driver for the 2.4 kernel that we plan to release soon.)
Provide support for ATA over Ethernet devices
Documentation/aoe.txt +58 -0
@@ -0,0 +1,58 @@ +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 + + Two scripts are in scripts/aoe for assisting in creating device + nodes for using the aoe driver. Usage is as follows. + + rm -rf /dev/etherd + sh scripts/mkdevs /dev/etherd + + ... or to make just one shelf's worth of block device nodes ... + + sh 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".
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 +1 -0
@@ -357,5 +357,6 @@ config LBD bigger than 2TB. Otherwise say N. source "drivers/s390/block/Kconfig" +source "drivers/block/aoe/Kconfig" endmenu
drivers/block/aoe/Kconfig +10 -0
@@ -0,0 +1,10 @@ +# +# ATA over Ethernet configuration +# +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.
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 aoeutils.o
drivers/block/aoe/all.h +156 -0
156 lines not on this page · raw message
drivers/block/aoe/aoeblk.c +194 -0
194 lines not on this page · raw message
drivers/block/aoe/aoechr.c +293 -0
293 lines not on this page · raw message
drivers/block/aoe/aoecmd.c +583 -0
583 lines not on this page · raw message
drivers/block/aoe/aoedev.c +233 -0
233 lines not on this page · raw message
drivers/block/aoe/aoemain.c +84 -0
84 lines not on this page · raw message
drivers/block/aoe/aoenet.c +168 -0
168 lines not on this page · raw message
drivers/block/aoe/aoeutils.c +183 -0
183 lines not on this page · raw message
drivers/block/aoe/if_aoe.h +64 -0
64 lines not on this page · raw message
drivers/block/aoe/u.h +10 -0
@@ -0,0 +1,10 @@ + +/* +typedef unsigned short ushort; +typedef unsigned int uint; +typedef unsigned long ulong; +typedef long long vlong; +typedef unsigned long long uvlong; +*/ +typedef unsigned char uchar ; +
scripts/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 +
scripts/aoe/mkdevs +31 -0
@@ -0,0 +1,31 @@ +#!/bin/sh +# create all the device nodes for using the aoe driver + +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
scripts/aoe/mkshelf +24 -0
@@ -0,0 +1,24 @@ +#! /bin/sh +# create one shelf's worth of device nodes + +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
Signature · 2 lines
-- Ed L Cashin <[email protected]>
On Mon, 2004-12-06 10:51:46 -0500, Ed L Cashin <[email protected]> wrote in message <[email protected]>:
Quoted 7 lines
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
So AoE is out of scope for many uses...
However, some comments:
Quoted 14 lines
diff -urpN linux-2.6.9/drivers/block/Kconfig linux-2.6.9-aoe/drivers/block/Kconfig
drivers/block/Kconfig +1
-0@@ -357,5 +357,6 @@ config LBD bigger than 2TB. Otherwise say N. source "drivers/s390/block/Kconfig" +source "drivers/block/aoe/Kconfig" endmenudiff -urpN linux-2.6.9/drivers/block/aoe/Kconfig linux-2.6.9-aoe/drivers/block/aoe/Kconfig
drivers/block/aoe/Kconfig line 10
Quoted 10 lines
drivers/block/aoe/Kconfig +10
-0@@ -0,0 +1,10 @@ +# +# ATA over Ethernet configuration +# +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.
Since your config only contains a single element, just put it into the block device Kconfig.
Quoted 3 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/all.hlinux-2.6.9-aoe/drivers/block/aoe/all.hdrivers/block/aoe/all.h +0
-0
[...]
Quoted 1 line
+#define nil NULL
It's not April 1st today :)
Quoted 1 line
+#define nelem(A) (sizeof (A) / sizeof (A)[0])
Just use ARRAY_SIZE()
drivers/block/aoe/all.h line 31
Quoted 3 lines
drivers/block/aoe/all.h +3
-0+#define AOE_MAJOR 152 +#define ROOT_PATH "/dev/etherd/" +#define PATHLEN (strlen(ROOT_PATH) + 8)
Looks strangely hardcoded...
[many typedefs deleted]
Typedefs tend to make the code harder to read and understand. Why don't you just use the struct xxx notation?
drivers/block/aoe/aoeblk.c line 130
Quoted 10 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/aoeblk.clinux-2.6.9-aoe/drivers/block/aoe/aoeblk.cdrivers/block/aoe/aoeblk.c +6
-0+static struct block_device_operations aoe_bdops = { + open: aoeblk_open, + release: aoeblk_release, + ioctl: aoeblk_ioctl, + owner:THIS_MODULE , +};
Please use C99 syntax:
.open = aoeblk_open,
etc.
drivers/block/aoe/aoeblk.c line 145
Quoted 4 lines
drivers/block/aoe/aoeblk.c +4
-0+void +aoeblk_gdalloc (void *vp) +{ +if (gd == nil) {
Could be shorter like "if (!gd) {"
Quoted 8 lines
drivers/block/aoe/aoeblk.c +8
-0+void +aoeblk_exit (void ) +{ +unregister_blkdev (AOE_MAJOR ,DEVICE_NAME ); +} + +int +aoeblk_init (void )
drivers/block/aoe/aoeblk.c line 193
Quoted 10 lines
drivers/block/aoe/aoeblk.c +10
-0+{ +int n; + + n =register_blkdev (AOE_MAJOR ,DEVICE_NAME ); +if (n <0 ) { +printk (KERN_ERR "aoe: aoeblk_init: can't register major\n" ); +return n; + } +return 0 ; +}
__exit and __init are missing here.
drivers/block/aoe/aoechr.c line 271
Quoted 10 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/aoechr.clinux-2.6.9-aoe/drivers/block/aoe/aoechr.cdrivers/block/aoe/aoechr.c +7
-0+struct file_operations aoe_fops = { + write: aoechr_write, + read: aoechr_read, + open: aoechr_open, + release: aoechr_rel, + owner:THIS_MODULE , +};
C99
Quoted 53 lines
drivers/block/aoe/aoechr.c +53
-0+u16 +nhget16 (uchar *p) +{ +u16 n; + + n = p[0 ]; + n <<=8 ; +return n |= p[1 ]; +} + +u32 +nhget32 (uchar *p) +{ +u32 n; + + n =nhget16 (p); + n <<=16 ; +return n |=nhget16 (p+2 ); +} + +void +hnput16 (uchar *p,u16 n) +{ + p[1 ] = n; + p[0 ] = n >>=8 ; +} + +void +hnput32 (uchar *p,u32 n) +{ +hnput16 (p+2 , n); +hnput16 (p, n >>=16 ); +} + +u16 +lhget16 (uchar *p) +{ +u16 n; + + n = p[1 ]; + n <<=8 ; +return n |= p[0 ]; +} + +u32 +lhget32 (uchar *p) +{ +u32 n; + + n =lhget16 (p+2 ); + n <<=16 ; +return n |=lhget16 (p); +}
drivers/block/aoe/aoechr.c
Quoted 10 lines
drivers/block/aoe/aoechr.c +10
-0+ +u64 +lhget64 (uchar *p) +{ +u64 n; + + n =lhget32 (p+4 ); + n <<=32 ; +return n |=lhget32 (p); +}
There are function available for this, look at the endianess header files.
After all, especially keeping in mind that AoE isn't routeable, my thinking is that this had better written as a (E)NBD server process running in userspace. This way, you'd use the in-kernel NBD driver (or the ENBD which isn't in the kernel) and you the the routing stuff for free :)
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 1 line · Jan-Benedict Glaw
On Mon, 2004-12-06 10:51:46 -0500, Ed L Cashin <[email protected]>
...
Quoted 4 lines
Quoted 2 lines
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
So AoE is out of scope for many uses...
The usual way of using it is to use an ethernet switch and then put all your AoE storage on that LAN.
You could probably get creative with tunnelling or something, but it's easier to just configure a LAN. If you really need storage over the internet, you'd probably use something else like iSCSI.
...
[helpful style comments, thanks]
...
Quoted 5 lines
After all, especially keeping in mind that AoE isn't routeable, my thinking is that this had better written as a (E)NBD server process running in userspace. This way, you'd use the in-kernel NBD driver (or the ENBD which isn't in the kernel) and you the the routing stuff for free :)
We looked at the NBD stuff, but it's a different model, and it goes over TCP/IP, if I recall correctly. ATA over Ethernet is a simpler, lower-level protocol, saving the host the work of doing TCP/IP.
Signature · 1 line
Ed L Cashin <[email protected]>
Quoted 4 lines · Jan-Benedict Glaw
Quoted 2 lines
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
So AoE is out of scope for many uses...
Take a look at their product range and you'll see the intended uses. For those I'm not sure routability actually matters too much. In addition you'd want to tunnel it on a shared LAN to add crypto.
Quoted 7 lines
drivers/block/aoe/aoeutils.c line 182
Quoted 4 lines
drivers/block/aoe/aoeutils.c +4
-0+ n =lhget32 (p+4 ); + n <<=32 ; +return n |=lhget32 (p); +}There are function available for this, look at the endianess header files.
Ed:cpu_to_le32() and friends to be more exact. These also have the advantage they've been optimised and are asm on some systems.
Quoted 5 lines
After all, especially keeping in mind that AoE isn't routeable, my thinking is that this had better written as a (E)NBD server process running in userspace. This way, you'd use the in-kernel NBD driver (or the ENBD which isn't in the kernel) and you the the routing stuff for free :)
Different problem space IMHO.
Hi!
Quoted 9 lines
Quoted 7 lines
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
So AoE is out of scope for many uses...
Well, at least it has a chance to correctly work in low-memory conditions, and it might be possible to swap over AoE.
ARPs are real problem there.
Pavel
Signature · 1 line
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
Quoted 7 lines · Pavel Machek
Well, at least it has a chance to correctly work in low-memory conditions, and it might be possible to swap over AoE.
ARPs are real problem there.
Pavel
Well, how about caching the hw address of the AoE device somewhere in the data structure describing the block device? Then you won't need to ARP for it, and I guess that address isn't likely to change while the device is in use?
Helge Hafting
Quoted 6 lines · Helge Hafting
Quoted 4 lines · Pavel Machek
Well, at least it has a chance to correctly work in low-memory conditions, and it might be possible to swap over AoE.
ARPs are real problem there.
...
Quoted 4 lines
Well, how about caching the hw address of the AoE device somewhere in the data structure describing the block device? Then you won't need to ARP for it, and I guess that address isn't likely to change while the device is in use?
The ATA over Ethernet protocol doesn't use IP addresses, so there's no need to use ARP. Pavel Machek appears to be noting that because ARP is not needed, AoE may be easier to do in situations where memory is scarce.
Signature · 1 line
Ed L Cashin <[email protected]>
Hi!
Quoted 8 lines
Quoted 5 lines
Well, at least it has a chance to correctly work in low-memory
conditions,
and it might be possible to swap over AoE.
ARPs are real problem there.
> Well, how about caching the hw address of the AoE devicesomewhere in the data structure describing the block device?s Then you won't need to ARP for it, and I guess that address isn't likely to change while the device is in use?
That's what I'm saying... There's chance to swap over ATA over ethernet. It would be very hard to do swap over ATA over IP.
Pavel
Signature · 1 line
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
Quoted 8 lines · Ed L Cashin
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
AoE devices on the LAN are accessable as block devices and can be used with filesystems, Software RAID, LVM, etc.
Quoted 7 lines
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
This patch is released under the terms of the GPL.
(We also have an AoE driver for the 2.4 kernel that we plan to release soon.)
Is there a free server for this?
Quoted 13 lines · Adam Heath
Quoted 11 lines · Ed L Cashin
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
AoE devices on the LAN are accessable as block devices and can be used with filesystems, Software RAID, LVM, etc.
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
Quoted 6 lines
Quoted 4 lines
This patch is released under the terms of the GPL.
(We also have an AoE driver for the 2.4 kernel that we plan to release soon.)
Is there a free server for this?
Are you asking whether anybody has written software that allows a network host to export block storage using AoE? Not yet, as far as I know.
Signature · 1 line
Ed L Cashin <[email protected]>
Quoted 17 lines · Ed L Cashin
Quoted 15 lines · Adam Heath
Quoted 13 lines · Ed L Cashin
The included patch allows the Linux kernel to use the ATA over Ethernet (AoE) network protocol to communicate with any block device that handles the AoE protocol. The Coraid EtherDrive (R) Storage Blade is the first hardware using AoE.
AoE devices on the LAN are accessable as block devices and can be used with filesystems, Software RAID, LVM, etc.
Like IP, AoE is an ethernet-level network protocol, registered with the IEEE. Unlike IP, AoE is not routable.
This patch is released under the terms of the GPL.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
(We also have an AoE driver for the 2.4 kernel that we plan to release soon.)
Is there a free server for this?
Are you asking whether anybody has written software that allows a network host to export block storage using AoE? Not yet, as far as I know.
Yup, that's what I was asking. Bummer.
Quoted 8 lines · Ed L Cashin
Quoted 4 lines
Quoted 2 lines
(We also have an AoE driver for the 2.4 kernel that we plan to release soon.)
Is there a free server for this?
Are you asking whether anybody has written software that allows a network host to export block storage using AoE? Not yet, as far as I know.
I wrote a very bad one for the original protocol draft when playing with this stuff some time ago. I'm not sure its actually worth ressurecting. It's pretty trivial to write but ATAoE is rather more interesting in the context of microcontroller on ethernet than the PC side.
Documentation/aoe.txt line 13
Quoted 7 lines · Ed L Cashin
Documentation/aoe.txt +7
-0+CREATING DEVICE NODES + + Two scripts are in scripts/aoe for assisting in creating device + nodes for using the aoe driver. Usage is as follows. + + rm -rf /dev/etherd + sh scripts/mkdevs /dev/etherd
If you use the /sys/class interface properly, and udev, you don't need this script at all. Care to add sysfs support to the driver so that people don't have to rely on this?
Quoted 7 lines
Documentation/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.txt line 39
Quoted 10 lines
Documentation/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
Why not just put this info in the /sys/class/block/eX.X files (with a symlink to the ethX device for the "device" symlink, and the status as another single file?
Quoted 1 line
+typedef struct Buf Buf;
This driver should not create _any_ new typedefs, please fix this.
drivers/block/aoe/all.h line 70
Quoted 4 lines
drivers/block/aoe/all.h +4
-0+typedef struct Bufq Bufq ; +struct Bufq { +Buf *head, *tail; +};
What's wrong with the in-kernel list structures that you need to create your own?
drivers/block/aoe/all.h line 156
Quoted 3 lines
drivers/block/aoe/all.h +3
-0+enum {FQUOTE = (1 <<0 ),FEMPTY = (1 <<1 ) }; +int getfields (char *,char **,int ,char *,int ); +#define tokenize (A ,B ,C ) getfields(A, B, C, " \t\r\n", FQUOTE)
"getfields" is a pretty "generic" symbol name that you are adding to the global symbol table. Are you sure that there's nothing already in the kernel tree for this?
Quoted 7 lines
drivers/block/aoe/aoeblk.c +7
-0+static int +aoeblk_release (struct inode *inode,struct file *filp) +{ +Aoedev *d; +ulong flags; + + d = (Aoedev *) inode->i_bdev->bd_disk->private_data;
drivers/block/aoe/aoeblk.c line 41
Quoted 10 lines
drivers/block/aoe/aoeblk.c +10
-0+ +spin_lock_irqsave (&d->lock, flags); +if (--d->nopen ==0 ) +if ((d->flags &DEVFL_CLOSEWAIT )) { + d->flags &= ~DEVFL_CLOSEWAIT ; +spin_unlock_irqrestore (&d->lock, flags); +aoecmd_cfg (d->aoemajor, d->aoeminor); +return 0 ; + } +spin_unlock_irqrestore (&d->lock, flags);
Looks like you didn't indent properly for the two if() statments.
Quoted 11 lines
drivers/block/aoe/aoeblk.c +11
-0+static int +aoeblk_ioctl (struct inode *inode,struct file *filp,uint cmd,ulong arg) +{ +Aoedev *d; + +if (!arg) +return -EINVAL ; + + d = (Aoedev *) inode->i_bdev->bd_disk->private_data; +if ((d->flags &DEVFL_UP ) ==0 ) { +printk (KERN_ERR "aoe: aoeblk_ioctl: disk not up\n" );
drivers/block/aoe/aoeblk.c line 115
Quoted 10 lines
drivers/block/aoe/aoeblk.c +10
-0+return -ENODEV ; + } + +if (cmd ==HDIO_GETGEO ) { + d->geo.start =get_start_sect (inode->i_bdev); +if (!copy_to_user ((void *) arg, &d->geo,sizeof d->geo)) +return 0 ; +return -EFAULT ; + } +printk (KERN_INFO "aoe: aoeblk_ioctl: unknown ioctl %d\n" , cmd);
So I can flood the syslog by sending improper ioctls to the driver? That's not nice...
drivers/block/aoe/aoeblk.c line 121
Quoted 5 lines
drivers/block/aoe/aoeblk.c +5
-0+/* We should return -ENOTTY for unrecognized ioctls later +* when everyone's running kernels that support it. See, +* e.g., BLKFLSBUF in ioctl.c:blkdev_ioctl. +*/ +return -EINVAL ;
Is this, 2.6.10-rc2 kernel sufficiently "later"? :)
drivers/block/aoe/aoechr.c line 51
Quoted 8 lines
drivers/block/aoe/aoechr.c +8
-0+static int +interfaces_cmd (int argc,char **argv) +{ +if (set_aoe_iflist (argc, argv)) { +printk (KERN_CRIT +"%s: could not set inteface list: %s\n" , + __FUNCTION__,"too many interfaces" ); +return -1 ;
-EINVAL?
drivers/block/aoe/aoechr.c line 150
Quoted 7 lines
drivers/block/aoe/aoechr.c +7
-0+ssize_t +aoechr_write (struct file *filp,const char *buf,size_t cnt,loff_t *offp) +{ +char *argv[NARGS ]; +char *str =kallocz (cnt+1 ,GFP_KERNEL ); +int argc; +int ret =-1 ;
-1 isn't a good "default" error return value, try picking the correct values.
drivers/block/aoe/aoechr.c line 154
Quoted 4 lines
drivers/block/aoe/aoechr.c +4
-0+ +if (!str) { +printk (KERN_CRIT "aoe: aoechr_write: cannot allocate memory\n" ); +return ret;
-ENOMEM is better.
drivers/block/aoe/aoechr.c line 159
Quoted 5 lines
drivers/block/aoe/aoechr.c +5
-0+ } + +if (copy_from_user (str, buf, cnt)) { +printk (KERN_INFO "aoe: aoechr_write: copy from user failed\n" ); + goto out;
-EFAULT is the proper return value for this error.
Quoted 5 lines
drivers/block/aoe/aoechr.c +5
-0+int +aoechr_open (struct inode *inode,struct file *filp) +{ +int n; +
drivers/block/aoe/aoechr.c line 193
Quoted 10 lines
drivers/block/aoe/aoechr.c +10
-0+ n =MINOR (inode->i_rdev); + filp->private_data = (void *) (unsigned long ) n; + +switch (n) { +case MINOR_CTL : +case MINOR_ERR : +case MINOR_STAT : +return 0 ; + } +return -1 ;
-EINVAL?
Oh, you have a lot of global functions that can be made static (like this one. Please make them static.
drivers/block/aoe/aoeutils.c line 3
Quoted 7 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/aoeutils.clinux-2.6.9-aoe/drivers/block/aoe/aoeutils.cdrivers/block/aoe/aoeutils.c +3
-0@@ -0,0 +1,183 @@ +/* +* utils.c +* utility functions that have no place in other modules.
I think this whole file can be deleted and the driver converted to use the functions already present in the kernel. Please don't reinvent the wheel...
drivers/block/aoe/aoeutils.c line 118
Quoted 10 lines
drivers/block/aoe/aoeutils.c +10
-0+void * +kallocz (ulong sz,ulong kmem) +{ +void *d; + + d =kmalloc (sz, kmem); +if (d) +memset (d,0 , sz); +return d; +}
What's wrong kcalloc() instead of creating your own?
drivers/block/aoe/u.h line 9
Quoted 13 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/u.hlinux-2.6.9-aoe/drivers/block/aoe/u.hdrivers/block/aoe/u.h +9
-0@@ -0,0 +1,10 @@ + +/* +typedef unsigned short ushort; +typedef unsigned int uint; +typedef unsigned long ulong; +typedef long long vlong; +typedef unsigned long long uvlong; +*/ +typedef unsigned char uchar ;
These are not needed at all, use the proper kernel versions (u32, u16, etc.)
scripts/aoe/autoload
Quoted 3 lines
diff -urpN linux-2.6.9/scripts/aoe/autoload linux-2.6.9-aoe/scripts/aoe/autoload
scripts/aoe/autoload +0
-0
Shouldn't this go into Documentation/aoe/ instead? I thought the scripts directory was for scripts needed when building the kernel source.
thanks,
greg k-h
Greg KH <[email protected]> writes:
...
Quoted 4 lines
drivers/block/aoe/aoeblk.c line 115
Quoted 1 line
drivers/block/aoe/aoeblk.c +1
-0+printk (KERN_INFO "aoe: aoeblk_ioctl: unknown ioctl %d\n" , cmd);So I can flood the syslog by sending improper ioctls to the driver? That's not nice...
Wouldn't root be the only user who could do that? When would this happen?
If it's happening by accident, then something's wrong, and it's helpful to make it known that something's wrong. If it's on purpose, then somebody has root and is doing malicious things, in which case syslog flooding is the least of our worries. They could do the same thing using "logger" anyway.
Signature · 1 line
Ed L Cashin <[email protected]>
Quoted 10 lines · Ed L Cashin
Greg KH <[email protected]> writes:
...
Quoted 4 lines
drivers/block/aoe/aoeblk.c line 115
Quoted 1 line
drivers/block/aoe/aoeblk.c +1
-0+printk (KERN_INFO "aoe: aoeblk_ioctl: unknown ioctl %d\n" , cmd);So I can flood the syslog by sending improper ioctls to the driver? That's not nice...
Wouldn't root be the only user who could do that? When would this happen?
Your code is not preventing anyone from calling it. So any user could.
Now if you are relying on the dev node from having the proper permissions, that's ok too, but you might want to put a comment in stating that.
If you aren't relying on the device node to have the proper permissions, then you need to do a check on capable() here.
thanks,
greg k-h
Greg KH <[email protected]> writes:
...
Quoted 7 lines
drivers/block/aoe/all.h line 70
Quoted 4 lines
drivers/block/aoe/all.h +4
-0+typedef struct Bufq Bufq ; +struct Bufq { +Buf *head, *tail; +};What's wrong with the in-kernel list structures that you need to create your own?
The Buf structures are singly linked (one next pointer). We could convert it to use list.h, but that would mean having another pointer per list element.
Signature · 1 line
Ed L Cashin <[email protected]>
Quoted 11 lines · Ed L Cashin
Greg KH <[email protected]> writes:
...
Quoted 7 lines
drivers/block/aoe/all.h line 70
Quoted 4 lines
drivers/block/aoe/all.h +4
-0+typedef struct Bufq Bufq ; +struct Bufq { +Buf *head, *tail; +};What's wrong with the in-kernel list structures that you need to create your own?
Quoted 3 lines
The Buf structures are singly linked (one next pointer). We could convert it to use
list.h, but that would mean having another pointer per list element.
Is the extra overhead of another pointer worth the effort of ensuring that your list handling code is correct in all cases? (hint, the answer is usually no...)
The in-kernel list code is very well debugged, and does lots of extra cpu lookup goodness if it can, ensuring that it is as fast as it possibly can be. It's also much better to rely on in-kernel datastructures as programmers can instantly recognize that you are using that, and not have to worry about exactly how you call your own custom list code, and if your list code is even correct in the first place.
So in short, unless this is a structure that is in dire need of a memory savings, I really suggest you use the in-kernel stuff.
thanks,
greg k-h
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 15 lines · Ed L Cashin
Quoted 12 lines · Greg KH
Documentation/aoe.txt line 13
Quoted 7 lines · Ed L Cashin
Documentation/aoe.txt +7
-0+CREATING DEVICE NODES + + Two scripts are in scripts/aoe for assisting in creating device + nodes for using the aoe driver. Usage is as follows. + + rm -rf /dev/etherd + sh scripts/mkdevs /dev/etherdIf you use the
/sys/classinterface properly, and udev, you don't need this script at all. Care to add sysfs support to the driver so that people don't have to rely on this?
Quoted 8 lines
Right now
genhd.cseems to be giving us some automatic sysfs support for the block devices:root@makki root# ls
/sys/block/etherd!e15.3 etherd!e5.2 etherd!e5.6 fd0 md0 ram11 ram15 ram5 ram9 etherd!e3.0 etherd!e5.3 etherd!e5.7 hda ram0 ram12 ram2 ram6 etherd!e5.0 etherd!e5.4 etherd!e5.8 hdb ram1 ram13 ram3 ram7 etherd!e5.1 etherd!e5.5 etherd!e5.9 hdc ram10 ram14 ram4 ram8
Yes, that is automatically handled for you by the block layer.
Quoted 3 lines
Will using
class_simple_device_addregister our char devices so that udev can create the device nodes dynamically? I'm looking at the example innetlink_dev.c.
You need the class_simple* stuff for your char driver. This is because the char layer does not automatically handle this for you, and is the responsibility of every individual driver to implement (yeah, it sucks at times, sorry...)
Hope this helps,
greg k-h
Quoted 2 lines
+#include "
u.h"
can you give this header a more human friendly name ?
drivers/block/aoe/all.h line 27
Quoted 4 lines
drivers/block/aoe/all.h +4
-0+#include "if_aoe.h" + +#define VER 2 +#define nil NULL
please just use NULL
drivers/block/aoe/all.h line 30
Quoted 3 lines
drivers/block/aoe/all.h +3
-0+#define nelem (A ) (sizeof (A) / sizeof (A)[0]) +#define AOE_MAJOR 152 +#define ROOT_PATH "/dev/etherd/"
ehhhh device drivers really should be agnostic to device node paths!
drivers/block/aoe/all.h line 70
Quoted 4 lines
drivers/block/aoe/all.h +4
-0+typedef struct Bufq Bufq ; +struct Bufq { +Buf *head, *tail; +};
is there a good reason to not use list.h lists ??
drivers/block/aoe/all.h line 90
Quoted 7 lines
drivers/block/aoe/all.h +7
-0+struct Aoedev { +Aoedev *next; +uchar addr[6 ];/* remote mac addr */ +ushort flags; +ulong sysminor; +ulong aoemajor; +ulong aoeminor;
sounds like the wrong type, why not use dev_t ?
drivers/block/aoe/all.h line 91
Quoted 1 line
drivers/block/aoe/all.h +1
-0+ulong nopen;/* user count */
why do you need this ?
drivers/block/aoe/aoeblk.c line 34
Quoted 10 lines
drivers/block/aoe/aoeblk.c +10
-0+static int +aoeblk_release (struct inode *inode,struct file *filp) +{ +Aoedev *d; +ulong flags; + + d = (Aoedev *) inode->i_bdev->bd_disk->private_data; + +spin_lock_irqsave (&d->lock, flags); +if (--d->nopen ==0 )
eh why not just a ->release function instead that uses the blocklayer refcounting instead of doing your own ?
drivers/block/aoe/aoeblk.c line 56
Quoted 11 lines
drivers/block/aoe/aoeblk.c +11
-0+int +aoeblk_make_request (request_queue_t *q,struct bio *bio) +{ +Aoedev *d; +Buf *buf; +struct sk_buff *sl; +ulong flags; + +blk_queue_bounce (q, &bio); + + buf =kallocz (sizeof *buf,GFP_KERNEL );
this is deadlocky; you HAVE to use a mempool for allocations here!
Arjan van de Ven <[email protected]> writes:
[helpful suggestions]
...
Quoted 9 lines
drivers/block/aoe/all.h line 90
Quoted 7 lines
drivers/block/aoe/all.h +7
-0+struct Aoedev { +Aoedev *next; +uchar addr[6 ];/* remote mac addr */ +ushort flags; +ulong sysminor; +ulong aoemajor; +ulong aoeminor;sounds like the wrong type, why not use
dev_t?
These are ATA over Ethernet major and minor addresses, not device node major and minor numbers.
Quoted 4 lines
drivers/block/aoe/all.h line 91
Quoted 1 line
drivers/block/aoe/all.h +1
-0+ulong nopen;/* user count */ why do you need this ?
Quoted 13 lines
drivers/block/aoe/aoeblk.c line 34
Quoted 10 lines
drivers/block/aoe/aoeblk.c +10
-0+static int +aoeblk_release (struct inode *inode,struct file *filp) +{ +Aoedev *d; +ulong flags; + + d = (Aoedev *) inode->i_bdev->bd_disk->private_data; + +spin_lock_irqsave (&d->lock, flags); +if (--d->nopen ==0 )eh why not just a ->release function instead that uses the blocklayer refcounting instead of doing your own ?
Do you just mean we should use inode->i_bdev->bd_openers instead of having d->nopen?
Quoted 5 lines
Quoted 5 lines
drivers/block/aoe/aoeblk.c +5
-0+int +aoeblk_make_request (request_queue_t *q,struct bio *bio) +{ +Aoedev *d; +Buf *buf;
Quoted 8 lines
drivers/block/aoe/aoeblk.c line 56
Quoted 6 lines
drivers/block/aoe/aoeblk.c +6
-0+struct sk_buff *sl; +ulong flags; + +blk_queue_bounce (q, &bio); + + buf =kallocz (sizeof *buf,GFP_KERNEL );this is deadlocky; you HAVE to use a mempool for allocations here!
OK. Thanks for pointing that out.
Signature · 1 line
Ed L Cashin <[email protected]>
Hi Ed,
Few comments below.
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500
Quoted 3 lines · Ed L Cashin
diff -urpN
linux-2.6.9/drivers/block/aoe/all.hlinux-2.6.9-aoe/drivers/block/aoe/all.hdrivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +0
-0
How about calling this aoe.h or similar?
Quoted 13 lines
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +12
-0@@ -0,0 +1,156 @@ +#include <linux/ctype.h> +#include <linux/string.h> +#include <linux/hdreg.h> +#include <linux/blkdev.h> +#include <linux/types.h> +#include <linux/fs.h> +#include <linux/module.h> +#include <linux/blkpg.h> +#include <linux/slab.h> +#include <linux/skbuff.h> +#include <linux/ioctl.h> +#include <linux/if.h>
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 line 22
Quoted 10 lines
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +10
-0+#include <linux/netdevice.h> +#include <linux/kdev_t.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/poll.h> +#include <linux/timer.h> +#include <linux/genhd.h> +#include <asm/uaccess.h> +#include <asm/namei.h> +#include <asm/semaphore.h>
This looks wrong. I don't see you using, for example, semaphores everywhere you include this header. Please just move these includes to the appropriate .c files.
Quoted 4 lines
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +4
-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 */
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 line 36
Quoted 10 lines
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +10
-0+ 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, + + MAXATADATA = 1024, + NPERSHELF = 10, + FREETAG = -1, +};
Perhaps it would be cleaner if you kept DEVFL flags in separate enum block as they're related and have nothing to do with the rest?
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 line 38
Quoted 2 lines
drivers/block/aoe/all.h 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/all.h 1969-12-31 19:00:00.000000000 -0500 +2
-0+typedef struct Buf Buf; +struct Buf {
Please don't introduce typedefs for structs. Furthermore, please make the struct name start with a lower case letter like the rest of the kernel.
drivers/block/aoe/aoeblk.c 2004-12-06 10:40:00.000000000 -0500
Quoted 10 lines
diff -urpN
linux-2.6.9/drivers/block/aoe/aoeblk.clinux-2.6.9-aoe/drivers/block/aoe/aoeblk.cdrivers/block/aoe/aoeblk.c 2004-12-06 10:40:00.000000000 -0500 renamed from drivers/block/aoe/aoeblk.c 1969-12-31 19:00:00.000000000 -0500 +7
-0+static int +aoeblk_open(struct inode *inode, struct file *filp) +{ + Aoedev *d; + ulong flags; + + d = (Aoedev *) inode->i_bdev->bd_disk->private_data;
Please remove the redundant casts when converting from a void pointer. The conversion is guaranteed by the C standard.
Pekka