Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dracut cpio-reflink feature detection #1

Closed
wants to merge 2 commits into from

Conversation

mwilck
Copy link

@mwilck mwilck commented Aug 30, 2021

Hi David,

here are 2 patches, the first fixes a minor issue in your code, the 2nd adds autodetection for cpio-reflink which we discussed in Jira.

Martin

dinfo() requires dracut-init.sh to be loaded.
The cpio-reflink feature requires certain features of the file system
and system configuratino and is incompatible with certain other
options. Make sure settings are consistent.
@mwilck mwilck changed the title Dracut cpio suse 55 Dracut cpio-reflink feature detection Aug 30, 2021
Copy link
Owner

@ddiss ddiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Martin! I'll collapse the dinfo fix in with my current upstream submission.
Regarding the is_reflink_supported() change - I'd prefer to only print a warning, rather than have it disable the explicitly enabled reflink option.

@mwilck
Copy link
Author

mwilck commented Aug 31, 2021

I'd prefer to only print a warning, rather than have it disable the explicitly enabled reflink option.

Well, you did the same in your code (albeit only if dracut-cpio was unavailable). IMO the option must be disabled (with warning) if it can't possibly work. Otherwise people will complain to us (you :-) ) because they don't understand the feature (which isn't exactly trivial).

The relationship between the various options is a different issue. My idea was:

  • disable cpio-reflink if conflicting options like --compress or --strip were used on the command line
  • adapt compress and do_strip if they were just set in config files / from defaults and cpio-reflink is active.

I did not distinguish whether cpio-reflink itself was set on the command line or in config files/defaults. IMO command line should always take precedence, but it's unclear what to do if e.g. both --compress=xz and --cpio-reflink were given on the command line. Actually we should probably just error out in that case.

This is to to explain my line of thought. Feel free to modify this logic as you see fit. But if there are contradictory / inconsistent options, we must do something about it.

@ddiss
Copy link
Owner

ddiss commented Aug 31, 2021

I'd prefer to only print a warning, rather than have it disable the explicitly enabled reflink option.

Well, you did the same in your code (albeit only if dracut-cpio was unavailable). IMO the option must be disabled (with warning) if it can't possibly work. Otherwise people will complain to us (you :-) ) because they don't understand the feature (which isn't exactly trivial).

The relationship between the various options is a different issue. My idea was:

* disable `cpio-reflink` if conflicting options like `--compress` or `--strip` were used _on the command line_

* adapt `compress` and `do_strip` if they were just set in config files / from defaults and `cpio-reflink` is active.

I completely agree that these options should be consistent. --strip is already a warning. For compression, the lack of dracut-cpio stdout support is why I took the easy option of disabling it. I'll fix the inconsistency by adding support for compression.
dracut-cpio should work just fine, regardless of whether the underlying FS supports CoW reflinks. To make this clearer, maybe I should change the option to enhanced-cpio or something like that?

I did not distinguish whether cpio-reflink itself was set on the command line or in config files/defaults. IMO command line should always take precedence, but it's unclear what to do if e.g. both --compress=xz and --cpio-reflink were given on the command line. Actually we should probably just error out in that case.

dracut.sh processes cmdline arguments before handing config files, so I don't think precedence should be changed here. We could consider detecting conflicting config and cmdline parameters, but I'd prefer to keep it simple for now.

This is to to explain my line of thought. Feel free to modify this logic as you see fit. But if there are contradictory / inconsistent options, we must do something about it.

Thanks for the explanation :-)

@mwilck
Copy link
Author

mwilck commented Aug 31, 2021

For compression, the lack of dracut-cpio stdout support is why I took the easy option of disabling it. I'll fix the inconsistency by adding support for compression.

Why the extra effort?

dracut-cpio should work just fine, regardless of whether the underlying FS supports CoW reflinks.

I have no doubt about that. But I was using a different definition of "work", see below.

To make this clearer, maybe I should change the option to enhanced-cpio or something like that?

My thinking (which obviously doesn't fully match yours) was that this option activates space-efficient initrd generation by making use of reflinks. Therefore I'd prefer a simpler option name such as --reflink. By the same mind set, I thought that this option should fail or print warnings if it can determine from the environment that space savings won't occur because reflinking won't work, or if conflicting options are detected.

Please take off your developer 🎩 for a while 😄. From a user perspective, it only matters whether reflinks are used, not which cpio executable creates them.

dracut.sh processes cmdline arguments before handing config files

Command line options set the -_l suffixed variables such as compress_l, which take precendence over all config file settings, independent of the order processed. E.g. SUSE currently sets compress="xz -0 --check=crc32 --memlimit-compress=50%" in 01-dist.conf, but I can override that by calling dracut --compress=cat.

@ddiss
Copy link
Owner

ddiss commented Sep 2, 2021

For compression, the lack of dracut-cpio stdout support is why I took the easy option of disabling it. I'll fix the inconsistency by adding support for compression.

Why the extra effort?

dracut-cpio should work just fine, regardless of whether the underlying FS supports CoW reflinks.

I have no doubt about that. But I was using a different definition of "work", see below.

To make this clearer, maybe I should change the option to enhanced-cpio or something like that?

My thinking (which obviously doesn't fully match yours) was that this option activates space-efficient initrd generation by making use of reflinks. Therefore I'd prefer a simpler option name such as --reflink. By the same mind set, I thought that this option should fail or print warnings if it can determine from the environment that space savings won't occur because reflinking won't work, or if conflicting options are detected.

dracut.sh already uses cp --reflink extensively when shuffling thing into the staging area, which is why I went with --cpio-reflink.

Please take off your developer tophat for a while smile. From a user perspective, it only matters whether reflinks are used, not which cpio executable creates them.

I'd be more inclined to say that the user doesn't care whether or not reflinks are used, they just want things to be as fast as possible. dracut-cpio should provide that in all cases, as data will either be reflinked or (when not available) be copied in kernel via splice().

I plan on updating the PR today with compression support and your fix squashed in.

@ddiss
Copy link
Owner

ddiss commented Sep 2, 2021

I missed a couple of other points...

For compression, the lack of dracut-cpio stdout support is why I took the easy option of disabling it. I'll fix the inconsistency by adding support for compression.

Why the extra effort?

It's not much in the way of effort, the dracut-cpio output just needs to get piped through $compress.

...

Command line options set the -_l suffixed variables such as compress_l, which take precendence over all config file settings, independent of the order processed. E.g. SUSE currently sets compress="xz -0 --check=crc32 --memlimit-compress=50%" in 01-dist.conf, but I can override that by calling dracut --compress=cat.

Thanks. I overlooked this, so will add _l precedence in the next round.

@ddiss
Copy link
Owner

ddiss commented Sep 17, 2021

I'm going to close this downstream PR. Please raise any further questions / concerns against the upstream PR at dracutdevs#1531 .

I think it probably does still make sense to retain the is_reflink_supported() logic for use when selecting defaults in suse.conf.example, but I think it can wait until the core functionality is upstream.

@ddiss ddiss closed this Sep 17, 2021
ddiss pushed a commit that referenced this pull request Jul 30, 2023
in install_dependent_modules we use &path[kerneldirlen] as the key for inserting,
let's do the same for checking.

otherwise installing kernel module has circular dependency from a custom kernel
module directory will cause infinite recursion and segfault.

$ grep ipmi kbuilt/lib/modules/5.10.121/modules.dep
kernel/drivers/char/ipmi/ipmi_msghandler.ko:
kernel/drivers/char/ipmi/ipmi_devintf.ko: kernel/drivers/char/ipmi/ipmi_msghandler.ko

$ grep ipmi kbuilt/lib/modules/5.10.121/modules.softdep
softdep ipmi_msghandler post: ipmi_devintf

$ ./dracut-install -D /tmp --kerneldir ~/working/kernel/linux-5.10.121/kbuilt/lib/modules/5.10.121 -m ipmi-devintf
Segmentation fault (core dumped)

(gdb) b install_dependent_modules
Breakpoint 1 at 0x7db0: file src/install/dracut-install.c, line 1513.
(gdb) bt
#0  install_dependent_modules (modlist=0x0) at src/install/dracut-install.c:1513
#1  0x000055555555c027 in install_dependent_modules (modlist=modlist@entry=0x555555579e90) at src/install/dracut-install.c:1553
dracutdevs#2  0x000055555555bf1c in install_dependent_modules (modlist=0x5555555799d0) at src/install/dracut-install.c:1548
dracutdevs#3  0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555557a3f0) at src/install/dracut-install.c:1554
dracutdevs#4  0x000055555555bf1c in install_dependent_modules (modlist=0x555555579d60) at src/install/dracut-install.c:1548
dracutdevs#5  0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555557b170) at src/install/dracut-install.c:1554
dracutdevs#6  0x000055555555bf1c in install_dependent_modules (modlist=0x55555557a0f0) at src/install/dracut-install.c:1548
dracutdevs#7  0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x555555575320) at src/install/dracut-install.c:1554
dracutdevs#8  0x000055555555bf1c in install_dependent_modules (modlist=0x55555557ab30) at src/install/dracut-install.c:1548
dracutdevs#9  0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555557dd60) at src/install/dracut-install.c:1554
dracutdevs#10 0x000055555555bf1c in install_dependent_modules (modlist=0x55555557b640) at src/install/dracut-install.c:1548
dracutdevs#11 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555557e0f0) at src/install/dracut-install.c:1554
dracutdevs#12 0x000055555555bf1c in install_dependent_modules (modlist=0x55555557b9d0) at src/install/dracut-install.c:1548
dracutdevs#13 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x555555574340) at src/install/dracut-install.c:1554
dracutdevs#14 0x000055555555bf1c in install_dependent_modules (modlist=0x55555557cf70) at src/install/dracut-install.c:1548
dracutdevs#15 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x5555555768d0) at src/install/dracut-install.c:1554
dracutdevs#16 0x000055555555bf1c in install_dependent_modules (modlist=0x55555557d750) at src/install/dracut-install.c:1548
dracutdevs#17 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555557e700) at src/install/dracut-install.c:1554
dracutdevs#18 0x000055555555bf1c in install_dependent_modules (modlist=0x55555557de90) at src/install/dracut-install.c:1548
dracutdevs#19 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x555555581c90) at src/install/dracut-install.c:1554
dracutdevs#20 0x000055555555bf1c in install_dependent_modules (modlist=0x555555571e60) at src/install/dracut-install.c:1548
dracutdevs#21 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555556b620) at src/install/dracut-install.c:1554
dracutdevs#22 0x000055555555bf1c in install_dependent_modules (modlist=0x555555583000) at src/install/dracut-install.c:1548
dracutdevs#23 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x55555556b640) at src/install/dracut-install.c:1554
dracutdevs#24 0x000055555555bf1c in install_dependent_modules (modlist=0x555555571b40) at src/install/dracut-install.c:1548
dracutdevs#25 0x000055555555c034 in install_dependent_modules (modlist=modlist@entry=0x555555574100) at src/install/dracut-install.c:1554
dracutdevs#26 0x000055555555c4b0 in install_module (mod=mod@entry=0x555555573bc0) at src/install/dracut-install.c:1617
dracutdevs#27 0x000055555555c93d in install_modules (argc=argc@entry=1, argv=argv@entry=0x7fffffffd6e0) at src/install/dracut-install.c:1952
dracutdevs#28 0x000055555555862a in main (argc=<optimized out>, argv=0x7fffffffd6a8) at src/install/dracut-install.c:2090

Signed-off-by: runsisi <runsisi@hust.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants