Second revision of my ATA Over Ethernet root device patch, now with
white space correction and removed debugging crud.
Any more comment, suggestions?
--
Jason McMullan <[email protected]>
TimeSys Corporation
On Fri, May 13, 2005 at 03:12:07PM -0400, McMullan, Jason wrote:
> Second revision of my ATA Over Ethernet root device patch, now with
> white space correction and removed debugging crud.
>
> Any more comment, suggestions?
I'm guessing you are only testing this out on devfs?
Why not fix this up properly, and allow root devices on _any_ type of
block device that is not immediately present at "try to mount time"? The
USB and firewire users of the world will love you...
Also, please CC the aoe maintainer, that's documented in
Documentation/SubmittingPatches :)
> +config ATA_OVER_ETH_ROOT_SHELF
> + int "Shelf ID"
> + depends on ATA_OVER_ETH_ROOT
Ick. Why not use a boot parameter if you really want to use something
so icky (hint, we should rely on the name or major/minor, not something
else like this.)
> --- linux-orig/drivers/block/aoe/aoeblk.c
> +++ linux/drivers/block/aoe/aoeblk.c
> @@ -229,6 +229,7 @@
> gd->capacity = d->ssize;
> snprintf(gd->disk_name, sizeof gd->disk_name, "etherd/e%ld.%ld",
> d->aoemajor, d->aoeminor);
> + strncpy(gd->devfs_name, gd->disk_name, sizeof gd->devfs_name);
You do know devfs is going away in 2 months, right?
> +#ifdef CONFIG_ATA_OVER_ETH_ROOT
> +void aoe_root(unsigned long major, unsigned long minor)
> +{
> + struct net_device *dev;
> +
> + printk(KERN_INFO
> + "aoe: Waiting for root AOE device e%ld.%ld\n", major, minor);
> +
> + /* Give hardware a chance to settle */
> + msleep(500);
> +
> + rtnl_shlock();
> + /* bring loopback device up first */
> + if (dev_change_flags(&loopback_dev, loopback_dev.flags | IFF_UP) < 0)
> + printk(KERN_ERR "AOE Root: Failed to open %s\n", loopback_dev.name);
> +
> + /* Setup all network devices */
> + for (dev = dev_base; dev ; dev = dev->next) {
> + if (dev == &loopback_dev)
> + continue;
> + dev_change_flags(dev, dev->flags | IFF_UP);
> + }
> + rtnl_shunlock();
> +
> + /* Give drivers a chance to settle */
> + ssleep(1);
> +
> + do {
> + aoecmd_cfg(major, minor);
> + msleep(1);
> + } while (!aoedev_bymajor_minor(CONFIG_ATA_OVER_ETH_ROOT_SHELF,CONFIG_ATA_OVER_ETH_ROOT_SLOT));
Looks like you are mixing up shelf and slot values with major and minor
numbers. I'm confused.
> +
> +}
> +#endif
> +
> static void
> aoe_exit(void)
> {
> @@ -63,6 +106,7 @@
> aoechr_exit();
> aoedev_exit();
> aoeblk_exit(); /* free cache after de-allocating bufs */
> + devfs_remove("etherd");
> }
>
> static int __init
> @@ -70,6 +114,8 @@
> {
> int ret;
>
> + devfs_mk_dir("etherd");
> +
Should be in a separate patch, to fix up devfs issues in the driver,
right? This goes for the other devfs calls in this patch. That is if
you don't mind me removing them in 2 months :)
> ret = aoedev_init();
> if (ret)
> return ret;
> @@ -91,6 +137,9 @@
> printk(KERN_INFO
> "aoe: aoe_init: AoE v2.6-%s initialised.\n",
> VERSION);
> +#ifdef CONFIG_ATA_OVER_ETH_ROOT
> + aoe_root(CONFIG_ATA_OVER_ETH_ROOT_SHELF,CONFIG_ATA_OVER_ETH_ROOT_SLOT);
Again with the major/minor confusion. I'm really confused now (you pass
the values in, yet use them as #defines in the function...)
> +#endif
And you could do this without an ifdef, if you really needed to do so.
So, my main question is, why is this patch needed? Is it because aoe
devices aren't quite present and found quick enough during the boot
process? If so, I suggest one of the two solutions:
- do like the rest of the world does for usb and firewire and
other types of slow boot devices and use an initrd/initramfs
that mounts the root partition after it is properly found.
Distros do this all the time, so there are lots of examples to
pull from if you want to do this for yours.
- fix up the patch that is floating around that allows the
kernel to pause and wait and not oops out if the root
partition is not found. That way all users of all kinds of
slow devices can benefit, and driver specific hacks like this
are not needed.
Andrew, care to drop this from your tree until it gets sorted out?
thanks,
greg k-h
[First off: Andrew, I'm sorry I didn't make it more clear that the AOE
root patch was just for review, not for submission. Please remove it
from -mm]
On Sat, 2005-05-14 at 00:28 -0700, Greg KH wrote:
> I'm guessing you are only testing this out on devfs?
Yes. udev takes 5 minutes to complete on my 8mb, 100Mhz board.
> Why not fix this up properly, and allow root devices on _any_ type of
> block device that is not immediately present at "try to mount time"? The
> USB and firewire users of the world will love you...
Sure. No problem. Where is this patch you speak of?
> Also, please CC the aoe maintainer, that's documented in
> Documentation/SubmittingPatches :)
I did to [email protected], in a separate message.
> > +config ATA_OVER_ETH_ROOT_SHELF
> > + int "Shelf ID"
> > + depends on ATA_OVER_ETH_ROOT
>
> Ick. Why not use a boot parameter if you really want to use something
> so icky (hint, we should rely on the name or major/minor, not something
> else like this.)
Because kernel major/minor change dynamically based upon the number of
AOE blades on your LAN. As setting them in __setup() - sure, no problem,
this patch was just a 15 minute hack job.
> You do know devfs is going away in 2 months, right?
Yes, much to my disappointment. udevd is so frick'n bloaty.
> Looks like you are mixing up shelf and slot values with major and minor
> numbers. I'm confused.
As was I. The original driver uses the words 'major' and 'minor' to
mean *both* kernel major/minor numbers and AOE blade slot/shelf IDs,
depending on context. Took me a bit to wrap my brain around it too.
> Should be in a separate patch, to fix up devfs issues in the driver,
> right? This goes for the other devfs calls in this patch. That is if
> you don't mind me removing them in 2 months :)
Yeah, I know, I was just being lazy and using the uber-fast devfs.
> So, my main question is, why is this patch needed? Is it because aoe
> devices aren't quite present and found quick enough during the boot
> process?
Yep.
> If so, I suggest one of the two solutions:
> - do like the rest of the world does for usb and firewire and
> other types of slow boot devices and use an initrd/initramfs
> that mounts the root partition after it is properly found.
> Distros do this all the time, so there are lots of examples to
> pull from if you want to do this for yours.
I only have 8Mb of RAM. No room for initrd.
> - fix up the patch that is floating around that allows the
> kernel to pause and wait and not oops out if the root
> partition is not found. That way all users of all kinds of
> slow devices can benefit, and driver specific hacks like this
> are not needed.
Again, this happy patch you speak of... Where can I find this wonder?
--
Jason McMullan <[email protected]>
TimeSys Corporation
On Mon, May 16, 2005 at 09:14:02AM -0400, McMullan, Jason wrote:
> [First off: Andrew, I'm sorry I didn't make it more clear that the AOE
> root patch was just for review, not for submission. Please remove it
> from -mm]
>
>
> On Sat, 2005-05-14 at 00:28 -0700, Greg KH wrote:
> > I'm guessing you are only testing this out on devfs?
>
> Yes. udev takes 5 minutes to complete on my 8mb, 100Mhz board.
Ouch, that's not good at all. Care to take this info to the
linux-hotplug-devel mailing list and we can work on fixing this for you?
(first a few things to try, use 'udevsend' and build udev using klibc.
Also do not have any rules that call out to shell scripts, as you are
using devfs I don't see that you need any special rules at all
probably.)
> > Why not fix this up properly, and allow root devices on _any_ type of
> > block device that is not immediately present at "try to mount time"? The
> > USB and firewire users of the world will love you...
>
> Sure. No problem. Where is this patch you speak of?
It can be based on your patch :)
Anyway, it's floating around in the lkml archives, sorry, don't have a
direct link.
> > Also, please CC the aoe maintainer, that's documented in
> > Documentation/SubmittingPatches :)
>
> I did to [email protected], in a separate message.
That's not the email address listed in MAINTAINERS, is it?
> > > +config ATA_OVER_ETH_ROOT_SHELF
> > > + int "Shelf ID"
> > > + depends on ATA_OVER_ETH_ROOT
> >
> > Ick. Why not use a boot parameter if you really want to use something
> > so icky (hint, we should rely on the name or major/minor, not something
> > else like this.)
>
> Because kernel major/minor change dynamically based upon the number of
> AOE blades on your LAN. As setting them in __setup() - sure, no problem,
> this patch was just a 15 minute hack job.
>
>
> > You do know devfs is going away in 2 months, right?
>
> Yes, much to my disappointment. udevd is so frick'n bloaty.
with klibc? Again, take it to linux-hotplug-devel, if we don't get
reports about problems, we assume that everyone is happy with it. I
haven't seen you asking about this there.
> > Should be in a separate patch, to fix up devfs issues in the driver,
> > right? This goes for the other devfs calls in this patch. That is if
> > you don't mind me removing them in 2 months :)
>
> Yeah, I know, I was just being lazy and using the uber-fast devfs.
It's just a mirrage, it's not really that fast :)
> > If so, I suggest one of the two solutions:
> > - do like the rest of the world does for usb and firewire and
> > other types of slow boot devices and use an initrd/initramfs
> > that mounts the root partition after it is properly found.
> > Distros do this all the time, so there are lots of examples to
> > pull from if you want to do this for yours.
>
> I only have 8Mb of RAM. No room for initrd.
Even with ash built with klibc and/or busybox?
> > - fix up the patch that is floating around that allows the
> > kernel to pause and wait and not oops out if the root
> > partition is not found. That way all users of all kinds of
> > slow devices can benefit, and driver specific hacks like this
> > are not needed.
>
> Again, this happy patch you speak of... Where can I find this wonder?
lkml archives. Or base it on your work you have already started to do
:)
thanks,
greg k-h
Greg KH <[email protected]> writes:
> On Mon, May 16, 2005 at 09:14:02AM -0400, McMullan, Jason wrote:
...
[gregkh writes]
>> > Also, please CC the aoe maintainer, that's documented in
>> > Documentation/SubmittingPatches :)
>>
>> I did to [email protected], in a separate message.
>
> That's not the email address listed in MAINTAINERS, is it?
I did get it, though. I didn't realize that the reason for the patch
is for use on systems that have so little RAM that they can't have an
initrd.
It seems like the system will hang forever with no diagnostics if the
root device isn't found. Perhaps an error message would be helpul?
I do wonder whether the problem this patch solves might be a special
case that doesn't merit the extra complexity of a Kconfig knob. It is
certainly a useful patch for some users, but I don't know how many.
--
Ed L Cashin <[email protected]>