Add suport for erofs in the kdump initrd #33

prudo1 · 2024-08-14T15:53:10Z

This series adds support for erofs images in the kdump initrd. The feature was introduced in dracut with this PR and will be included with dracut 104.

While working on the feature I got a little bit side tracked so this series also includes a few cleanups.

This change is

The indent_size gives the number of spaces an indentation should have. When combined with indent_style=tab and tab_width=X the leading X spaces are replaced by a tab. If tab_width is omitted it is set to the value of indent_size. For the current settings this means that every level of indentation is indented by a single space which gets replaced by a tab. The tab again is displayed by a single space, if you set your editor to follow the editconfig. This is not only barely readable but also impacts line breaks for over long lines. Thus set indent_size=8 to better match the way over long lines are broken at the moment. Signed-off-by: Philipp Rudo <prudo@redhat.com>

With the introduction of the Makefile it is now easy to move files into different sub-directories. Thus create a sub-directory for each of the dracut modules and move their files there. This not only cleans up the main directory but also simplifies the Makefile and prevents bugs like the one fixed in 5109f11 ("Makefile: Fix early-kdump file names"). One nice site effect by using 'cp' instead of 'install' is that file permissions from the repo are preserved. So instead of manually setting the file permissions for each call to 'install' we can now simply change (and commit) the file permissions in the repo. Like it is done for 99kdumpbase/monitor_dd_progress.sh and 99zz-fadumpinit/module-setup.sh in this commit. Also adjust the .editorconfig to the new structure. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Commit generated using $ shfmt -s -w dracut/ Signed-off-by: Philipp Rudo <prudo@redhat.com>

Bash allows to redirect/pipe the stdout and stderr together using '&>' and '|&'. Make use of this to simplify the code a little bit. Signed-off-by: Philipp Rudo <prudo@redhat.com>

.editorconfig

coiby

Reviewed 1 of 1 files at r1, 15 of 15 files at r2, 5 of 5 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 2 of 2 files at r14, 3 of 3 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @prudo1)

mkdumprd line 16 at r9 (raw file):

# shellcheck source=/dev/null
. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR

shellcheck still complains,

In mkdumprd line 10:
        . /etc/sysconfig/kdump
          ^------------------^ SC1091 (info): Not following: /etc/sysconfig/kdump was not specified as input (see shellcheck -x).

In mkdumprd line 17:
. /lib/kdump/kdump-lib.sh
  ^---------------------^ SC1091 (info): Not following: /lib/kdump/kdump-lib.sh was not specified as input (see shellcheck -x).


In mkdumprd line 18:
. /lib/kdump/kdump-logger.sh
  ^------------------------^ SC1091 (info): Not following: /lib/kdump/kdump-logger.sh was not specified as input (see shellcheck -x).

mkfadumprd line 13 at r10 (raw file):

. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR
. /lib/kdump/kdump-lib.sh

Unforunately, shellcheck still makes the following complaints,

In mkfadumprd line 6:
        . /etc/sysconfig/kdump
          ^------------------^ SC1091 (info): Not following: /etc/sysconfig/kdump was not specified as input (see shellcheck -x).


In mkfadumprd line 13:
. /lib/kdump/kdump-lib.sh
  ^---------------------^ SC1091 (info): Not following: /lib/kdump/kdump-lib.sh was not specified as input (see shellcheck -x).


In mkfadumprd line 14:
. /lib/kdump/kdump-logger.sh
  ^------------------------^ SC1091 (info): Not following: /lib/kdump/kdump-logger.sh was not specified as input (see shellcheck -x).


In mkfadumprd line 24:
trap '
     ^-- SC2154 (warning): ret is referenced but not assigned.

dracut/99kdumpbase/kdump.sh line 8 at r5 (raw file):

# shellcheck source=/dev/null
. /lib/dracut-lib.sh
# shellcheck source-path=SCRIPTDIR/../../

Unfortunately, # shellcheck source=/dev/null masks the next errors. If we remove it, we still the following errors,

In dracut/99kdumpbase/kdump.sh line 8:
. /lib/kdump-logger.sh
  ^------------------^ SC1091 (info): Not following: /lib/kdump-logger.sh was not specified as input (see shellcheck -x).


In dracut/99kdumpbase/kdump.sh line 9:
. /lib/kdump-lib-initramfs.sh
  ^-------------------------^ SC1091 (info): Not following: /lib/kdump-lib-initramfs.sh was not specified as input (see shellcheck -x).

So we need to explicit specify the paths,

# shellcheck source=SCRIPTDIR/../../kdump-logger.sh
. /lib/kdump-logger.sh
# shellcheck source=SCRIPTDIR/../../kdump-lib-initramfs.sh
. /lib/kdump-lib-initramfs.sh

