[PATCH v2 0/3] fadump improvements

[Hari Bathini]

This set tries to do a few improvements to fadump. The 1st patch makes
'zstd' as the default compression method for fadump. The next patch
fixes the default initrd backup/restore logic. The 3rd patch ensures
fadump initramfs is cleaned up on kernel uninstall.

Changes in v2:
* Improve changelog on why 'zstd' is used for fadump in patch#1.
* Patch#2: A new patch to fix default initrd backup/restore logic.
* Patch#3: Cleaning up kernel version based checksum file.

Hari Bathini (3):
  fadump: use 'zstd' as the default compression method
  fadump: fix default initrd backup and restore logic
  fadump: add a kernel install hook to clean up fadump initramfs

60-fadump.install | 32 ++++++++++++++++++++++++++++++++
 kdumpctl          |  8 ++++----
 kexec-tools.spec  |  3 +++
 mkfadumprd        |  6 ++----
 4 files changed, 41 insertions(+), 8 deletions(-)
 create mode 100755 60-fadump.install

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Hari Bathini]

If available, use 'zstd' compression method to optimize the size of
the initrd built with fadump support. Also, 'squash+zstd' is not
preferred because more disk space is consumed with 'squash+zstd' due
to the additional binaries needed for fadump with squash case.

Signed-off-by: Hari Bathini 
---

Changes in v2:
* Updated changelog with why 'zstd' is used for fadump.

mkfadumprd | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mkfadumprd b/mkfadumprd
index 36ad645..2fc396c 100644
--- a/mkfadumprd
+++ b/mkfadumprd
@@ -62,11 +62,9 @@ _dracut_isolate_args=(
    /usr/lib/dracut/fadump-kernel-modules.txt
 )

-# Same as setting zstd in mkdumprd
+# Use zstd compression method, if available
 if ! have_compression_in_dracut_args; then
-	if is_squash_available && dracut_have_option "--squash-compressor"; then
-		_dracut_isolate_args+=(--squash-compressor zstd)
-	elif is_zstd_command_available; then
+	if is_zstd_command_available; then
    	_dracut_isolate_args+=(--compress zstd)
    fi
 fi

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Tao Liu]

Hi Hari,

On Mon, Nov 21, 2022 at 8:24 PM Hari Bathini  wrote:
> If available, use 'zstd' compression method to optimize the size of
> the initrd built with fadump support. Also, 'squash+zstd' is not
> preferred because more disk space is consumed with 'squash+zstd' due
> to the additional binaries needed for fadump with squash case.
> Signed-off-by: Hari Bathini
>
> Changes in v2:
> Updated changelog with why 'zstd' is used for fadump.

Thanks for the update, the patch looks good to me.

Acked-by: Tao Liu
> mkfadumprd | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/mkfadumprd b/mkfadumprd
> index 36ad645..2fc396c 100644
> --- a/mkfadumprd
> +++ b/mkfadumprd
> @@ -62,11 +62,9 @@ _dracut_isolate_args=(
>         /usr/lib/dracut/fadump-kernel-modules.txt
>  )
>
> -# Same as setting zstd in mkdumprd
> +# Use zstd compression method, if available
>  if ! have_compression_in_dracut_args; then
>   if is_squash_available && dracut_have_option "--squash-compressor"; then
>           _dracut_isolate_args+=(--squash-compressor zstd)
>   elif is_zstd_command_available; then
>   if is_zstd_command_available; then
>           _dracut_isolate_args+=(--compress zstd)
>   fi
> fi
>
> 2.38.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedoraproject.org/archives/list/[e-mail]
> Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Coiby Xu]

On Tue, Nov 22, 2022 at 09:24:24AM +0800, Tao Liu wrote:
> Hi Hari,
>
> On Mon, Nov 21, 2022 at 8:24 PM Hari Bathini  wrote:
> > If available, use 'zstd' compression method to optimize the size of
> > the initrd built with fadump support. Also, 'squash+zstd' is not
> > preferred because more disk space is consumed with 'squash+zstd' due
> > to the additional binaries needed for fadump with squash case.
> > Signed-off-by: Hari Bathini
> >
> > Changes in v2:
> > Updated changelog with why 'zstd' is used for fadump.
>
> Thanks for the update, the patch looks good to me.
>
> Acked-by: Tao Liu

Hi Tao,

