On 07/07/17 at 11:24am, Xunlei Pang wrote:
> On 07/07/2017 at 10:52 AM, Dave Young wrote:
>> On 07/07/17 at 10:43am, Dave Young wrote:
>>> Hi Xunlei,
>>> On 07/06/17 at 02:37pm, Xunlei Pang wrote:
>>>> Resolves: bz1451717
>>>>
https://bugzilla.redhat.com/1451717
>>>>
>>>> When there is no crypt related kdump target, we can safely
>>>> omit "crypt" dracut module, this can avoid the pop asking
>>>> disk password during kdump boot in some cases.
>>>>
>>>> This patch introduces omit_dracut_modules() before calling
>>>> dracut, we can omit more modules to reduce initrd size in
>>>> the future.
>>>>
>>>> Signed-off-by: Xunlei Pang <xlpang(a)redhat.com>
>>>> ---
>>>> kdump-lib.sh | 12 ++++++++++++
>>>> kdumpctl | 12 ------------
>>>> mkdumprd | 26 ++++++++++++++++++++++++++
>>>> 3 files changed, 38 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/kdump-lib.sh b/kdump-lib.sh
>>>> index a33f172..3f0af91 100755
>>>> --- a/kdump-lib.sh
>>>> +++ b/kdump-lib.sh
>>>> @@ -6,6 +6,7 @@
>>>> DEFAULT_PATH="/var/crash/"
>>>> FENCE_KDUMP_CONFIG_FILE="/etc/sysconfig/fence_kdump"
>>>> FENCE_KDUMP_SEND="/usr/libexec/fence_kdump_send"
>>>> +FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled"
>>>>
>>>> perror_exit() {
>>>> echo $@ >&2
>>>> @@ -481,3 +482,14 @@ get_dracut_args_target()
>>>> {
>>>> echo $1 | grep "\-\-mount" | sed "s/.*--mount
.\(.*\)/\1/" | cut -d' ' -f1
>>>> }
>>>> +
>>>> +is_fadump_capable()
>>>> +{
>>>> + # Check if firmware-assisted dump is enabled
>>>> + # if no, fallback to kdump check
>>>> + if [ -f $FADUMP_ENABLED_SYS_NODE ]; then
>>>> + rc=`cat $FADUMP_ENABLED_SYS_NODE`
>>>> + [ $rc -eq 1 ] && return 0
>>>> + fi
>>>> + return 1
>>>> +}
>>>> diff --git a/kdumpctl b/kdumpctl
>>>> index 2783e23..2f158fc 100755
>>>> --- a/kdumpctl
>>>> +++ b/kdumpctl
>>>> @@ -13,7 +13,6 @@ DUMP_TARGET=""
>>>> DEFAULT_INITRD=""
>>>> DEFAULT_INITRD_BAK=""
>>>> 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
>>>> DEFAULT_DUMP_MODE="kdump"
>>>> @@ -932,17 +931,6 @@ handle_mode_switch()
>>>> fi
>>>> }
>>>>
>>>> -is_fadump_capable()
>>>> -{
>>>> - # Check if firmware-assisted dump is enabled
>>>> - # if no, fallback to kdump check
>>>> - if [ -f $FADUMP_ENABLED_SYS_NODE ]; then
>>>> - rc=`cat $FADUMP_ENABLED_SYS_NODE`
>>>> - [ $rc -eq 1 ] && return 0
>>>> - fi
>>>> - return 1
>>>> -}
>>>> -
>>>> check_current_fadump_status()
>>>> {
>>>> # Check if firmware-assisted dump has been registered.
>>>> diff --git a/mkdumprd b/mkdumprd
>>>> index 6956d8e..501a689 100644
>>>> --- a/mkdumprd
>>>> +++ b/mkdumprd
>>>> @@ -374,6 +374,30 @@ check_crypt()
>>>> return 1
>>>> }
>>>>
>>>> +omit_dracut_modules()
>>>> +{
>>>> + local target majmin
>>>> + local crypt_exists
>>>> +
>>>> + # Skip fadump case
>>>> + is_fadump_capable && return
>>>> +
>>>> + crypt_exists=0
>>>> +
>>>> + for target in $(get_kdump_targets); do
>>>> + if [ -b "$target" ]; then
>>>> + # Check "crypt"
>>>> + majmin=$(get_maj_min $target)
>>>> + check_block_and_slaves is_crypt $majmin &&
crypt_exists=1
>>>> + fi
>>>> + done
>>> We have for_each_block_target adapted with your get_dump_targets so
>>> Can we use check_crypt function instead?
>>>
>>> Something like
>>>
>>> omit_dracut_module()
>>> {
>>> if is_fadump then return
>>>
>>> add_dracut_arg "--omit" "crypt"
>>> }
>>>
>>>
>>> Then we can call it in mkdumprd, like below then we do not iternate the
>>> devices again, can save some boot time:
>>>
>>> if ! check_crypt; then
>>> echo "Warning: Encrypted device is in dump path. User will prompted
>>> for password during second kernel boot."
>>> else
>>> omit_dracut_module crypt
>>> fi
>> Or maybe save the status of crypt as a global variable and check it in
>> your current omit_dracut_modules() version..
> Yes, we can, my thought was to handle them uniformly in omit_dracut_modules(),
> that's why I appended Patch 6~7 which actually should be in a separate series.
>
> When we reach here, it does initramfs rebuild, the time it consumes should be
> negligible compared to the total rebuild time.
>
> But I am also fine with this global crypt status variable approach, it looks
harmless.
Yes, a global status var is better then my reply to omit module multiple
times..