Consider block device in the special "--dracut-args --mount ..." in get_user_configured_dump_disk(). Consider save path instead of only root fs in get_block_dump_target(), and move it into kdump-lib.sh, becuase we will use it in kdumpctl. We did not add it, because get_block_dump_target() is only used by check_resettable/check_crypt and users are expected to make to work for the special "--dracut-args --mount ...", anyway there is nothing wrong we add it in. We will have another user in the next patch. Moreover, for nfs/ssh dumping, we do not need check the root device. Move get_save_path into kdump-lib.sh. After this patch, get_block_dump_target() can always return our correct block dump target specified. Signed-off-by: Xunlei Pang <xlpang(a)redhat.com&gt; --- kdump-lib.sh | 44 ++++++++++++++++++++++++++++++++++++-------- kdumpctl | 10 ---------- mkdumprd | 13 ------------- 3 files changed, 36 insertions(+), 31 deletions(-) diff --git a/kdump-lib.sh b/kdump-lib.sh index 8ebad70..ae54b7f 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -44,12 +44,6 @@ is_fs_dump_target() egrep -q "^ext[234]|^xfs|^btrfs|^minix" /etc/kdump.conf } -is_user_configured_dump_target() -{ - return $(is_mount_in_dracut_args || is_ssh_dump_target || is_nfs_dump_target || \ - is_raw_dump_target || is_fs_dump_target) -} - strip_comments() { echo $@ | sed -e 's/\(.*\)#.*/\1/' @@ -88,6 +82,12 @@ to_dev_name() { echo $dev } +is_user_configured_dump_target() +{ + return $(is_mount_in_dracut_args || is_ssh_dump_target || is_nfs_dump_target || \ + is_raw_dump_target || is_fs_dump_target) +} + get_user_configured_dump_disk() { local _target @@ -97,9 +97,10 @@ get_user_configured_dump_disk() fi _target=$(egrep "^ext[234]|^xfs|^btrfs|^minix|^raw" /etc/kdump.conf 2>/dev/null |awk '{print $2}') - [ -n "$_target" ] && echo $_target + [ -n "$_target" ] && echo $_target && return - return + _target=$(get_dracut_args_target "$(grep "^dracut_args .*\-\-mount" /etc/kdump.conf)") + [ -b "$_target" ] && echo $_target } get_root_fs_device() @@ -111,6 +112,33 @@ get_root_fs_device() return } +get_save_path() +{ + local _save_path=$(grep "^path" /etc/kdump.conf|awk '{print $2}') + if [ -z "$_save_path" ]; then + _save_path=$DEFAULT_PATH + fi + + echo $_save_path +} + +get_block_dump_target() +{ + local _target _path + + _target=$(get_user_configured_dump_disk) + [ -n "$_target" ] && echo $(to_dev_name $_target) && return + + if is_ssh_dump_target || is_nfs_dump_target; then + return + fi + + # Get block device name from local save path + _path=$(get_save_path) + _target=$(get_target_from_path $_path) + [ -b "$_target" ] && echo $(to_dev_name $_target) +} + # findmnt uses the option "-v, --nofsroot" to exclusive the [/dir] # in the SOURCE column for bind-mounts, then if $_mntpoint equals to # $_mntpoint_nofsroot, the mountpoint is not bind mounted directory. diff --git a/kdumpctl b/kdumpctl index 4d68be0..d8f049f 100755 --- a/kdumpctl +++ b/kdumpctl @@ -930,16 +930,6 @@ save_raw() return 0 } -get_save_path() -{ - local _save_path=$(grep "^path" /etc/kdump.conf|awk '{print $2}') - if [ -z "$_save_path" ]; then - _save_path="/var/crash" - fi - - echo $_save_path -} - is_dump_target_configured() { local _target diff --git a/mkdumprd b/mkdumprd index f30d9c2..426149b 100644 --- a/mkdumprd +++ b/mkdumprd @@ -256,19 +256,6 @@ add_mount() { fi } -get_block_dump_target() -{ - local _target - - - _target=$(get_user_configured_dump_disk) - [ -n "$_target" ] && echo $(to_dev_name $_target) && return - - #get rootfs device name - _target=$(get_root_fs_device) - [ -b "$_target" ] && echo $(to_dev_name $_target) -} - #handle the case user does not specify the dump target explicitly handle_default_dump_target() { -- 1.8.3.1

For the following scripts, cmdline="root=/dev/mapper/rhel-root rd.lvm.lv=rhel/root quiet" remove_cmdline_param $cmdline "root" we will get the result "rd.lvm.lv=rhel/ quiet", after this patch we can get the correct "rd.lvm.lv=rhel/root quiet". Signed-off-by: Xunlei Pang <xlpang(a)redhat.com&gt; --- kdumpctl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kdumpctl b/kdumpctl index ca97580..e9dc8e5 100755 --- a/kdumpctl +++ b/kdumpctl @@ -75,7 +75,8 @@ remove_cmdline_param() for arg in $@; do cmdline=`echo $cmdline | \ sed -e "s/\b$arg=[^ ]*\b//g" \ - -e "s/\b$arg\b//g" \ + -e "s/^$arg\b//g" \ + -e "s/[[:space:]]$arg\b//g" \ -e "s/\s\+/ /g"` done echo $cmdline -- 1.8.3.1

We can easily achieve this by adding a "any_lvm" job in the KDUMP_TARGET_CHECKLIST. We need this, because for example in case of "nfs"(and root device is not a lvm volmue) dumping, if "lvm" module is added, "lvm2" tool under kdump still operates the lvm volume due to the "rd.lvm.lv=X" inherited from the first kernel if any. This patch removes "lvm" and all the related cmdline from the kdump/initramfs in case of no lvm target, it can reduce several megabytes from initramfs. Signed-off-by: Xunlei Pang <xlpang(a)redhat.com&gt; --- kdumpctl | 15 ++++++++++++++- mkdumprd | 5 +++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/kdumpctl b/kdumpctl index 1cb6f36..8a0c400 100755 --- a/kdumpctl +++ b/kdumpctl @@ -27,7 +27,7 @@ image_time=0 # implement its logic to update its tag value in the tag inventory. # Then "kdump_targets_tell xxx" will be available for you to use. # See kdump_target_handle_item() for examples. -KDUMP_TARGET_CHECKLIST="all_root" +KDUMP_TARGET_CHECKLIST="all_root any_lvm" standard_kexec_args="-p" @@ -216,6 +216,11 @@ prepare_cmdline() cmdline=`remove_cmdline_param "$cmdline" root` fi + # Remove all the lvm related cmdlines to avoid confusion if no lvm targets + if ! kdump_targets_tell any_lvm; then + cmdline=`remove_cmdline_param "$cmdline" rd.lvm.lv rd_LVM_LV rd.lvm.vg rd_LVM_VG` + fi + KDUMP_COMMANDLINE=$cmdline check_kdump_cpus @@ -515,6 +520,14 @@ kdump_target_handle_item() value=1 target_is_root $dev || value=0 + any_lvm) + # Sticky value is 1 + [[ $value = "1" ]] && return + + value=0 + if [[ $dev =~ "/dev/mapper/" ]]; then + value=1 + fi ;; *) echo "unhandled item in kdump target checklist" diff --git a/mkdumprd b/mkdumprd index d27bf94..92d68a3 100644 --- a/mkdumprd +++ b/mkdumprd @@ -495,6 +495,11 @@ then add_dracut_arg "--add-drivers" "$extra_modules" fi +# Omit "lvm" dracut module if we have no lvm targets. +if ! kdump_targets_tell any_lvm ; then + add_dracut_arg "-o" "lvm" +fi + dracut "${dracut_args[@]}" "$@" _rc=$? sync -- 1.8.3.1

On 03/27/2017 at 10:42 AM, Xunlei Pang wrote: Currently, I missed the diskless case, I need some discussion about the diskless environment. For diskless systems, will "findmnt /" always return the "nfs" mount information? Regards, Xunlei

Hi Xunlei, On Monday 27 March 2017 08:12 AM, Xunlei Pang wrote: I have not seen the next patches so far, however I tried to read this patch again and again but I could not understand why we are doing all these, what problem we are trying to address. Moreover, patch is not just adding a generic hook, it is modifying existing functionality of check_files_modified(). IMO, it would again be good to divide it in multiple patches, with clear explanation that why do we do that. Even cover letter also does not talk about overall what we try to achieve with the series. ~Pratyush > kdump_targets_tell() is exported as a generic helper that can > be used as some specific purpose. > > To implement a new checklist: > Add your new checklist(job) "xxx" separated by whitespace, and > implement its logic to update its tag value in the tag inventory. > Then "kdump_targets_tell xxx" will be available for you to use. > > We will use this mechanism in the following patches. > > Signed-off-by: Xunlei Pang <xlpang(a)redhat.com&gt; > --- > kdump-lib.sh | 28 +++++++++++++++ > kdumpctl | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 144 insertions(+) > > diff --git a/kdump-lib.sh b/kdump-lib.sh > index ae54b7f..d3c0d7b 100755 > --- a/kdump-lib.sh > +++ b/kdump-lib.sh > @@ -434,3 +434,31 @@ get_dracut_args_target() > { > echo $1 | grep "\-\-mount" | sed "s/.*--mount .\(.*\)/\1/" | cut -d' ' -f1 > } > + > +# $1: item > +# Return the item tag value in the inventory. > +kdump_target_get_inventory() > +{ > + local item=$1 tag > + > + tag=$(eval echo '${KDUMP_TARGET_TAG_'${item}'}') > + echo $tag > +} > + > +# $1: item > +kdump_targets_tell() > +{ > + local item=$1 tagvalue > + > + tagvalue=$(kdump_target_get_inventory $item) > + > + if [ "$tagvalue" = "1" ]; then > + return 0 > + elif [ "$tagvalue" = "0" ]; then > + return 1 > + else > + echo "Warning: wrong tag value for \"kdump_target_is $item\"" > + fi > + > + return 1 > +} > diff --git a/kdumpctl b/kdumpctl > index d8f049f..ca97580 100755 > --- a/kdumpctl > +++ b/kdumpctl > @@ -23,6 +23,12 @@ image_time=0 > . $dracutbasedir/dracut-functions.sh > . /lib/kdump/kdump-lib.sh > > +# Add your new checklist(job) "xxx" separated by whitespace, and > +# implement its logic to update its tag value in the tag inventory. > +# Then "kdump_targets_tell xxx" will be available for you to use. > +# See kdump_target_handle_item() for examples. > +KDUMP_TARGET_CHECKLIST="" > + > standard_kexec_args="-p" > > # Some default values in case /etc/sysconfig/kdump doesn't include > @@ -435,6 +441,114 @@ setup_initrd() > fi > } > > +kdump_target_inventory_init() > +{ > + kdump_target_checklist kdump_target_init_item > +} > + > +# Three values(0, 1, 2) are allowed in the tag inventory, > +# basically each value has the following meaning: > +# 0 - "surely no", 1 - "surely yes", 2 - "not sure" > +# Each item maintains its own way to update the value. > +# > +# Update the item tag value to 2 > +# $1: item > +kdump_target_init_item() > +{ > + kdump_target_update_inventory $1 2 > +} > + > +# $1: ssh, nfs, or block dev name > +kdump_target_hook() > +{ > + local dev=$1 > + > + if [ -b "$dev" ]; then > + dev=$(get_persistent_dev $dev) > + fi > + > + kdump_target_checklist kdump_target_handle_item $dev > +} > + > +# $1: function to be executed > +# $2,..: arguments of $1 > +kdump_target_checklist() > +{ > + local item func > + > + func=$1 > + shift > + for item in $KDUMP_TARGET_CHECKLIST; do > + "$func" $item $@ > + done > +} > + > +# $1: the item to be checked > +# $2: persistent block dev name, nfs, or ssh > +# > +# You can add an item(job) in KDUMP_TARGET_CHECKLIST, implement its > +# own logic on how to update its tag value in the tag inventory. > +kdump_target_handle_item() > +{ > + local item=$1 dev=$2 > + local value > + > + value=$(kdump_target_get_inventory $item) > + > + # The item handling begins > + case "$item" in > + *) > + echo "unhandled item in kdump target checklist" > + return > + esac > + > + kdump_target_update_inventory $item $value > +} > + > +# Update the item in the tag inventory. > +# $1: item > +# $2: value > +kdump_target_update_inventory() > +{ > + local tag > + > + tag=KDUMP_TARGET_TAG_$1 > + eval export $tag=$2 > +} > + > +# Currently, we add no more than two target hooks > +# 1) cases that only one root target > +# 2) cases that non-root target and the root target > +check_kdump_targets() > +{ > + local _target _root > + > + kdump_target_inventory_init > + > + _target=$(get_block_dump_target) > + if [ -n "$_target" ]; then > + kdump_target_hook $_target > + elif is_ssh_dump_target; then > + kdump_target_hook "ssh" > + else > + kdump_target_hook "nfs" > + fi > + > + # Add the root device if needed, currently kdump always mounts the root > + _root=$(get_root_fs_device) > + if [[ -b "$_root" && "$_root" != "$_target" ]]; then > + kdump_target_hook $_root > + fi > + > + # NOTE: > + # dracut parses devices from "/etc/fstab" with the "x-initrd.mount" option, > + # which will be added as host_devs, it also includes usually simple devices > + # (say mounted to /boot, /boot/efi/, etc) plus the root device. Then kdump > + # must wait for these devices if initramfs is built with "--hostonly-cmdline". > + # > + # Fedora doesn't have "--hostonly-cmdline" specified, so there's no problem. > +} > + > check_files_modified() > { > local modified_files="" > @@ -1144,6 +1258,8 @@ start() > fi > fi > > + check_kdump_targets > + > check_rebuild > if [ $? != 0 ]; then > echo "Starting kdump: [FAILED]" >

On Monday 27 March 2017 08:12 AM, Xunlei Pang wrote: Acked-by: Pratyush Anand <panand(a)redhat.com&gt; > --- > kdumpctl | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kdumpctl b/kdumpctl > index ca97580..e9dc8e5 100755 > --- a/kdumpctl > +++ b/kdumpctl > @@ -75,7 +75,8 @@ remove_cmdline_param() > for arg in $@; do > cmdline=`echo $cmdline | \ > sed -e "s/\b$arg=[^ ]*\b//g" \ > - -e "s/\b$arg\b//g" \ > + -e "s/^$arg\b//g" \ > + -e "s/[[:space:]]$arg\b//g" \ > -e "s/\s\+/ /g"` > done > echo $cmdline >

On 03/27/17 at 10:42am, Xunlei Pang wrote: Acked-by: Dave Young <dyoung(a)redhat.com&gt; Xunlei, I can merge this one first so that it does not being included in the series again. Thanks Dave

On 03/28/2017 at 01:37 PM, Pratyush Anand wrote: Yes, I will do it if sending next version. Thanks! Regards, Xunlei

On 03/28/2017 at 03:10 PM, Pratyush Anand wrote: At the beginning, I used a tmp file to store all the results. Then I found in mkdumprd there has "export IN_KDUMP=1" and then use it in 99kdump module-setup.sh, that's why I changed to use this way. At least It has one advantage that we don't need to worry about the problem of informatin gets removed during kdump starting when using tmp file. Regards, Xunlei

On 03/28/2017 at 04:03 PM, Dave Young wrote: Yes, I think so. e.g. all the lvm targets including lvm-typed raid, and multipath.

On 03/28/2017 at 03:55 PM, Dave Young wrote: Yes, thanks, PATCH6 can be independent now, I already updated it to be the following implementation since we don't need to judge the linear lvm, I did it unconditionally and ignored the error by using &>/dev/null: diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 1f96bb8..674f3ee 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -743,4 +743,9 @@ install() { # target. Ideally all this should be pushed into dracut iscsi module # at some point of time. kdump_check_iscsi_targets + + # For the lvm type target under kdump, in /etc/lvm/lvm.conf we can safely replace + # "reserved_memory = 8192" with "reserved_memory = 1024" to lower memory pressure + sed -i -e 's/\(^[[:space:]]*\)reserved_memory[[:space:]]*=[[:space:]]*[[:digit:]]*/ \ + \1reserved_memory = 1024/' ${initdir}/etc/lvm/lvm.conf &>/dev/null } Regards, Xunlei

On 03/28/2017 at 06:01 PM, Xunlei Pang wrote: Hi Dave, I've sent this update out separately, please help review it, so that we can focus on the remaining "remove root=X" issue from now on. Thanks! Regards, Xunlei