Why /dev/sdc1 doesn't show up...

Because module loading of any incestious, cross-locking modules is horked :-P

NOTE: I suspect the same bug applies to IDE devices as well, but you wouldn't see it unless you compile your IDE drivers as modules and use initrd or equivelant to load the modules.

Longer answer:

Device scans happen almost exclusively at either host module init time or device module init time. At that point in time, either the host the device is on or the high level driver accessing the device will still be in it's init_module() routine. That, of course, implies that either host->hostt->module->live is 0 or that *_template->module->live is 0 (and consequently so is fops->owner->live == 0). As a result, when you register sdc with the driverfs code, it then tries to fops->open(sdc) to read the partition table and then automatically register subdevices. This fails if you are currently loading sd_mod because fops->owner->live == 0. This fails if you are loading your low level driver because the replacement for __MOD_INC_USE_COUNT(sdev->host->hostt->module); in sd_open() is to instead use try_module_get(sdev->host->hostt->module) and with the low level driver still in it's init_module() routine, this fails.

For hot plug events when the module is already live, things work fine. However, that doesn't help hot plug drivers at boot up because when they register their hot plug table they immediately get called for the devices that are already present, and in those cases they will have the exact same problem that we have with non hot plug drivers.

Hrmph...

Working on a fix. Haven't decided how to do it yet. Something as ugly as adding:

driver_template.module->live = 1;
scsi_register_host(&driver_template);
driver_template.module->live = 0;

in scsi_module.c works, but is too ugly to live (and totally defeats the purpose of the new module loading code anyway). Oh, and all the high level drivers would have to do the same thing in their module init routines in order to make things work properly when the lldd is loaded before the high level driver.

