On Wed, Mar 27, 2019 at 4:34 PM Hari Bathini <hbathini(a)linux.ibm.com> wrote:
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> 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>
>>>>> ---
>>>>> 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..
Yes I had some misunderstanding, please ignore my concern about reload...
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
This looks good, I'll update V2, thanks!
--
Best Regards,
Kairui Song