Ccing Hari for the dump mode issue. On 03/13/19 at 08:28pm, Kairui Song wrote: > Use "kdumpctl rebuild" to rebuild the image directly. This could help > admins to rebuild kdump image directly. > > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; > --- > kdumpctl | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/kdumpctl b/kdumpctl > index 1cfbe31..e223224 100755 > --- a/kdumpctl > +++ b/kdumpctl > @@ -1141,6 +1141,27 @@ stop() > return 0 > } > > +rebuild() { > + setup_initrd > + if [ $? -ne 0 ]; then > + return 1 > + fi > + > + if [[ ! -w "$KDUMP_BOOTDIR" ]];then > + echo "$KDUMP_BOOTDIR does not have write permission. Can not rebuild $TARGET_INITRD" > + return 1 > + fi > + > + handle_mode_switch > + if [ $? -ne 0 ]; then > + return 1 > + fi Maybe handle_mode_switch can be moved to rebuild_initrd() then we do not need it in this function.

Hi Hari, On 03/26/19 at 11:23pm, Hari Bathini wrote: > Hi Dave, > > > On 25/03/19 11:17 AM, Dave Young wrote: > > Ccing Hari for the dump mode issue. > > > > On 03/13/19 at 08:28pm, Kairui Song wrote: > > > Use "kdumpctl rebuild" to rebuild the image directly. This could help > > > admins to rebuild kdump image directly. > > > > > > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; > > > --- > > > kdumpctl | 26 +++++++++++++++++++++++++- > > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/kdumpctl b/kdumpctl > > > index 1cfbe31..e223224 100755 > > > --- a/kdumpctl > > > +++ b/kdumpctl > > > @@ -1141,6 +1141,27 @@ stop() > > > return 0 > > > } > > > +rebuild() { > > > + setup_initrd > > > + if [ $? -ne 0 ]; then > > > + return 1 > > > + fi > > > + > > > + if [[ ! -w "$KDUMP_BOOTDIR" ]];then > > > + echo "$KDUMP_BOOTDIR does not have write permission. Can not rebuild $TARGET_INITRD" > > > + return 1 > > > + fi > > > + > > > + handle_mode_switch > > > + if [ $? -ne 0 ]; then > > > + return 1 > > > + fi > > Maybe handle_mode_switch can be moved to rebuild_initrd() > > then we do not need it in this function. > > IIUC, remove handle_mode_switch() function call here and check_rebuild() functions and > call it from rebuild_initrd() instead..? That change is likely to ignore scenario where > a rebuild is not necessary (no change in /etc/kdump.conf file) but a restore is needed > when we switch from FADump to KDump? We may eventually restore when initrd is rebuilt > though... > Hmm, then maybe move it to setup_initrd? In setup_initrd, we have the if else for dump mode then we can just move to backup/restore there and drop this handle_mode_switch function. And the -w -w "$KDUMP_BOOTDIR" can be moved to rebuild_initrd(), kairui?

On 27/03/19 12:12 PM, Kairui Song wrote: > On Wed, Mar 27, 2019 at 11:28 AM Dave Young <dyoung(a)redhat.com&gt; wrote: >> Hi Hari, >> On 03/26/19 at 11:23pm, Hari Bathini wrote: >>> Hi Dave, >>> >>> >>> On 25/03/19 11:17 AM, Dave Young wrote: >>>> Ccing Hari for the dump mode issue. >>>> >>>> On 03/13/19 at 08:28pm, Kairui Song wrote: >>>>> Use "kdumpctl rebuild" to rebuild the image directly. This could help >>>>> admins to rebuild kdump image directly. >>>>> >>>>> Signed-off-by: Kairui Song <kasong(a)redhat.com&gt; >>>>> --- >>>>> kdumpctl | 26 +++++++++++++++++++++++++- >>>>> 1 file changed, 25 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/kdumpctl b/kdumpctl >>>>> index 1cfbe31..e223224 100755 >>>>> --- a/kdumpctl >>>>> +++ b/kdumpctl >>>>> @@ -1141,6 +1141,27 @@ stop() >>>>> return 0 >>>>> } >>>>> +rebuild() { >>>>> + setup_initrd >>>>> + if [ $? -ne 0 ]; then >>>>> + return 1 >>>>> + fi >>>>> + >>>>> + if [[ ! -w "$KDUMP_BOOTDIR" ]];then >>>>> + echo "$KDUMP_BOOTDIR does not have write permission. Can not rebuild $TARGET_INITRD" >>>>> + return 1 >>>>> + fi >>>>> + >>>>> + handle_mode_switch >>>>> + if [ $? -ne 0 ]; then >>>>> + return 1 >>>>> + fi >>>> Maybe handle_mode_switch can be moved to rebuild_initrd() >>>> then we do not need it in this function. >>> IIUC, remove handle_mode_switch() function call here and check_rebuild() functions and >>> call it from rebuild_initrd() instead..? That change is likely to ignore scenario where >>> a rebuild is not necessary (no change in /etc/kdump.conf file) but a restore is needed >>> when we switch from FADump to KDump? We may eventually restore when initrd is rebuilt >>> though... >>> >> Hmm, then maybe move it to setup_initrd? In setup_initrd, we have the >> if else for dump mode then we can just move to backup/restore there and >> drop this handle_mode_switch function. >> >> And the -w -w "$KDUMP_BOOTDIR" can be moved to rebuild_initrd(), kairui? > Hi, Dave, just one thing, move handle_mode_switch to setup_initrd this > will make "kdumpctl reload" possible trigger a backup / restore but > don't rebuild anything, doesn't seems very reasonable. > > Currently kdumpctl reload is not behaving very well on mode switch Can you share the sequence in which this is happening.. > anyway (may fail silently on normal->fadump switch or error out on I think backup & restore functions don't return any error explicitly.. > fadump->normal), but after all user should really trigger a rebuild Restore may error out only when `sync` fails and if `sync` fails, admin has filesystem related issues to sort out first..

With commit da6b75f59bdd ("fadump: use the original initrd to rebuild fadump initrdfrom"), error check in setup_initrd() is no longer needed. So, I think the below snippet is not a bad idea based on Dave's suggestion: -- diff --git a/kdumpctl b/kdumpctl index 1cfbe31..1dd7fa3 100755 --- a/kdumpctl +++ b/kdumpctl @@ -298,12 +298,17 @@ setup_initrd() DEFAULT_INITRD_BAK="${KDUMP_BOOTDIR}/.initramfs-`uname -r`.img.default" if [ $DEFAULT_DUMP_MODE == "fadump" ]; then TARGET_INITRD="$DEFAULT_INITRD" - if [ ! -s "$TARGET_INITRD" ]; then - echo "Error: No initrd found to rebuild!" - return 1 - fi + + # backup initrd for reference before replacing it + # with fadump aware initrd + backup_default_initrd else TARGET_INITRD="${KDUMP_BOOTDIR}/initramfs-${kdump_kver}kdump.img" + + # check if a backup of default initrd exists. If yes, + # it signifies a switch from fadump mode. So, restore + # the backed up default initrd. + restore_default_initrd fi } @@ -602,8 +607,6 @@ check_rebuild() system_modified="1" fi - handle_mode_switch - if [ $image_time -eq 0 ]; then echo -n "No kdump initial ramdisk found."; echo elif [ "$capture_capable_initrd" == "0" ]; then @@ -745,20 +748,6 @@ show_reserved_mem() echo "Reserved "$mem_mb"MB memory for crash kernel" } -handle_mode_switch() -{ - if [ "$DEFAULT_DUMP_MODE" == "fadump" ]; then - # backup initrd for reference before replacing it - # with fadump aware initrd - backup_default_initrd - else - # check if a backup of default initrd exists. If yes, - # it signifies a switch from fadump mode. So, restore - # the backed up default initrd. - restore_default_initrd - fi -} - check_current_fadump_status() { # Check if firmware-assisted dump has been registered. -- Thanks Hari