When trying to learn how squash affects the memory requirement of kdump,
I notice currently for both F36 and F37, the initramfs is no longer
compressed,

[root@ci-vm-10-0-137-168 t]# file /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img
   /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)

[root@ci-vm-10-0-139-47 ~]# file /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img
/boot/initramfs-6.0.9-200.fc36.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)

This is fc1c79ff ("Seperate dracut and dracut-squash compressor for
zstd") disabls passing "--compress zstd" to dracut when the squashfs
dracut module is going to be used. Is the change intentional? If not,
maybe we should drop this patch and do it in mkdumprd instead.

Btw, it seems "--compress zstd" or "--zstd" no longer compresses the
initramfs in rawhide (2022-11-24) whereas in f36 it works as expected. Another
difference between rawhide and f36 is in 36 with "--zstd" not specified,
dracut will gzip-compress the initramfs while in rawhide, it doesn't
compress the initramfs.
> > mkfadumprd | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/mkfadumprd b/mkfadumprd
> > index 36ad645..2fc396c 100644
> > --- a/mkfadumprd
> > +++ b/mkfadumprd
> > @@ -62,11 +62,9 @@ _dracut_isolate_args=(
> >         /usr/lib/dracut/fadump-kernel-modules.txt
> >  )
> >
> > -# Same as setting zstd in mkdumprd
> > +# Use zstd compression method, if available
> >  if ! have_compression_in_dracut_args; then
> >   if is_squash_available && dracut_have_option "--squash-compressor"; then
> >           _dracut_isolate_args+=(--squash-compressor zstd)
> >   elif is_zstd_command_available; then
> >   if is_zstd_command_available; then
> >           _dracut_isolate_args+=(--compress zstd)
> >   fi
> > fi
> >
> > 2.38.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives: https://lists.fedoraproject.org/archives/list/[e-mail]
> > Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Coiby Xu]

On Thu, Nov 24, 2022 at 10:12:14AM +0800, Coiby Xu wrote:
> On Tue, Nov 22, 2022 at 09:24:24AM +0800, Tao Liu wrote:
> > Hi Hari,
> >
> > On Mon, Nov 21, 2022 at 8:24 PM Hari Bathini  wrote:
> > > If available, use 'zstd' compression method to optimize the size of
> > > the initrd built with fadump support. Also, 'squash+zstd' is not
> > > preferred because more disk space is consumed with 'squash+zstd' due
> > > to the additional binaries needed for fadump with squash case.
> > > Signed-off-by: Hari Bathini
> > >
> > > Changes in v2:
> > > Updated changelog with why 'zstd' is used for fadump.
> >
> > Thanks for the update, the patch looks good to me.
> >
> > Acked-by: Tao Liu
>
> Hi Tao,
>
> When trying to learn how squash affects the memory requirement of kdump,
> I notice currently for both F36 and F37, the initramfs is no longer
> compressed,
>
> [root@ci-vm-10-0-137-168 t]# file /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img
>  /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
>  [root@ci-vm-10-0-139-47 ~]# file /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img
> /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
>
> This is fc1c79ff ("Seperate dracut and dracut-squash compressor for
> zstd") disabls passing "--compress zstd" to dracut when the squashfs
> dracut module is going to be used. Is the change intentional? If not,
> maybe we should drop this patch and do it in mkdumprd instead.
>
> Btw, it seems "--compress zstd" or "--zstd" no longer compresses the
> initramfs in rawhide (2022-11-24) whereas in f36 it works as expected. Another
> difference between rawhide and f36 is in 36 with "--zstd" not specified,
> dracut will gzip-compress the initramfs while in rawhide, it doesn't
> compress the initramfs.

After having a further look, I realize the latter issue has nothing to
do with the difference between rawhide and f36. Once the dracut squash
module is enabled, "--compress zstd" will no longer work.

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Coiby Xu]

