Skip to content

Conversation

@jsmeix
Copy link
Member

@jsmeix jsmeix commented Jul 11, 2016

Tries to fix 0ec8716
by changing 40-start-udev-or-load-modules.sh
so taht the code that always loads modules
in /etc/modules is always run.

@jsmeix jsmeix added the bug The code does not do what it is meant to do label Jul 11, 2016
@jsmeix jsmeix added this to the Rear v1.19 milestone Jul 11, 2016
@jsmeix jsmeix self-assigned this Jul 11, 2016
jsmeix referenced this pull request Jul 11, 2016
…ted, so that

we always load the modules listed in /etc/modules (set by MODULES_LOAD)
Issue #905
@jsmeix
Copy link
Member Author

jsmeix commented Jul 11, 2016

Now it works well for me.

In particulat I verified with the additional "debug" kernel
command line parameter for the rear recovery system that
now 40-start-udev-or-load-modules.sh runs the code
that loads all modules in /etc/moidules.

I just merge it because I think it is now at least
somewhat better than before (even if it is perhaps
not yet fully perfect).

@jsmeix jsmeix merged commit 1e870cd into rear:master Jul 11, 2016
@jsmeix jsmeix deleted the always_load_modules_in_etc_modules_issue905_and_others branch July 12, 2016 12:25
@jsmeix
Copy link
Member Author

jsmeix commented Jul 12, 2016

There is a small thing left where I don't know
how it is actually meant to work:

At the end of 40-start-udev-or-load-modules.sh
I left the "modprobe -q dm-mod" call as is
which means this modprobe call is now also always
done because it does no longer simply "return" in
the "systemd-udevd case" but now it runs through
to the end of the whole script:

RESCUE f121:~ # lsmod | grep dm
dm_mod                110780  0 

I only assume this modprobe call was also meant
to be always done but I don't know.

@jsmeix
Copy link
Member Author

jsmeix commented Jul 13, 2016

git forensic via

git log -p usr/share/rear/skel/default/etc/scripts/system-setup.d/40-start-udev-or-load-modules.sh

shows that the unconditioned "modprobe -q dm-mod" call is there
at the very end since 40-start-udev-or-load-modules.sh
was created (i.e. before the "systemd-udevd case" was
added on top of it) which indicates that the "modprobe -q dm-mod"
call is really meant to be done in any case (I assume when
the "systemd-udevd case" was added on top of it it was
forgotten to deal with the "modprobe -q dm-mod" call).

FYI:

git blame -w usr/share/rear/skel/default/etc/scripts/system-setup.d/40-start-udev-or-load-modules.sh

shows it was this git commit: 844d50b

@jsmeix
Copy link
Member Author

jsmeix commented Jun 20, 2017

@schlomo
many thanks for your question!

I think now I understand the reason for my confusion in
#626 (comment)
(excerpt)

In gereral I noticed at that time that in the rear recovery system
I had more than 100 kernel modules loaded while in my original
system I had something about 40 kernel modules loaded.
From my point of view this indicates that there is something
wrong in general in the rear recovery system that it has much
more kernel modules loaded than in the original system...

I don't know a mandatory reason why the explicitly specified modules
must be loaded after the udev automatism.
My change was only meant to get the explicitly specified modules
loaded at all.
I did not intend to change the odering of what happens when in
skel/default/etc/scripts/system-setup.d/40-start-udev-or-load-modules.sh
so that I simply added loading modules from /etc/modules
below the already existing udev loading code.

But I could imagine a reason why the explicitly specified modules
may sometimes have to be loaded after the udev automatism:

If the explicitly specified modules were loaded before the
automated udev loading, the explicitly specified modules list
(i.e. the MODULES_LOAD array) may have to be
"somewhat complete" which means it may need to specify
all needed modules from the very beginning in the right ordering.

In contrast when the udev automatism runs first
the basic stuff should be automatically loaded so that
the MODULES_LOAD array could contain only
additionally needed modules.

I think in theory the module dependencies should
always automatically load all needed modules from the
very beginning in the right ordering
but perhaps in practice some module dependencies
are implemented only via udev rules nowadays?