In case the version matters, I use shellcheck-0.9.0 on F40 .

dracut/99kdumpbase/module-setup.sh line 21 at r5 (raw file):

    mkdir -p "$_DRACUT_KDUMP_NM_TMP_DIR"

    # shellcheck source-path=SCRIPTDIR/../../

Unfortunately, this doesn't work since we use absolute path.

dracut/99kdumpbase/module-setup.sh line 1110 at r5 (raw file):

    inst "/lib/kdump/kdump-lib-initramfs.sh" "/lib/kdump-lib-initramfs.sh"
    inst "/lib/kdump/kdump-logger.sh" "/lib/kdump-logger.sh"
    # shellcheck disable=SC2154

# shellcheck disable=SC2154 is not needed here since the early rule also applies here.

coiby · 2024-08-21T06:19:35Z

Hi @prudo1,

This PR looks good to me except for some issues about shellcheck.

Fix the shellcheck warnings for 99kdumpbase. With this $ shellcheck -x dracut/99kdumpbase/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Fix the shellcheck warnings for 99earlykdump. With this $ shellcheck -x dracut/99earlykdump/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Fix the shellcheck warnings for 99zz-fadumpinit. With this $ shellcheck -x dracut/99zz-fadumpinit/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Fix the shellcheck warnings for kdumpctl. With this $ $ shellcheck -x kdumpctl now returns without finding. While at it make use of the special SCRIPTDIR value for source-path. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Fix the shellcheck warnings for mkdumprd. With this $ shellcheck -x mkdumprd now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Fix the shellcheck warnings for mkfadumprd. With this $ shellcheck -x mkfadumprd now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

There are currently three functions to add arguments to dracut. None of these improve readability or add any other benefit. So remove the functions and clean up the code a little bit. Signed-off-by: Philipp Rudo <prudo@redhat.com>

Add the drivers directly and drop the extra_modules variable. Signed-off-by: Philipp Rudo <prudo@redhat.com>

The lvmthinpool-moitor module was added with dracut 057 but the minimal required dracut version in the Fedora spec file is dracut >=058. So we can safely assume that the module is always present. Signed-off-by: Philipp Rudo <prudo@redhat.com>

The option --squash-compressor was introduced with dracut 057 but the minimal required dracut version in the Fedora spec file is dracut >=058. So we can safely assume that the option is always present. Signed-off-by: Philipp Rudo <prudo@redhat.com>

The handling of compression in the initrd currently is a total mess. There are multiple problems: 1) It is handled in two different locations, mkdumprd and 99kdumpbase, making the code unnecessarily complex. 2) While mkdumprd only adds the --squash-compressor option when there is no compression requested in kdump.conf:dracut_args, 99kdumpbase unconditionally adds the 99squash module. But once 99squash is added dracut ignores all compression options passed on the command line and produces an uncompressed initrd (assuming the compression is done in the squashfs image). So adding a compression option to dracut_args will neither compress the initrd nor the squashfs image. 3) To depend on 99squash, 99kdumpbase only checks whether the required kernel modules are present but doesn't check whether the squashfs-tools are installed. This can lead to failures when building the initrd as 99squash fails to install. At the moment this only works as the dracut-squash rpm depends on the squashfs-tools. But once support for erofs is added this might no longer be the case. 4) In case 99squash cannot be used mkdumprd compresses the initrd with zstd. But that doesn't really makes sense. For one compressing the initrd only reduces the on-disk size but not the memory usage during runtime of the initrd. Plus in case no compression is specified dracut will automatically compress the initrd. For that it checks for the 'best' available compression algorithm with zstd being the default. Clean this mess up be moving everything to mkdumprd, only addding 99squash when there is no compression given in the dracut_args, drop setting the --compress option and, only include 99squash when the squashfs-tools are installed. Note: Only checking the squashfs-tools is sufficient as it doesn't make sense to have them installed, when the kernel doesn't support squashfs. There might be a use case to build images that then get used on an other machine where the kernel supports it. But as the initrd is build with --hostonly that is no use case we need to consider for kdump. Signed-off-by: Philipp Rudo <prudo@redhat.com>

prudo1 · 2024-08-22T13:18:47Z

Hi @coiby,

unfortunately github doesn't allow to reply in-line so ...

Reviewed 1 of 1 files at r1, 15 of 15 files at r2, 5 of 5 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 2 of 2 files at r14, 3 of 3 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @prudo1)