On Thu, Nov 24, 2022 at 10:26:22AM +0800, Coiby Xu wrote:
> On Thu, Nov 24, 2022 at 10:12:14AM +0800, Coiby Xu wrote:
> > On Tue, Nov 22, 2022 at 09:24:24AM +0800, Tao Liu wrote:
> > > Hi Hari,
> > >
> > > On Mon, Nov 21, 2022 at 8:24 PM Hari Bathini  wrote:
> > > > If available, use 'zstd' compression method to optimize the size of
> > > > the initrd built with fadump support. Also, 'squash+zstd' is not
> > > > preferred because more disk space is consumed with 'squash+zstd' due
> > > > to the additional binaries needed for fadump with squash case.
> > > > Signed-off-by: Hari Bathini
> > > >
> > > > Changes in v2:
> > > > Updated changelog with why 'zstd' is used for fadump.
> > >
> > > Thanks for the update, the patch looks good to me.
> > >
> > > Acked-by: Tao Liu
> >
> > Hi Tao,
> >
> > When trying to learn how squash affects the memory requirement of kdump,
> > I notice currently for both F36 and F37, the initramfs is no longer
> > compressed,
> >
> > [root@ci-vm-10-0-137-168 t]# file /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img
> > /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
> > [root@ci-vm-10-0-139-47 ~]# file /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img
> > /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
> >
> > This is fc1c79ff ("Seperate dracut and dracut-squash compressor for
> > zstd") disabls passing "--compress zstd" to dracut when the squashfs
> > dracut module is going to be used. Is the change intentional? If not,
> > maybe we should drop this patch and do it in mkdumprd instead.
> >
> > Btw, it seems "--compress zstd" or "--zstd" no longer compresses the
> > initramfs in rawhide (2022-11-24) whereas in f36 it works as expected. Another
> > difference between rawhide and f36 is in 36 with "--zstd" not specified,
> > dracut will gzip-compress the initramfs while in rawhide, it doesn't
> > compress the initramfs.
>
> After having a further look, I realize the latter issue has nothing to
> do with the difference between rawhide and f36. Once the dracut squash
> module is enabled, "--compress zstd" will no longer work.

After having discussion with Tao, I realize my confusion is due to
to my misconception. In fact, dracut explicitly prevents compressing
the initramfs when the squash module is used because there is very
little benefit when compressing it for a second time  so it's pointless
to pass "--compress zstd" to dracut in this case. Thanks to Tao's
clarification!

[PATCH v2 1/3] fadump: use 'zstd' as the default compression method

[Tao Liu]

Hi,

Currently the --zstd and --compressor zstd only effect on initramfs
compression, --squash-compressor only effect on squash-root.img
compression. We don't want to compress initramfs and squash-root.img
both, because it is unnecessary, if doing so the squash-root.img will
be compressed 2 times, the 2nd compression ratio will be low and waste
the cpu time for decompression in vain.

So

if squashfs avaliable,  --squash-compressor zstd will be passed.
squashfs-root.img will be compressed into zstd, and initramfs will not
be compressed.
if squashfs unavaliable --compress zstd will be passed, and initramfs
will be zstd compressed.

if squashfs avaliable, but --squash-compressor option not supported by
dracut(due to dracut version < 057), squashfs-root.img will be
compressed into default format(zlib).

Thanks,
Tao Liu

On Thu, Nov 24, 2022 at 10:30 AM Coiby Xu  wrote:
> On Thu, Nov 24, 2022 at 10:12:14AM +0800, Coiby Xu wrote:
> > On Tue, Nov 22, 2022 at 09:24:24AM +0800, Tao Liu wrote:
> > > Hi Hari,
> > >
> > > On Mon, Nov 21, 2022 at 8:24 PM Hari Bathini  wrote:
> > > > If available, use 'zstd' compression method to optimize the size of
> > > > the initrd built with fadump support. Also, 'squash+zstd' is not
> > > > preferred because more disk space is consumed with 'squash+zstd' due
> > > > to the additional binaries needed for fadump with squash case.
> > > > Signed-off-by: Hari Bathini
> > > >
> > > > Changes in v2:
> > > > Updated changelog with why 'zstd' is used for fadump.
> > >
> > > Thanks for the update, the patch looks good to me.
> > >
> > > Acked-by: Tao Liu
> >
> > Hi Tao,
> >
> > When trying to learn how squash affects the memory requirement of kdump,
> > I notice currently for both F36 and F37, the initramfs is no longer
> > compressed,
> >
> > [root@ci-vm-10-0-137-168 t]# file /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img
> >  /boot/initramfs-6.1.0-0.rc6.46.fc38.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
> >  [root@ci-vm-10-0-139-47 ~]# file /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img
> > /boot/initramfs-6.0.9-200.fc36.x86_64kdump.img: ASCII cpio archive (SVR4 with no CRC)
> >
> > This is fc1c79ff ("Seperate dracut and dracut-squash compressor for
> > zstd") disabls passing "--compress zstd" to dracut when the squashfs
> > dracut module is going to be used. Is the change intentional? If not,
> > maybe we should drop this patch and do it in mkdumprd instead.
> >
> > Btw, it seems "--compress zstd" or "--zstd" no longer compresses the
> > initramfs in rawhide (2022-11-24) whereas in f36 it works as expected. Another
> > difference between rawhide and f36 is in 36 with "--zstd" not specified,
> > dracut will gzip-compress the initramfs while in rawhide, it doesn't
> > compress the initramfs.
>
> After having a further look, I realize the latter issue has nothing to
> do with the difference between rawhide and f36. Once the dracut squash
> module is enabled, "--compress zstd" will no longer work.
>
> --
> Best regards,
> Coiby

[PATCH v2 2/3] fadump: fix default initrd backup and restore logic

[Hari Bathini]

In case of fadump, default initrd is rebuilt with dump capturing
capability, as the same initrd is used for booting production kernel
as well as capture kernel.

The original initrd file is backed up with a checksum, to restore
it as the default initrd when fadump is disabled. As the checksum
file is not kernel version specific, switching between different
kernel versions and kdump/fadump dump mode breaks the default initrd
backup/restore logic. Fix this by having a kernel version specific
checksum file.

Signed-off-by: Hari Bathini 
---

* New patch to fix default initrd backup/restore logic.

kdumpctl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 7caaae5..03c91f3 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -163,7 +163,7 @@ backup_default_initrd()
    if [[ ! -e $DEFAULT_INITRD_BAK ]]; then
    	dinfo "Backing up $DEFAULT_INITRD before rebuild."
    	# save checksum to verify before restoring
-		sha1sum "$DEFAULT_INITRD" > "$INITRD_CHECKSUM_LOCATION"
+		sha1sum "$DEFAULT_INITRD" > "$INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER"
    	if ! cp "$DEFAULT_INITRD" "$DEFAULT_INITRD_BAK"; then
    		dwarn "WARNING: failed to backup $DEFAULT_INITRD."
    		rm -f "$DEFAULT_INITRD_BAK"
@@ -181,14 +181,14 @@ restore_default_initrd()

# If a backup initrd exists, we must be switching back from
    # fadump to kdump. Restore the original default initrd.
-	if [[ -f $DEFAULT_INITRD_BAK ]] && [[ -f $INITRD_CHECKSUM_LOCATION ]]; then
+	if [[ -f $DEFAULT_INITRD_BAK ]] && [[ -f $INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER ]]; then
    	# verify checksum before restoring
    	backup_checksum=$(sha1sum "$DEFAULT_INITRD_BAK" | awk '{ print $1 }')
-		default_checksum=$(awk '{ print $1 }' "$INITRD_CHECKSUM_LOCATION")
+		default_checksum=$(awk '{ print $1 }' "$INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER")
    	if [[ $default_checksum != "$backup_checksum" ]]; then
    		dwarn "WARNING: checksum mismatch! Can't restore original initrd.."
    	else
-			rm -f $INITRD_CHECKSUM_LOCATION
+			rm -f $INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER
    		if mv "$DEFAULT_INITRD_BAK" "$DEFAULT_INITRD"; then
    			derror "Restoring original initrd as fadump mode is disabled."
    			sync -f "$DEFAULT_INITRD"

[PATCH v2 2/3] fadump: fix default initrd backup and restore logic

[Philipp Rudo]

Hi Hari,

On Mon, 21 Nov 2022 17:48:01 +0530
Hari Bathini  wrote:
> In case of fadump, default initrd is rebuilt with dump capturing
> capability, as the same initrd is used for booting production kernel
> as well as capture kernel.
>
> The original initrd file is backed up with a checksum, to restore
> it as the default initrd when fadump is disabled. As the checksum
> file is not kernel version specific, switching between different
> kernel versions and kdump/fadump dump mode breaks the default initrd
> backup/restore logic. Fix this by having a kernel version specific
> checksum file.
>
> Signed-off-by: Hari Bathini

Thanks for taking care!

Two small comments:

1) in backup_default_initrd you should remove the checksum file in the
   error case, when the initrd cannot be backed up (this should
   probably be done today already...)

2) why don't you simply update INITRD_CHECKSUM_OCATION one instead of
   editing every line where it is used? For example consider the patch
   below. I see multiple benefits (1) it's shorter, (2) the name shows
   clearer to which initrd the checksum belongs to, (3) it reuses
   KDUMP_BOOTDIR instead of hard coding /boot and (4)
   INITRD_CHECKSUM_LOCATION now is basically local to setup_initrd now
   making it easier for a clean up to remove the global variable later
   on.