I could make all the scsi drivers delay bus scans (via a work queue entry that we don't wait for completion on, but I haven't figured out how to do that without leaking mem yet, unless I write a special SCSI worker thread that kfree()'s the work structs on completion...) until after their init routines have finished (and do the same for the high level scsi drivers), but that's quite a pain in the ass and doesn't fix IDE. A generic fix would be preferable.

Suggestions Rusty?

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

There is a purpose? Seriously, "no use of ones object during init" is WRONG. Rusty, remember I've told you that block devices need to be able to open() during init? That's what it is.

We _might_ eventually kludge around that, but IMO the ->live checks on the init side are just plain wrong.

I tend to agree, and I almost wrote that in my email, but then decided I hadn't thought on the issue long enough to declare that and then be proven wrong and look like a fool.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

PS: logics in case of block devices is very simple: by the time when you can handle open and IO on a disk, you are already set up - no point bailing out after that point; if other disks served by the same driver do not work, it means that they should be left out, not that driver should fail init. IOW, logics of init, regardless of modular/nonmodular makes that a point where you are past failure exits.

And I'd rather see similar rules for other interfaces. Yes, it might require API changes - e.g. old block API was badly b0rken in that respect since register_blkdev() allowed open() attempts, no matter whether you are ready or not, so block drivers used to be seriously racy in that area.

Right way is "don't export an object until you are ready to serve it" and once your API is of that sort, you are pretty much done. Note that non-modular case _also_ need that sort of logics, simply because we might get userland up and running before the initcalls are done.

I encountered similar phenomena with usb-storage.
I think the proper solution is to never automatically scan for a partition table.
We perhaps need to do that at boot time, but in all other cases user space can ask the kernel to read a partition table. For usb-storage things work fairly well (for some kernels) using

	blockdev --rereadpt /dev/sdx

Andries

Scanning for a partition table is just one aspect. In general, once a module has handed over it's entry points to some other kernel area those entry points should be ready for use, so blocking access to them isn't needed. OTOH, when you are tearing a module down then you do want access to stop, so that's the proper time to set live == 0.

(When it works, which it currently doesn't for me in latest bk)

Yeah, that's fine for hotplug. But, you know what, for PCI hotplug scsi this wouldn't even be an issue since a true hot plug event normally would be after module init is complete and scanning the table wouldn't be a problem then because module->live would be non-0 ;-)

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

In message <[email protected]> you write:

Yes, it's an interesting corner case. As I implied before, if a module is register an interface, you do not need to try_module_get(). Why? Because *someone* already has a reference by definition (either the module itself is calling you, or someone else with whom the module registered).

Unfortunately, this does complicate code slightly if you other paths which *do* need the try_module_get(). But the existence of this interface is no accident: I know Al Viro wants to split every register interface to "reserve" and "use", and make all modules use them correctly, but I chose to avoid forcing such changes on all module authors and all interface authors for good reason.

[ Mainly to Al ]:

I know what a PITA it is, because I implemented it a year ago. So first the interface had to be changed to two-stage init, *then* the module had to be changed to use it. If a module uses 5 interfaces, that's a series of five patches, each one interlocked with the corresponding interface. And implementing it showed me that authors were unlikely to correctly use any interface more complex than the current one 8(

And it *still* means you need two paths for your code: one for "I know it's not actually "active" yet, but I'm doing init and I need to access it anyway". So every interface gains significant complexity by effectively implementing their own "live" flag...

Hope that helps,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

No. I want to get the things into "don't bloody export it until you can handle it" land.

Not really. For case in question (block devices) there is only one path and I'd rather keep it that way, thank you very much.

Again, by the time when add_disk() got to reading partition table, device is _there_. That's it - we had set it up completely, it's ready for IO, whatever. At that point we want generic code to do some work with that device. And there is no magic path for that - it's normal open/read/close.

There is no "live" flag - you had shown it, you'd better be ready to have it used. Doesn't cause any problems.

It's simpler than old interface - there you indeed had to pull off all sorts of tricks. Current rule is trivial - "nobody touches it until they see it; register when you are 100% ready to deal with users".

And no, I don't buy arguments about poor interface-writers. You do some infrastructure intended to be used by driver-writers - you are supposed to have a clue. 'Cause having rabid monkeys on crack on *both* sides of an interface is a recipe for disaster and on the driver side you are guaranteed to have a bunch of them.

We need to have interfaces cleaned up. No silver bullets there. There's maybe a dozen of interfaces that cover 99% of all drivers out there. Remaining 1% can and should fend for itself - you do something really tricky, you are responsible for getting it right.

And yes, we need clued people for that cleanup. Forget the modules; if use of an API requires magic, it will lead to fuckups. Lots of them. If API doesn't make sense for users of that API - ditto.

Papering over the races in driver init is OK, as long as it doesn't hurt drivers with clean init. Your logics with ->live on init side *does* hurt such drivers - it forces them to do magic just to neutralize your band-aids.

In message <[email protected]> you write:

See other posting. This is a fundamental design decision, and it's not changing. Sorry.

Unless the module does something else afterwards which fails and wants to fail the init. You're saying "don't do that", which is not a good answer 8(

You can implement a "make_module_live()" in module.h if you want module authors to do two-stage init manually (and trust them to get it right). Or you can run a notifier on "enlivening" a module: I'd hoped to avoid that.

Hope that helps,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

So is the block layer one, and it was fundamentally designed first 8)

Rusty, you take that approach, and the module code flies out of the kernel.

The block device code (and a lot of other code) has been there happily forever. And not breaking drivers was part of the module loader rule. Now you seem to say that drivers should be broken een if they are perfectly fine and do not have any races as is.

If so, then bye bye new module loader.

Linus

In message <[email protected]> you write:

Linus,

Sorry, you're right of course. I should at least have done
something like the "unsafe" wedge which was used for the other interfaces. Under the circumstances, I agree with your fix.

Linus, I would like an answer: how does one register two /proc
entries? The following has a race when modular:

	if (!create_proc_entry("foo"...))
		goto fail;
	if (!create_proc_entry("bar"...))
		goto fail_free_foo;
	return 0;

	I sympathize with Al's "we have worse problems, just BUG()

when it happens" approach, but I don't agree with Doug's "that's clearly broken code!" assertion. Al wants to split such code into:

	if (!reserve_proc_entry("foo"...))
		goto fail;
	if (!reserve_proc_entry("bar"...))
		goto fail_free_foo;

	/* Now we're safe! */
	activate_proc_entry("foo");
	activate_proc_entry("bar");
	return 0;

Note that this code change is *only* required because the code is modular. The code has works perfectly fine in the kernel, and has for years, and very few people have ever noticed any problem with it.

Now, let's imagine one simple implementation of reserve_proc_entry and
activate_proc_entry: a proc entry gets a flag (call it "live") which it can use to tell the difference between reserved and activated entries, and it ignores !live ones.

Now, if you substitute the "live" flag in the proc entry for a "live" flag in the proc->owner entry, you have the previous module code, except:

1) You don't need to change all the modules to do two-stage init.
2) You don't need to change all the interfaces to two-stage init.
3) You *do* have problems with complex interfaces which need to do something more than just flip the flag to make it "live".