mkdumprd line 16 at r9 (raw file):
# shellcheck source=/dev/null
. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR
shellcheck still complains,
In mkdumprd line 10:
        . /etc/sysconfig/kdump
          ^------------------^ SC1091 (info): Not following: /etc/sysconfig/kdump was not specified as input (see shellcheck -x).

In mkdumprd line 17:
. /lib/kdump/kdump-lib.sh
  ^---------------------^ SC1091 (info): Not following: /lib/kdump/kdump-lib.sh was not specified as input (see shellcheck -x).


In mkdumprd line 18:
. /lib/kdump/kdump-logger.sh
  ^------------------------^ SC1091 (info): Not following: /lib/kdump/kdump-logger.sh was not specified as input (see shellcheck -x).

Those are all only hints that you need to call shellcheck with option -x for it to follow those files. When you do that the warnings goes away. Same for most of the other warnings you mentioned.

mkfadumprd line 13 at r10 (raw file):
. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR
. /lib/kdump/kdump-lib.sh
Unforunately, shellcheck still makes the following complaints,

[...]

In mkfadumprd line 24:
trap '
^-- SC2154 (warning): ret is referenced but not assigned.

Ouch, this one was masked by using -x as ret is defined in the kdump-logger.sh...
I've disabled the warning here and in mkdumprd.
I'm not entirely sure why it doesn't show in kdumpctl. Shall I disable it there, too?

[...]

In case the version matters, I use shellcheck-0.9.0 on F40 .

I'm using the same version. Although, when I created the series I was still on F39. But IIRC the shellcheck version was the same.

dracut/99kdumpbase/module-setup.sh line 21 at r5 (raw file):
    mkdir -p "$_DRACUT_KDUMP_NM_TMP_DIR"

    # shellcheck source-path=SCRIPTDIR/../../
Unfortunately, this doesn't work since we use absolute path.

Right, I misunderstood the shellcheck documentation. I thought that it would always only use the filename and attach it to SCRIPTDIR. Do you know if there is a way to tell shellcheck to print out the absolute path it uses as source? That would be handy when adding those statements so you can verify that shellcheck uses the right file.

Anyway, I've dropped the use of source-path and explicitly set source every time. This should also solve the problem with source=/dev/null hiding problems.

dracut/99kdumpbase/module-setup.sh line 1110 at r5 (raw file):
    inst "/lib/kdump/kdump-lib-initramfs.sh" "/lib/kdump-lib-initramfs.sh"
    inst "/lib/kdump/kdump-logger.sh" "/lib/kdump-logger.sh"
    # shellcheck disable=SC2154
# shellcheck disable=SC2154 is not needed here since the early rule also applies here.

Thanks! Dropped it.

prudo1 · 2024-08-22T13:22:21Z

Added changes requested by @coiby

liutgnu

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @coiby and @prudo1)

mkdumprd line 70 at r29 (raw file):

	done

	dracut --list-modules | grep -x -q "${_args[@]}"

I guess it will have a problem same as commit ("98087d78eda Use "grep -q <<< $(cmd)" instead of "cmd | grep -q""), could we use "<<<" instead?

Other than that, the patchset LGTM.

prudo1 · 2024-08-27T14:18:26Z

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @coiby and @prudo1)

mkdumprd line 70 at r29 (raw file):
	done

	dracut --list-modules | grep -x -q "${_args[@]}"
I guess it will have a problem same as commit ("98087d78eda Use "grep -q <<< $(cmd)" instead of "cmd | grep -q""), could we use "<<<" instead?

@liutgnu Thanks for the hint! Fixed it.

Other than that, the patchset LGTM.

Thanks

With dracut 104 support for erofs in 99squash was added. For that the squashfs specific code was split from 99squash module into 95squash-squashfs and a new 95squash-erofs was added. The modules are structured the way, that you can either add 99squash, which then picks the 'best' back end, or one of the 95squash-{squashfs,erofs} if you want to make sure which back end is used. Unfortunately erofs doesn't support the same compression algorithms squashfs supports. So explicitly set which image type we want so we can set the correct --squash-compressor option. Keep support for the old 99squash for the time being so newer versions of kdump-utils can work with dracut <= 103. Signed-off-by: Philipp Rudo <prudo@redhat.com>

prudo1 · 2024-08-27T14:20:20Z

fixed the problem pointed out by @liutgnu

liutgnu

LGTM

prudo1 added 4 commits August 13, 2024 14:34