What do you think?

Thanks
Philipp

diff --git a/kdumpctl b/kdumpctl
index 7caaae5..94af3e2 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -9,7 +9,7 @@ KDUMP_LOG_PATH="/var/log"
 MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
-INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
+INITRD_CHECKSUM_LOCATION=""
 DEFAULT_INITRD=""
 DEFAULT_INITRD_BAK=""
 KDUMP_INITRD=""
@@ -166,7 +166,8 @@ backup_default_initrd()
                sha1sum "$DEFAULT_INITRD" > "$INITRD_CHECKSUM_LOCATION"
                if ! cp "$DEFAULT_INITRD" "$DEFAULT_INITRD_BAK"; then
                        dwarn "WARNING: failed to backup $DEFAULT_INITRD."
-                       rm -f "$DEFAULT_INITRD_BAK"
+                       rm -f -- "$INITRD_CHECKSUM_LOCATION"
+                       rm -f -- "$DEFAULT_INITRD_BAK"
                fi
        fi
 }
@@ -317,6 +318,7 @@ setup_initrd()
        fi

DEFAULT_INITRD_BAK="$KDUMP_BOOTDIR/.$(basename "$DEFAULT_INITRD").default"
+       INITRD_CHECKSUM_LOCATION="$DEFAULT_INITRD_BAK.checksum"
        if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
                TARGET_INITRD="$DEFAULT_INITRD"