Now do you see why I like this scheme? For most interfaces, it "just works": I shall endevour to fix the remaining cases in a way which is no less clear than current code, and if I fail, hey, we leave your change in and we're no worse than 2.4.

Fair enough. Frankly, I just wanted to neaten the module code to allow other improvements without breaking userspace every time. But I felt it wrong to re-implement the same module races: let this be a valuable lesson to me not to bite off more than I can chew 8(

Thanks for your patience,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

I think the answer is simple: you don't.

In practice this doesn't happen. And even in theory when it does happen, trying to do it right is _worse_ than just letting it be. So what if one of them fails? Big deal. You didn't get the file, because you are playing with modules that do something that they shouldn't have done.

Rule #1: KISS

Rule #2: "don't overdesign" aka "perfect is the enemy of the good"

In other words, screw the cases that don't matter, we only need to verify that we don't oops. Which means that the _right_ approach for proc entries is something like this:

	if (register_device(....))
		return -ENOMEM;

	if (!create_proc_entry("foo"...))
		printk("Unable to create '/proc/foo'\n");
	if (!create_proc_entry("bar"...))
		printk("Unable to create '/proc/bar'\n");
	return 0;

and simply face the fact that trying to fail the module load at a late time is a bad idea, and if somebody already has registered your /proc entries then the system maintainer is doing something wrong and it is _his_ problem and he can

The kernel shouldn't oops. And if that means that we should just leave the module loaded (not return an error), then so what? Hey, it may work fine without the /proc entries, erroring out might be _worse_.

This is what happens with built-in drivers already. Modules are nothing but a convenience. They're not "worthy" of complexity.

Linus

In message <[email protected]> you wr ite:

OK. Two devices? A device and a sysfs attribute? A device and a filesystem? A device and a setsockopt? So I sympathise, but I disagree here.

Agreed. Having an invisible "MUST NOT fail beyond this point" line is leaving a subtle trap for driver writers.

We *already have* a mechanism to isolate a module: we did it to avoid a two-stage module destroy policy. We can use the exact same mechanism to avoid such a two-stage module init policy.

Modulo bugs, I've erred on the side of not breaking the 1500+ modules in the kernel, and *not* exposing the unload races to them. Please consider carefully.

Note 1: Al mentioned he wants to fire off userspace before initcalls are done, introducing the same races in core kernel init. Without seeing reasons, it's hard to tell why he wants this, but this would require everyone to do two-stage init anyway.

Note 2: If you really want two-stage init, you're best off making it
explicit: ie. two functions: one which can fail (int module_prep()), and one which can't (void module_start()). I regarded this as too invasive a change just for an obscure module race.

Hope that clarifies?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Then you'll have to back out the patch to module.c because it's already changed.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

In message <[email protected]> you write:

you

Yeah, I just noticed. To be honest, I was wrong. And the code shouldn't be put back until (if ever) I have a solution which solves the races and *doesn't* break working code.

And meanwhile, there are more important things (like reducing the 400k overhead of CONFIG_KALLSYMS adds to the kernel).

Sorry for the overzealousness,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Hi,

Rusty, you know how refcounts work? Especially the "if (atomic_dec_and_test(&obj->ref)) cleanup(obj)" part is really interesting.

bye, Roman