dracut/*: fix coding style using shfmt

4b863c8

Commit generated using $ shfmt -s -w dracut/ Signed-off-by: Philipp Rudo <prudo@redhat.com>

99kdumpbase: drop unnecessary 2>&1

c062f01

Bash allows to redirect/pipe the stdout and stderr together using '&>' and '|&'. Make use of this to simplify the code a little bit. Signed-off-by: Philipp Rudo <prudo@redhat.com>

prudo1 changed the title ~~Features/erofs/main~~ Add suport for erofs in the kdump initrd Aug 20, 2024

coiby reviewed Aug 20, 2024

View reviewed changes

.editorconfig Show resolved Hide resolved

coiby requested changes Aug 21, 2024

View reviewed changes

prudo1 added 11 commits August 22, 2024 14:53

99kdumpbase: fix shellcheck warnings

a692e1f

Fix the shellcheck warnings for 99kdumpbase. With this $ shellcheck -x dracut/99kdumpbase/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

99earlykdump: fix shellcheck warnings

d7f5719

Fix the shellcheck warnings for 99earlykdump. With this $ shellcheck -x dracut/99earlykdump/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

99zz-fadumpinit: fix shellcheck warnings

48a9e90

Fix the shellcheck warnings for 99zz-fadumpinit. With this $ shellcheck -x dracut/99zz-fadumpinit/*.sh now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

kdumpctl: fix shellcheck warnings

2df3c07

Fix the shellcheck warnings for kdumpctl. With this $ $ shellcheck -x kdumpctl now returns without finding. While at it make use of the special SCRIPTDIR value for source-path. Signed-off-by: Philipp Rudo <prudo@redhat.com>

mkdumprd: fix shellcheck warnings

b1bcde3

Fix the shellcheck warnings for mkdumprd. With this $ shellcheck -x mkdumprd now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

mkfadumprd: fix shellcheck warnings

5465ac6

Fix the shellcheck warnings for mkfadumprd. With this $ shellcheck -x mkfadumprd now returns without finding. Signed-off-by: Philipp Rudo <prudo@redhat.com>

mkdumprd: simplify handling of dracut arguments

8592a4a

There are currently three functions to add arguments to dracut. None of these improve readability or add any other benefit. So remove the functions and clean up the code a little bit. Signed-off-by: Philipp Rudo <prudo@redhat.com>

mkdumprd: drop extra_modules variable

1ebb17c

Add the drivers directly and drop the extra_modules variable. Signed-off-by: Philipp Rudo <prudo@redhat.com>

prudo1 force-pushed the features/erofs/main branch from 453fdc1 to 123a3c5 Compare August 22, 2024 13:19

liutgnu requested changes Aug 27, 2024

View reviewed changes

prudo1 force-pushed the features/erofs/main branch from 123a3c5 to 377eb26 Compare August 27, 2024 14:19

prudo1 requested review from coiby and liutgnu August 28, 2024 12:53

coiby approved these changes Aug 29, 2024

View reviewed changes

liutgnu approved these changes Aug 29, 2024

View reviewed changes

coiby merged commit e2b8463 into rhkdump:main Aug 29, 2024

This was referenced Aug 30, 2024

Enable GitHub action to test the patches as early and as often as possible #21

Merged

Introduce vmcore creation notification to kdump #20

Merged

prudo1 mentioned this pull request Sep 24, 2024

Spec file fix and cleanups #36

Merged

jozzsi mentioned this pull request Sep 29, 2025

consider adding test coverage for squash-squashfs dracut module dracut-ng/dracut-ng#1733

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add suport for erofs in the kdump initrd #33

Add suport for erofs in the kdump initrd #33

Uh oh!

prudo1 commented Aug 14, 2024 •

edited by coiby

Loading

Uh oh!

Uh oh!

coiby left a comment

Uh oh!

coiby commented Aug 21, 2024

Uh oh!

prudo1 commented Aug 22, 2024

Uh oh!

prudo1 commented Aug 22, 2024

Uh oh!

liutgnu left a comment •

edited

Loading

Uh oh!

prudo1 commented Aug 27, 2024

Uh oh!

prudo1 commented Aug 27, 2024

Uh oh!

liutgnu left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add suport for erofs in the kdump initrd #33

Add suport for erofs in the kdump initrd #33

Uh oh!

Conversation

prudo1 commented Aug 14, 2024 • edited by coiby Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coiby left a comment

Choose a reason for hiding this comment

Uh oh!

coiby commented Aug 21, 2024

Uh oh!

prudo1 commented Aug 22, 2024

Uh oh!

prudo1 commented Aug 22, 2024

Uh oh!

liutgnu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prudo1 commented Aug 27, 2024

Uh oh!

prudo1 commented Aug 27, 2024

Uh oh!

liutgnu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prudo1 commented Aug 14, 2024 •

edited by coiby

Loading

liutgnu left a comment •

edited

Loading