> New patch to fix default initrd backup/restore logic.
>
> kdumpctl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 7caaae5..03c91f3 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -163,7 +163,7 @@ backup_default_initrd()
>    if [[ ! -e $DEFAULT_INITRD_BAK ]]; then
>    	dinfo "Backing up $DEFAULT_INITRD before rebuild."
>    	# save checksum to verify before restoring
> sha1sum "$DEFAULT_INITRD" > "$INITRD_CHECKSUM_LOCATION"
> sha1sum "$DEFAULT_INITRD" > "$INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER"
> if ! cp "$DEFAULT_INITRD" "$DEFAULT_INITRD_BAK"; then
> 	dwarn "WARNING: failed to backup $DEFAULT_INITRD."
> 	rm -f "$DEFAULT_INITRD_BAK"
>
> @@ -181,14 +181,14 @@ restore_default_initrd()
>
> # If a backup initrd exists, we must be switching back from
>    # fadump to kdump. Restore the original default initrd.
> if [[ -f $DEFAULT_INITRD_BAK ]] && [[ -f $INITRD_CHECKSUM_LOCATION ]]; then
> if [[ -f $DEFAULT_INITRD_BAK ]] && [[ -f $INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER ]]; then
> # verify checksum before restoring
> backup_checksum=$(sha1sum "$DEFAULT_INITRD_BAK" | awk '{ print $1 }')
> default_checksum=$(awk '{ print $1 }' "$INITRD_CHECKSUM_LOCATION")
> default_checksum=$(awk '{ print $1 }' "$INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER")
> if [[ $default_checksum != "$backup_checksum" ]]; then
> 	dwarn "WARNING: checksum mismatch! Can't restore original initrd.."
> else
> 	rm -f $INITRD_CHECKSUM_LOCATION
> 	rm -f $INITRD_CHECKSUM_LOCATION.$KDUMP_KERNELVER
> if mv "$DEFAULT_INITRD_BAK" "$DEFAULT_INITRD"; then
> 	derror "Restoring original initrd as fadump mode is disabled."
> 	sync -f "$DEFAULT_INITRD"

[PATCH v2 2/3] fadump: fix default initrd backup and restore logic

[Hari Bathini]

Hi Philipp,

Thanks for the review.

On 23/11/22 5:38 pm, Philipp Rudo wrote:
> Hi Hari,
>
> On Mon, 21 Nov 2022 17:48:01 +0530
> Hari Bathini  wrote:
> > In case of fadump, default initrd is rebuilt with dump capturing
> > capability, as the same initrd is used for booting production kernel
> > as well as capture kernel.
> >
> > The original initrd file is backed up with a checksum, to restore
> > it as the default initrd when fadump is disabled. As the checksum
> > file is not kernel version specific, switching between different
> > kernel versions and kdump/fadump dump mode breaks the default initrd
> > backup/restore logic. Fix this by having a kernel version specific
> > checksum file.
> >
> > Signed-off-by: Hari Bathini
>
> Thanks for taking care!
>
> Two small comments:
>
> in backup_default_initrd you should remove the checksum file in the
>  error case, when the initrd cannot be backed up (this should
>  probably be done today already...)
>
> why don't you simply update INITRD_CHECKSUM_OCATION one instead of
>  editing every line where it is used? For example consider the patch
>  below. I see multiple benefits (1) it's shorter, (2) the name shows
>  clearer to which initrd the checksum belongs to, (3) it reuses
>  KDUMP_BOOTDIR instead of hard coding /boot and (4)
>  INITRD_CHECKSUM_LOCATION now is basically local to setup_initrd now
>  making it easier for a clean up to remove the global variable later
>  on.
>
> What do you think?

Yeah. Makes it much better and simpler. Will respin with the change.

Thanks
Hari

[PATCH v2 3/3] fadump: add a kernel install hook to clean up fadump initramfs

[Hari Bathini]

Kdump service will create fadump initramfs when needed, but it won't
clean up the fadump initramfs on kernel uninstall. So create a kernel
install hook to do the clean up job.

Signed-off-by: Hari Bathini 
---

* Could have had 60-kdump.install.ppc64 that takes care of both kdump
  & fadump cleanup for ppc64 but that needs kdump cleanup code to be
  maintained in two files. So, preferred to go with a separate install
  file (60-fadump.install) instead.

Changes in v2:
* Added cleanup of kernel version based checksum file.

60-fadump.install | 32 ++++++++++++++++++++++++++++++++
 kexec-tools.spec  |  3 +++
 2 files changed, 35 insertions(+)
 create mode 100755 60-fadump.install

diff --git a/60-fadump.install b/60-fadump.install
new file mode 100755
index 0000000..c317d9c
--- /dev/null
+++ b/60-fadump.install
@@ -0,0 +1,32 @@
+#!/usr/bin/bash
+
+COMMAND="$1"
+KERNEL_VERSION="$2"
+
+if ! [[ ${KERNEL_INSTALL_MACHINE_ID-x} ]]; then
+    exit 0
+fi
+
+# Currently, fadump is supported only in environments with
+# writable /boot directory.
+if [[ ! -w "/boot" ]]; then
+    exit 0
+fi
+
+FADUMP_INITRD_DIR_ABS="/boot"
+FADUMP_INITRD=".initramfs-${KERNEL_VERSION}.img.default"
+FADUMP_INITRD_CHECKSUM=".fadump_initrd_checksum.${KERNEL_VERSION}"
+
+ret=0
+case "$COMMAND" in
+    add)
+        # Do nothing, fadump initramfs is strictly host only
+        # and managed by kdump service
+        ;;
+    remove)
+        rm -f -- "$FADUMP_INITRD_DIR_ABS/$FADUMP_INITRD"
+        rm -f -- "$FADUMP_INITRD_DIR_ABS/$FADUMP_INITRD_CHECKSUM"
+        ret=$?
+        ;;
+esac
+exit $ret
diff --git a/kexec-tools.spec b/kexec-tools.spec
index bfa7a5b..d4c3590 100644
--- a/kexec-tools.spec
+++ b/kexec-tools.spec
@@ -38,6 +38,7 @@ Source33: 92-crashkernel.install
 Source34: crashkernel-howto.txt
 Source35: kdump-migrate-action.sh
 Source36: kdump-restart.sh
+Source37: 60-fadump.install

#######################################
 # These are sources for mkdumpramfs
@@ -203,6 +204,7 @@ install -m 644 %{SOURCE13} $RPM_BUILD_ROOT%{_udevrulesdir}/98-kexec.rules
 %endif
 %ifarch ppc64 ppc64le
 install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_udevrulesdir}/98-kexec.rules
