On 07/11/2014 06:54 PM, Vivek Goyal wrote:
On Thu, Jun 12, 2014 at 08:26:15PM +0530, Hari Bathini wrote:
> The current kdump infrastructure builds a separate initrd which then
> gets loaded into memory by kexec-tools for use by kdump kernel. But
> firmware assisted dump (FADUMP) does not use kexec-based approach.
> After crash, firmware reboots the partition and loads grub loader
> like the normal booting process does. Hence in the FADUMP approach,
> the second kernel (after crash) will always use the default initrd
> (OS built). So, to support FADUMP, change is required, as in to add
> dump capturing steps, in this initrd.
>
> The current kdumpctl script implementation already has the code to
> build initrd using mkdumprd. This patch uses the new '--rebuild'
> option introduced, in dracut, to incrementally build the initramfs
> image. Before rebuilding, we may need to probe the initrd image for
> fadump support, to avoid rebuilding the initrd image multiple times
> unnecessarily. This can be done using "lsinitrd" tool with the newly
> proposed '--mod' option & inspecting the presence of
"kdumpbase" in
> the list of modules of default initrd image. We rebuild the image if
> only "kdumpbase" module is missing in the initrd image. Also, before
> rebuilding, a backup of default initrd image is taken.
>
> Kexec-tools package in rhel7 is now enhanced to insert a out-of-tree
> kdump module for dracut, which is responsible for adding vmcore
> capture steps into initrd, if dracut is invoked with "IN_KDUMP"
> environment variable set to 1. mkdumprd script exports "IN_KDUMP=1"
> environment variable before invoking dracut to build kdump initrd.
> This patch relies on this current mechanism of kdump init script.
>
> Patch that adds "--mod" option to lsinitrd tool is posted upstream,
> awaiting approval. Link for reference:
>
http://www.spinics.net/lists/linux-initramfs/msg03670.html
>
> Signed-off-by: Mahesh Salgaonkar <mahesh(a)linux.vnet.ibm.com>
> Signed-off-by: Hari Bathini <hbathini(a)linux.vnet.ibm.com>
> ---
> kdumpctl | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 4bc3952..e86bee0 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -9,6 +9,7 @@ MKDUMPRD="/sbin/mkdumprd -f"
> SAVE_PATH=/var/crash
> SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
> DUMP_TARGET=""
> +TARGET_INITRD=""
> FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled"
> FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"
> #kdump shall be the default dump mode
> @@ -133,13 +134,45 @@ function save_core()
> fi
> }
>
Hi Hari,
This patch is almost there. I have few more minor comments.
> -function rebuild_initrd()
> +rebuild_fadump_initrd()
> {
> - $MKDUMPRD $kdump_initrd $kdump_kver
> + # backup fadump initrd for reference before replacing it
> + backup_initrd
> +
> + target_initrd_tmp="$TARGET_INITRD.tmp"
All the local variables please define the at beginning of function, like.
local target_initrd_tmp
> + $MKDUMPRD $target_initrd_tmp --rebuild $TARGET_INITRD --kver $kdump_kver
> + if [ $? != 0 ]; then
> + echo "mkdumprd: failed to rebuild initrd with fadump support"
>&2
> + return 1
> + fi
> +
> + # updating fadump initrd
> + mv $target_initrd_tmp $TARGET_INITRD
> + sync
This system wide sync might harm us at some point of time. For the time
being it is fine. But ideally send a patch to util-linux maintainer and
write a small "fsync" utility and call "fsync $TARGET_INITRD". I
heard
that some unix variant had "fsync" as stand alone program.
Will try to work on this. Adding a TODO for the time being..
> +
> + return 0
> +}
> +
> +rebuild_kdump_initrd()
> +{
> + $MKDUMPRD $TARGET_INITRD $kdump_kver
> if [ $? != 0 ]; then
> echo "mkdumprd: failed to make kdump initrd" >&2
> return 1
> fi
> +
> + return 0
> +}
> +
> +rebuild_initrd()
> +{
> + if [ $DEFAULT_DUMP_MODE == "fadump" ]; then
> + rebuild_fadump_initrd
> + else
> + rebuild_kdump_initrd
> + fi
> +
> + return $?
> }
>
> #$1: the files to be checked with IFS=' '
> @@ -164,6 +197,17 @@ function check_executable()
> done
> }
>
> +backup_initrd()
> +{
> + target_initrd_bak="$TARGET_INITRD.default.bak"
Why not just "$TARGET_INITRD.bak". I am wondering what's the purpose of
"default" string.
> +
> + # Check if backup initrd is already present.
> + if [ ! -e $target_initrd_bak ];then
> + echo "Backing up $TARGET_INITRD"
> + cp $TARGET_INITRD $target_initrd_bak
> + fi
So this backup happens only if backup is not already present?
Ideally, want to take a backup the first time fadump is used
and be done with it..
> +}
> +
> function check_config()
> {
> local nr
> @@ -242,7 +286,17 @@ function check_rebuild()
> fi
>
>
kdump_kernel="${KDUMP_BOOTDIR}/${KDUMP_IMG}-${kdump_kver}${KDUMP_IMG_EXT}"
> - kdump_initrd="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}kdump.img"
> + if [ $DEFAULT_DUMP_MODE == "fadump" ]; then
> + TARGET_INITRD="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}.img"
> + if [ ! -s "$TARGET_INITRD" ]; then
> + echo "Error: No initrd found to rebuild!"
> + return 1
> + fi
> + fadump_support=`lsinitrd -m $TARGET_INITRD | grep ^kdumpbase$ | wc -l`
fadump_support name is little confusing. How about "initramfs_has_fadump".
Also define this variable as "local" in the beginning of function.
> + else
> + TARGET_INITRD="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}kdump.img"
> + fadump_support=-1
I don't think this is required. See below.
> + fi
Can we move above logic in a separate function, say setup_target_initrd().
And in that function you can also do error handling if default intird is
not found. In parent function check for return code and bail out.
>
> _force_rebuild=`grep ^force_rebuild $KDUMP_CONFIG_FILE 2>/dev/null`
> if [ $? -eq 0 ]; then
> @@ -259,8 +313,8 @@ function check_rebuild()
>
> #check to see if dependent files has been modified
> #since last build of the image file
> - if [ -f $kdump_initrd ]; then
> - image_time=`stat -c "%Y" $kdump_initrd 2>/dev/null`
> + if [ -f $TARGET_INITRD ]; then
> + image_time=`stat -c "%Y" $TARGET_INITRD 2>/dev/null`
> else
> image_time=0
> fi
> @@ -287,8 +341,10 @@ function check_rebuild()
>
> if [ $image_time -eq 0 ]; then
> echo -n "No kdump initial ramdisk found."; echo
> + elif [ "$fadump_support" -eq "0" ]; then
This check could be combination of two checks. Something like.
elif [ DUMP_MODE == fadump && !initramfs_has_fadump ]
This is much more readable.
Sure. Will update accordingly..
> + echo "$TARGET_INITRD has no fadump support"
> elif [ "$force_rebuild" != "0" ]; then
> - echo -n "Force rebuild $kdump_initrd"; echo
> + echo -n "Force rebuild $TARGET_INITRD"; echo
> elif [ -n "$modified_files" ]; then
> echo "Detected change(s) the following file(s):"
> echo -n " "; echo "$modified_files" | sed 's/\s/\n
/g'
> @@ -296,7 +352,7 @@ function check_rebuild()
> return 0
> fi
>
> - echo "Rebuilding $kdump_initrd"
> + echo "Rebuilding $TARGET_INITRD"
> rebuild_initrd
> return $?
> }
> @@ -349,7 +405,7 @@ function load_kdump()
>
> $KEXEC $KEXEC_ARGS $standard_kexec_args \
> --command-line="$KDUMP_COMMANDLINE" \
> - --initrd=$kdump_initrd $kdump_kernel 2>/dev/null
> + --initrd=$TARGET_INITRD $kdump_kernel 2>/dev/null
> if [ $? == 0 ]; then
> echo "kexec: loaded kdump kernel"
> return 0
Thanks
Vivek