[PATCH 0/2] fadump improvements

[Hari Bathini]

This set tries to do a couple of improvements to fadump. The 1st patch
ensures fadump initramfs is cleaned up on kernel uninstall. The 2nd
patch makes 'zstd' as the default compression method for fadump.

Hari Bathini (2):
  fadump: add a kernel install hook to clean up fadump initramfs
  fadump: use 'zstd' as the default compression method

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

[PATCH 1/2] 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 
---
 60-fadump.install | 30 ++++++++++++++++++++++++++++++
 kexec-tools.spec  |  3 +++
 2 files changed, 33 insertions(+)
 create mode 100755 60-fadump.install

diff --git a/60-fadump.install b/60-fadump.install
new file mode 100755
index 0000000..70fcd27
--- /dev/null
+++ b/60-fadump.install
@@ -0,0 +1,30 @@
+#!/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"
+
+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"
+        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 1/2] fadump: add a kernel install hook to clean up fadump initramfs

[Philipp Rudo]

Hi Hari,

it totally makes sense to have some functionality like this. What I'm
not sure about is if it would make more sense to include it into
60-kdump.install. With that the patch would reduce to something like
this

--- a/60-kdump.install
+++ b/60-kdump.install
@@ -22,6 +22,8 @@ else
     KDUMP_INITRD="initramfs-${KERNEL_VERSION}kdump.img"
 fi

+KDUMP_INITRD_BAK=".initramfs-${KERNEL_VERSION}.img.default"
+
 ret=0
 case "$COMMAND" in
     add)
@@ -29,7 +31,8 @@ case "$COMMAND" in
         # and managed by kdump service
         ;;
     remove)
-        rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD"
+        rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD" || exit
+        rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD_BAK"
         ret=$?
         ;;
 esac

Does that work for you? Or is there a reason KDUMP_INITRD_DIR_ABS must
not be anything else than /boot?

One more thing I noticed was the handling of INITRD_CHECKSUM_LOCATION.
First I thought this should be removed as well. But then I noticed that
there is only one instance for the last kernel which gets overwritten
when a new kernel is installed. For example assume the following

1. install kernel1
2. boot kernel1 and enable fadump
3. install kernel2
4. boot kernel2 and enable fadump
5. boot kernel1 and disable fadump

In this scenario step 4 will create a backup initrd for kernel2 and
overwrites INITRD_CHECKSUM_LOCATION without removing the backup initrd
for kernel1. Consequently step 5 will try to restore the backup initrd
for kernel1 but will fail as the checksum no longer matches. Is that
correct? Or am I missing something?

If I'm correct I think it would make sense to make
INITRD_CHECKSUM_LOCATION version specific and remove it together with
the backup initrd.

Thanks
Philipp

On Fri, 11 Nov 2022 20:13:05 +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
>
> 60-fadump.install | 30 ++++++++++++++++++++++++++++++
>  kexec-tools.spec  |  3 +++
>  2 files changed, 33 insertions(+)
>  create mode 100755 60-fadump.install
>
> diff --git a/60-fadump.install b/60-fadump.install
> new file mode 100755
> index 0000000..70fcd27
> --- /dev/null
> +++ b/60-fadump.install
> @@ -0,0 +1,30 @@
> +#!/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"
>
> +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"
>    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 1/2] fadump: add a kernel install hook to clean up fadump initramfs

[Hari Bathini]

On 14/11/22 6:56 pm, Philipp Rudo wrote:
> Hi Hari,
>
> it totally makes sense to have some functionality like this. What I'm
> not sure about is if it would make more sense to include it into
> 60-kdump.install. With that the patch would reduce to something like
> this
>
> --- a/60-kdump.install
> +++ b/60-kdump.install
> @@ -22,6 +22,8 @@ else
>       KDUMP_INITRD="initramfs-${KERNEL_VERSION}kdump.img"
>   fi
>
> +KDUMP_INITRD_BAK=".initramfs-${KERNEL_VERSION}.img.default"
> ret=0
> case "$COMMAND" in
>     add)
>
> @@ -29,7 +31,8 @@ case "$COMMAND" in
>           # and managed by kdump service
>           ;;
>       remove)
>    rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD"
>    rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD" || exit
>    rm -f -- "$KDUMP_INITRD_DIR_ABS/$KDUMP_INITRD_BAK"
>     ret=$?
>     ;;
> esac
>
> Does that work for you? Or is there a reason KDUMP_INITRD_DIR_ABS must
> not be anything else than /boot?

Yeah. I don't see fadump supported in any other case than /boot
> One more thing I noticed was the handling of INITRD_CHECKSUM_LOCATION.
> First I thought this should be removed as well. But then I noticed that
> there is only one instance for the last kernel which gets overwritten
> when a new kernel is installed. For example assume the following
> install kernel1
> boot kernel1 and enable fadump
> install kernel2
> boot kernel2 and enable fadump
> boot kernel1 and disable fadump
>
> In this scenario step 4 will create a backup initrd for kernel2 and
> overwrites INITRD_CHECKSUM_LOCATION without removing the backup initrd
> for kernel1. Consequently step 5 will try to restore the backup initrd
> for kernel1 but will fail as the checksum no longer matches. Is that
> correct? Or am I missing something?
>
> If I'm correct I think it would make sense to make
> INITRD_CHECKSUM_LOCATION version specific and remove it together with
> the backup initrd.

Right. This sequence breaks the restore logic for older kernel version
Will repost fixing this..

Coiby, I will follow-up on this . Meanwhile, can you consider picking
patch#2

Thanks
Hari

[PATCH 2/2] fadump: use 'zstd' as the default compression method

[Hari Bathini]

If available, use 'zstd' compression method to optimize size of the
initramfs built with fadump support.

Signed-off-by: Hari Bathini 
---
 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 2/2] fadump: use 'zstd' as the default compression method

[Tao Liu]

Hi Hari

On Fri, Nov 11, 2022 at 10:43 PM Hari Bathini  wrote:
> If available, use 'zstd' compression method to optimize size of the
> initramfs built with fadump support.
> Signed-off-by: Hari Bathini
>
> 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

I didn't understand why only consider the case of zstd cmd, don't we
use zstd format squashfs for the fadump case? If possible, could you
please append the reason at the commit log for future reference?
Thanks a lot!

Thanks,
Tao Liu
>   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 2/2] fadump: use 'zstd' as the default compression method

[Hari Bathini]

On 12/11/22 8:03 am, Tao Liu wrote:
> Hi Hari
>
> On Fri, Nov 11, 2022 at 10:43 PM Hari Bathini  wrote:
> > If available, use 'zstd' compression method to optimize size of the
> > initramfs built with fadump support.
> > Signed-off-by: Hari Bathini
> >
> > 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
>
> I didn't understand why only consider the case of zstd cmd, don't we
> use zstd format squashfs for the fadump case? If possible, could you
> please append the reason at the commit log for future reference?

Observed better space optimization with zstd instead of squash+zstd
most likely due to the additional binary requirement for squash case..

Thanks
Hari

[PATCH 2/2] fadump: use 'zstd' as the default compression method

[Tao Liu]

Hi Hari,

On Mon, Nov 14, 2022 at 3:13 PM Hari Bathini  wrote:
> On 12/11/22 8:03 am, Tao Liu wrote:
> > Hi Hari
> >
> > On Fri, Nov 11, 2022 at 10:43 PM Hari Bathini  wrote:
> > > If available, use 'zstd' compression method to optimize size of the
> > > initramfs built with fadump support.
> > > Signed-off-by: Hari Bathini
> > >
> > > 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
> >
> > I didn't understand why only consider the case of zstd cmd, don't we
> > use zstd format squashfs for the fadump case? If possible, could you
> > please append the reason at the commit log for future reference?
>
> Observed better space optimization with zstd instead of squash+zstd
> most likely due to the additional binary requirement for squash case..

OK, thanks for the clarification!

Thanks,
Tao Liu
> Thanks
> Hari

[PATCH 2/2] fadump: use 'zstd' as the default compression method

[Hari Bathini]

On 14/11/22 12:43 pm, Hari Bathini wrote:
> On 12/11/22 8:03 am, Tao Liu wrote:
> > Hi Hari
> >
> > On Fri, Nov 11, 2022 at 10:43 PM Hari Bathini  
> > wrote:
> > > If available, use 'zstd' compression method to optimize size of the
> > > initramfs built with fadump support.
> > > Signed-off-by: Hari Bathini
> > >
> > > 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
> >
> > I didn't understand why only consider the case of zstd cmd, don't we
> > use zstd format squashfs for the fadump case? If possible, could you
> > please append the reason at the commit log for future reference?

fwiw, posted v2 with the updated changelog.

Thanks
Hari