+install -m 755 -D %{SOURCE37} $RPM_BUILD_ROOT%{_prefix}/lib/kernel/install.d/60-fadump.install
 %endif
 install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
 install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
@@ -365,6 +367,7 @@ fi
 %endif
 %ifarch ppc64 ppc64le
 /usr/sbin/mkfadumprd
+%{_prefix}/lib/kernel/install.d/60-fadump.install
 %endif
 /usr/sbin/mkdumprd
 /usr/sbin/vmcore-dmesg

[PATCH v2 3/3] fadump: add a kernel install hook to clean up fadump initramfs

[Philipp Rudo]

Hi Hari,

On Mon, 21 Nov 2022 17:48:02 +0530
Hari Bathini  wrote:
> Kdump service will create fadump initramfs when needed, but it won't
> clean up the fadump initramfs on kernel uninstall. So create a kernel
> install hook to do the clean up job.
> Signed-off-by: Hari Bathini
> Could have had 60-kdump.install.ppc64 that takes care of both kdump
> & fadump cleanup for ppc64 but that needs kdump cleanup code to be
> maintained in two files. So, preferred to go with a separate install
> file (60-fadump.install) instead.

Besides the potential name change for the checksum.

Reviewed-by: Philipp Rudo
> Changes in v2:
> Added cleanup of kernel version based checksum file.
>
> 60-fadump.install | 32 ++++++++++++++++++++++++++++++++
>  kexec-tools.spec  |  3 +++
>  2 files changed, 35 insertions(+)
>  create mode 100755 60-fadump.install
>
> diff --git a/60-fadump.install b/60-fadump.install
> new file mode 100755
> index 0000000..c317d9c
> --- /dev/null
> +++ b/60-fadump.install
> @@ -0,0 +1,32 @@
> +#!/usr/bin/bash
>
> +COMMAND="$1"
> +KERNEL_VERSION="$2"
>
> +if ! [[ ${KERNEL_INSTALL_MACHINE_ID-x} ]]; then
> exit 0
>
> +fi
>
> +# Currently, fadump is supported only in environments with
> +# writable /boot directory.
> +if [[ ! -w "/boot" ]]; then
> exit 0
>
> +fi
>
> +FADUMP_INITRD_DIR_ABS="/boot"
> +FADUMP_INITRD=".initramfs-${KERNEL_VERSION}.img.default"
> +FADUMP_INITRD_CHECKSUM=".fadump_initrd_checksum.${KERNEL_VERSION}"
>
> +ret=0
> +case "$COMMAND" in
> add)
>    # Do nothing, fadump initramfs is strictly host only
>    # and managed by kdump service
>    ;;
> remove)
>    rm -f -- "$FADUMP_INITRD_DIR_ABS/$FADUMP_INITRD"
>    rm -f -- "$FADUMP_INITRD_DIR_ABS/$FADUMP_INITRD_CHECKSUM"
>    ret=$?
>    ;;
>
> +esac
> +exit $ret
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index bfa7a5b..d4c3590 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -38,6 +38,7 @@ Source33: 92-crashkernel.install
>  Source34: crashkernel-howto.txt
>  Source35: kdump-migrate-action.sh
>  Source36: kdump-restart.sh
> +Source37: 60-fadump.install
>
> #######################################
>  # These are sources for mkdumpramfs
> @@ -203,6 +204,7 @@ install -m 644 %{SOURCE13} $RPM_BUILD_ROOT%{_udevrulesdir}/98-kexec.rules
>  %endif
>  %ifarch ppc64 ppc64le
>  install -m 644 %{SOURCE14} $RPM_BUILD_ROOT%{_udevrulesdir}/98-kexec.rules
> +install -m 755 -D %{SOURCE37} $RPM_BUILD_ROOT%{_prefix}/lib/kernel/install.d/60-fadump.install
>  %endif
>  install -m 644 %{SOURCE15} $RPM_BUILD_ROOT%{_mandir}/man5/kdump.conf.5
>  install -m 644 %{SOURCE16} $RPM_BUILD_ROOT%{_unitdir}/kdump.service
> @@ -365,6 +367,7 @@ fi
>  %endif
>  %ifarch ppc64 ppc64le
>  /usr/sbin/mkfadumprd
> +%{_prefix}/lib/kernel/install.d/60-fadump.install
>  %endif
>  /usr/sbin/mkdumprd
>  /usr/sbin/vmcore-dmesg