Currently usr/share/rear/conf/default.conf
is very terse about how MODULES_LOAD is meant:

# Enforce to load these modules in the given order in the rescue/recovery system:
MODULES_LOAD=()

Currently it only means that MODULES_LOAD=( foo bar )
will load first 'foo' and then 'bar' but it does not tell
if that also means that 'foo' will be the very first module
that gets loaded in the rescue/recovery system.

I think to make MODULES_LOAD really powerful
its meaning should be that the modules that are
specified therein will be the very first modules
that get loaded in the rescue/recovery system.

Furthermore I would like to have a switch so that the
user can - if needed - skip the automated udev module
loading for example via a special module name like

MODULES_LOAD=( 'foo' 'bar' 'skip_automated_udev_module_loading' )

but this makes implementation a bit awkward because I
would have to check everywhere for that special value.
Perhaps I prefer a separated config variable to
skip the automated udev module loading.

Because I think I know now the root cause why the loaded
modules in the recovery system do not match the ones
in the original system, I will fix it and clean it up and
enhance it so that - if needed - the user has the final
power to specify exactly what modules will be loaded
in what ordering in the recovery system.

@schlomo
Copy link
Member

schlomo commented Jun 20, 2017

@jsmeix please wait with your effort till I am done with my changes in https://github.com/rear/rear/tree/schlomo-2017-06 (and feel free to look there and comment). Maybe you don't need to do anything at all.

I already took out one sort which should fix the ordering.

In my experience modules auto-load their dependencies without problems. In this example I had mptspi in the MODULES_LOAD array and it auto-loaded all the other SCSI and MPT modules.

I would really prefer to keep things simple here. That means that we use MODULES_LOAD to really mean load these modules before anything else in this order which is my current implementation. This also most closely reflects what the distros do with the modules inside their initrd. They are also loaded before the main system comes up and loads udev or systemd-udev. Therefore I don't see any need to make MODULES_LOAD contain some magic before and after. And I for sure wouldn't build that without a specific use case where our current code does not work.

I am actually much more worried about us supporting systemd-modules-load.service properly. Do we start that? Does systemd start it when we copy it into the rescue system? Unfortunately a quick look did not enlighten me on these questions, our boot process is already too complex.

FYI, on my Ubuntu 17.04 /etc/modules is also loaded by systemd-modules-load via a symlink:

$ ll /etc/modules-load.d/
total 8
drwxr-xr-x 1 root root   58 Jun 15 17:48 ./
drwxr-xr-x 1 root root 4558 Jun 20 10:25 ../
-rw-r--r-- 1 root root  119 Mär 21 19:36 cups-filters.conf
lrwxrwxrwx 1 root root   10 Apr 13 19:10 modules.conf -> ../modules

@jsmeix
Copy link
Member Author

jsmeix commented Jun 20, 2017

I am afraid I know basically nothing at all about
systemd related things in the recovery system.

I fully agree to load the MODULES_LOAD modules
before anything else in this order.

I watch your changes in
https://github.com/rear/rear/compare/schlomo-2017-06
and I am in particular happy how loading the specified
modules first in any case makes the code simpler
(no longer duplicated code) and better!

@jsmeix
Copy link
Member Author

jsmeix commented Jun 20, 2017

@schlomo
I think in your
usr/share/rear/build/GNU/Linux/400_copy_modules.sh

if ! grep -q $module_to_be_loaded $recovery_system_etc_modules ; then

should be more specific to avoid that substrings match,
e.g. think about module names like 'parport' and 'parport_pc'.
When 'parport_pc' is already in $recovery_system_etc_modules
no 'parport' would be added because the substring 'parport'
matches 'parport_pc'.

schlomo pushed a commit that referenced this pull request Jun 20, 2017
@schlomo
Copy link
Member

schlomo commented Jun 20, 2017

excellent catch - thanks a lot! (please ignore the last commit there, it was wrong and I pulled it back).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug The code does not do what it is meant to do fixed / solved / done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants