With this series, bind mount will be treated equally, and no longer have special workaround for atomic. And also cleaned up user configured target handling code. There should be no function change by this fat.
Another fix applied here is that in atomic/silverblue enviroment, the path is always resolved as current bind mounted path under root device, but for user configured targets, path should be just used as it is else the behavior will be uncontrollable and go wrong easily.
Kairui Song (5): Remove is_dump_target_configured mkdumprd: Use get_save_path instead of parsing config mkdumprd: Simplify handling of user specified target No longer treat atomic/silverblue specially Remove adjust_bind_mount_path call
dracut-module-setup.sh | 39 +--------------- kdump-lib.sh | 102 +++++++++++++---------------------------- kdumpctl | 18 ++------ mkdumprd | 37 ++++----------- 4 files changed, 48 insertions(+), 148 deletions(-)
It's basically same with is_user_configured_dump_target and only have one caller. And the name is confusing, the dump target is always configured, it's either user configured or path based.
Signed-off-by: Kairui Song kasong@redhat.com --- kdumpctl | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 594f99d..3543bc2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -891,15 +891,6 @@ save_raw() return 0 }
-is_dump_target_configured() -{ - local _target - - _target=$(egrep "^ext[234]|^xfs|^btrfs|^minix|^raw|^ssh|^nfs" /etc/kdump.conf) - - [ -n "$_target" ] -} - local_fs_dump_target() { local _target @@ -914,7 +905,11 @@ path_to_be_relabeled() { local _path _target _mnt="/" _rmnt
- if is_dump_target_configured; then + if is_user_configured_dump_target; then + if is_mount_in_dracut_args; then + return; + fi + _target=$(local_fs_dump_target) if [[ -n "$_target" ]]; then _mnt=$(findmnt -k -f -n -r -o TARGET $_target) @@ -926,9 +921,6 @@ path_to_be_relabeled() fi fi
- if is_mount_in_dracut_args; then - return; - fi _path=$(get_save_path) # if $_path is masked by other mount, we will not relabel it. _rmnt=$(df $_mnt/$_path 2>/dev/null | tail -1 | awk '{ print $NF }')
get_save_path provides default value fail back and error check, no need to repeat it again.
Also remove a redundant echo and grep in get_save_path
Signed-off-by: Kairui Song kasong@redhat.com --- kdump-lib.sh | 4 ++-- mkdumprd | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index d27aa3b..ea41d0c 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -138,11 +138,11 @@ get_root_fs_device()
get_save_path() { - local _save_path=$(grep "^path" /etc/kdump.conf|awk '{print $2}') + local _save_path=$(awk '$1 == "path" {print $2}' /etc/kdump.conf) [ -z "$_save_path" ] && _save_path=$DEFAULT_PATH
# strip the duplicated "/" - echo $(echo $_save_path | tr -s /) + echo $_save_path | tr -s / }
get_block_dump_target() diff --git a/mkdumprd b/mkdumprd index c6a0586..d86d9b5 100644 --- a/mkdumprd +++ b/mkdumprd @@ -13,10 +13,7 @@ export IN_KDUMP=1
conf_file="/etc/kdump.conf" SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa" -SAVE_PATH=$(awk '/^path/ {print $2}' $conf_file) -[ -z "$SAVE_PATH" ] && SAVE_PATH=$DEFAULT_PATH -# strip the duplicated "/" -SAVE_PATH=$(echo $SAVE_PATH | tr -s /) +SAVE_PATH=$(get_save_path) OVERRIDE_RESETTABLE=0
extra_modules=""
For user specified target, the config value is used as the dump target, and SAVE_PATH (path in kdump.conf) value is used as the dump path within the dump target, no need to do anything extra with the path value.
Current code logic is not only complicated, it also wrongly generate an redundantly long path in atomic/silverblue enviroment.
The right way is only check two things, and do nothing else:
1. The path exists within the target; 2. The target is large engouth to hold to contain the vmcore.
Currently checking the target still requires it to be mounted so it will error out if it's not mounted. Will implement some auto mount as next step.
Signed-off-by: Kairui Song kasong@redhat.com --- kdump-lib.sh | 29 +++++++++-------------------- mkdumprd | 15 ++++----------- 2 files changed, 13 insertions(+), 31 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index ea41d0c..f3ee95d 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -310,26 +310,6 @@ get_option_value() { strip_comments `grep "^$1[[:space:]]+" /etc/kdump.conf | tail -1 | cut -d\ -f2-` }
-#This function compose a absolute path with the mount -#point and the relative $SAVE_PATH. -#target is passed in as argument, could be UUID, LABEL, -#block device or even nfs server export of the form of -#"my.server.com:/tmp/export"? -#And possibly this could be used for both default case -#as well as when dump taret is specified. When dump -#target is not specified, then $target would be null. -make_absolute_save_path() -{ - local _target=$1 - local _mnt - - [ -n $_target ] && _mnt=$(get_mntpoint_from_target $1) - _mnt="${_mnt}/$SAVE_PATH" - - # strip the duplicated "/" - echo "$_mnt" | tr -s / -} - check_save_path_fs() { local _path=$1 @@ -339,6 +319,15 @@ check_save_path_fs() fi }
+# Check if path exists within dump target +check_save_path_user_configured() +{ + local _target=$1 _path=$2 + local _mnt=$(get_mntpoint_from_target $_target) + + check_save_path_fs "$_mnt/$_path" +} + is_atomic() { grep -q "ostree" /proc/cmdline diff --git a/mkdumprd b/mkdumprd index d86d9b5..a32cead 100644 --- a/mkdumprd +++ b/mkdumprd @@ -377,20 +377,13 @@ do perror_exit "Dump target $config_val is probably not mounted." fi
- _absolute_save_path=$(make_absolute_save_path $config_val) - _mntpoint=$(get_mntpoint_from_path $_absolute_save_path) - if is_atomic && is_bind_mount $_mntpoint; then - SAVE_PATH=${_absolute_save_path##"$_mntpoint"} - # the real dump path in the 2nd kernel, if the mount point is bind mounted. - SAVE_PATH=$(get_bind_mount_directory $_mntpoint)/$SAVE_PATH - fi - + # User configured target, use $SAVE_PATH as the dump path within the target + check_save_path_user_configured "$config_val" "$SAVE_PATH" + check_size fs "$config_val" add_mount "$config_val" - check_save_path_fs $_absolute_save_path - check_size fs $config_val ;; raw) - #checking raw disk writable + # checking raw disk writable dd if=$config_val count=1 of=/dev/null > /dev/null 2>&1 || { perror_exit "Bad raw disk $config_val" }
This commit remove (almose) all special workaround for atomic, and treat all bind mounts in any enviroment equally.
Use a heleper get_bind_mount_directory_from_path to get the bind mount source path of given path.
is_atomic function now only used to determine the right /boot path for atomic/silverblue enviroment.
And remove get_mntpoint_from_path(), it's the only function that never ignore bind mount, and it have no caller after this clean up.
Signed-off-by: Kairui Song kasong@redhat.com --- dracut-module-setup.sh | 36 +++++----------------- kdump-lib.sh | 69 +++++++++++++----------------------------- mkdumprd | 17 +++-------- 3 files changed, 32 insertions(+), 90 deletions(-)
diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index e2315d5..57f311c 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -428,19 +428,9 @@ default_dump_target_install_conf()
is_user_configured_dump_target && return
- _save_path=$(get_save_path) - _mntpoint=$(get_mntpoint_from_path $_save_path) + _save_path=$(get_bind_mount_source $(get_save_path)) _target=$(get_target_from_path $_save_path) - - if is_atomic && is_bind_mount $_mntpoint; then - _save_path=${_save_path##"$_mntpoint"} - # the real dump path in the 2nd kernel, if the mount point is bind mounted. - _save_path=$(get_bind_mount_directory $_mntpoint)/$_save_path - _mntpoint=$(get_mntpoint_from_target $_target) - - # the absolute path in the 1st kernel - _save_path=$_mntpoint/$_save_path - fi + _mntpoint=$(get_mntpoint_from_target $_target)
_fstype=$(get_fs_type_from_target $_target) if is_fs_type_nfs $_fstype; then @@ -452,8 +442,6 @@ default_dump_target_install_conf()
echo "$_fstype $_target" >> ${initdir}/tmp/$$-kdump.conf
- # strip the duplicated "/" - _save_path=$(echo $_save_path | tr -s /) # don't touch the path under root mount if [ "$_mntpoint" != "/" ]; then _save_path=${_save_path##"$_mntpoint"} @@ -466,21 +454,13 @@ default_dump_target_install_conf()
adjust_bind_mount_path() { - local _target=$1 local _save_path=$(get_save_path) - local _absolute_save_path=$(get_mntpoint_from_target $_target)/$_save_path + local _mntpoint=$(get_mntpoint_from_target $1) + local _absolute_save_path=$(get_bind_mount_source $_mntpoint$_save_path)
- _absolute_save_path=$(echo "$_absolute_save_path" | tr -s /) - local _mntpoint=$(get_mntpoint_from_path $_absolute_save_path) - - if is_bind_mount $_mntpoint; then - _save_path=${_absolute_save_path##"$_mntpoint"} - # the real dump path in the 2nd kernel, if the mount point is bind mounted. - _save_path=$(get_bind_mount_directory $_mntpoint)/$_save_path - - #erase the old path line, then insert the parsed path + if [ $_absolute_save_path != $_save_path ]; then sed -i "/^path/d" ${initdir}/tmp/$$-kdump.conf - echo "path $_save_path" >> ${initdir}/tmp/$$-kdump.conf + echo "path $_absolute_save_path" >> ${initdir}/tmp/$$-kdump.conf fi }
@@ -500,9 +480,7 @@ kdump_install_conf() { ext[234]|xfs|btrfs|minix) _pdev=$(kdump_get_persistent_dev $_val) sed -i -e "s#^$_opt[[:space:]]+$_val#$_opt $_pdev#" ${initdir}/tmp/$$-kdump.conf - if is_atomic; then - adjust_bind_mount_path "$_val" - fi + adjust_bind_mount_path "$_val" ;; ssh|nfs) kdump_install_net "$_val" diff --git a/kdump-lib.sh b/kdump-lib.sh index f3ee95d..64d0603 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -204,44 +204,39 @@ get_kdump_targets() echo "$kdump_targets" }
- +# Return the bind mount source path, return the path itself if it's not bind mounted +# Eg. if /path/to/src is bind mounted to /mnt/bind, then: +# /mnt/bind -> /path/to/src, /mnt/bind/dump -> /path/to/src/dump +# # 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. -is_bind_mount() -{ - local _mntpoint=$(findmnt $1 | tail -n 1 | awk '{print $2}') - local _mntpoint_nofsroot=$(findmnt -v $1 | tail -n 1 | awk '{print $2}') - - if [[ $_mntpoint = $_mntpoint_nofsroot ]]; then - return 1 - else - return 0 - fi -} - +# # Below is just an example for mount info # /dev/mapper/atomicos-root[/ostree/deploy/rhel-atomic-host/var], if the # directory is bind mounted. The former part represents the device path, rest # part is the bind mounted directory which quotes by bracket "[]". -get_bind_mount_directory() +get_bind_mount_source() { - local _mntpoint=$(findmnt $1 | tail -n 1 | awk '{print $2}') - local _mntpoint_nofsroot=$(findmnt -v $1 | tail -n 1 | awk '{print $2}') + local _path=$1 + # In case it's a sub path in a mount point, get the mount point first + local _mnt_top=$(df $_path | tail -1 | awk '{print $NF}') + local _mntpoint=$(findmnt $_mnt_top | tail -n 1 | awk '{print $2}') + local _mntpoint_nofsroot=$(findmnt -v $_mnt_top | tail -n 1 | awk '{print $2}')
- _mntpoint=${_mntpoint#*$_mntpoint_nofsroot} + if [[ "$_mntpoint" = $_mntpoint_nofsroot ]]; then + echo $_path && return + fi
+ _mntpoint=${_mntpoint#*$_mntpoint_nofsroot} _mntpoint=${_mntpoint#[} _mntpoint=${_mntpoint%]} + _path=${_path#$_mnt_top}
- echo $_mntpoint -} - -get_mntpoint_from_path() -{ - df $1 | tail -1 | awk '{print $NF}' + echo $_mntpoint$_path }
+# Return the real underlaying device of a path, ignore bind mounts get_target_from_path() { local _target @@ -256,33 +251,11 @@ get_fs_type_from_target() findmnt -k -f -n -r -o FSTYPE $1 }
-# input: device path -# output: the general mount point -# find the general mount point, not the bind mounted point in atomic -# As general system, Use the previous code -# -# ERROR and EXIT: -# the device can be umounted the general mount point, if one of the mount point is bind mounted -# For example: -# mount /dev/sda /mnt/ -# mount -o bind /mnt/var /var -# umount /mnt +# Find the general mount point of a dump target, not the bind mount point get_mntpoint_from_target() { - if is_atomic; then - for _mnt in $(findmnt -k -n -r -o TARGET $1) - do - if ! is_bind_mount $_mnt; then - echo $_mnt - return - fi - done - - echo "Mount $1 firstly, without the bind mode" >&2 - exit 1 - else - echo $(findmnt -k -f -n -r -o TARGET $1) - fi + # Expcilitly specify --source to findmnt could ensure non-bind mount is returned + findmnt -k -f -n -r -o TARGET --source $1 }
# Get the path where the target will be mounted in kdump kernel diff --git a/mkdumprd b/mkdumprd index a32cead..8802307 100644 --- a/mkdumprd +++ b/mkdumprd @@ -230,20 +230,11 @@ handle_default_dump_target()
check_save_path_fs $SAVE_PATH
- _mntpoint=$(get_mntpoint_from_path $SAVE_PATH) - _target=$(get_target_from_path $SAVE_PATH) + _save_path=$(get_bind_mount_source $SAVE_PATH) + _target=$(get_target_from_path $_save_path) + _mntpoint=$(get_mntpoint_from_target $_target)
- if is_atomic && is_bind_mount $_mntpoint; then - SAVE_PATH=${SAVE_PATH##"$_mntpoint"} - # the real dump path in the 2nd kernel, if the mount point is bind mounted. - SAVE_PATH=$(get_bind_mount_directory $_mntpoint)/$SAVE_PATH - _mntpoint=$(get_mntpoint_from_target $_target) - - # the absolute path in the 1st kernel - SAVE_PATH=$_mntpoint/$SAVE_PATH - fi - - SAVE_PATH=${SAVE_PATH##"$_mntpoint"} + SAVE_PATH=${_save_path##"$_mntpoint"} add_mount "$_target" check_size fs $_target }
If user configured target is used, path should be used as the absolute path within the dump target direct, and user should be fully aware of the path structure within the target device. The adjust_bind_mount_path call here make it very hard to control the behavior.
Especially, if it's a cross device bind mount, this will likely create a invalid path in the target. And for atomic case, adjust_bind_mount_path call here assumes user will always pass root device as the explicitly configured dump target, which is not true.
If user configured target device is used, the path is always be the absolute path inside of given target. If user don't know about the path structure in the target device, then user should either use the path based config, or carefully exam the target device before using it as a dump target.
Signed-off-by: Kairui Song kasong@redhat.com --- dracut-module-setup.sh | 13 ------------- 1 file changed, 13 deletions(-)
diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 57f311c..ec16bd1 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -452,18 +452,6 @@ default_dump_target_install_conf() echo "path $_save_path" >> ${initdir}/tmp/$$-kdump.conf }
-adjust_bind_mount_path() -{ - local _save_path=$(get_save_path) - local _mntpoint=$(get_mntpoint_from_target $1) - local _absolute_save_path=$(get_bind_mount_source $_mntpoint$_save_path) - - if [ $_absolute_save_path != $_save_path ]; then - sed -i "/^path/d" ${initdir}/tmp/$$-kdump.conf - echo "path $_absolute_save_path" >> ${initdir}/tmp/$$-kdump.conf - fi -} - #install kdump.conf and what user specifies in kdump.conf kdump_install_conf() { local _opt _val _pdev @@ -480,7 +468,6 @@ kdump_install_conf() { ext[234]|xfs|btrfs|minix) _pdev=$(kdump_get_persistent_dev $_val) sed -i -e "s#^$_opt[[:space:]]+$_val#$_opt $_pdev#" ${initdir}/tmp/$$-kdump.conf - adjust_bind_mount_path "$_val" ;; ssh|nfs) kdump_install_net "$_val"
Hi, Kairui
Thank you for the improvement.
This series looks good to me. But I noticed some typos(I put them at the end) in the patch log, you could correct them when merging this series, and no need to post again.
Acked-by: Lianbo Jiang lijiang@redhat.com
Lianbo 在 2020年03月12日 21:33, Kairui Song 写道:
With this series, bind mount will be treated equally, and no longer have special workaround for atomic. And also cleaned up user configured target handling code. There should be no function change by this fat.
Another fix applied here is that in atomic/silverblue enviroment, the path is always resolved as current bind mounted path under root device, but for user configured targets, path should be just used as it is else the behavior will be uncontrollable and go wrong easily.
Kairui Song (5): Remove is_dump_target_configured mkdumprd: Use get_save_path instead of parsing config mkdumprd: Simplify handling of user specified target No longer treat atomic/silverblue specially Remove adjust_bind_mount_path call
dracut-module-setup.sh | 39 +--------------- kdump-lib.sh | 102 +++++++++++++---------------------------- kdumpctl | 18 ++------ mkdumprd | 37 ++++----------- 4 files changed, 48 insertions(+), 148 deletions(-)
Some typos in patch log: [1] Current code logic is not only complicated, it also wrongly generate an redundantly long path in atomic/silverblue ***enviroment***. [2] The target is large ***engouth*** to hold to contain the vmcore. [3] This commit remove (***almose***) all special workaround for atomic, and treat [4] all bind mounts in any ***enviroment*** equally. [5] Use a ***heleper*** get_bind_mount_directory_from_path to get the bind mount [6] for atomic/silverblue ***enviroment***.
On Fri, Mar 27, 2020 at 1:52 PM lijiang lijiang@redhat.com wrote:
Hi, Kairui
Thank you for the improvement.
This series looks good to me. But I noticed some typos(I put them at the end) in the patch log, you could correct them when merging this series, and no need to post again.
Acked-by: Lianbo Jiang lijiang@redhat.com
Lianbo 在 2020年03月12日 21:33, Kairui Song 写道:
With this series, bind mount will be treated equally, and no longer have special workaround for atomic. And also cleaned up user configured target handling code. There should be no function change by this fat.
Another fix applied here is that in atomic/silverblue enviroment, the path is always resolved as current bind mounted path under root device, but for user configured targets, path should be just used as it is else the behavior will be uncontrollable and go wrong easily.
Kairui Song (5): Remove is_dump_target_configured mkdumprd: Use get_save_path instead of parsing config mkdumprd: Simplify handling of user specified target No longer treat atomic/silverblue specially Remove adjust_bind_mount_path call
dracut-module-setup.sh | 39 +--------------- kdump-lib.sh | 102 +++++++++++++---------------------------- kdumpctl | 18 ++------ mkdumprd | 37 ++++----------- 4 files changed, 48 insertions(+), 148 deletions(-)
Some typos in patch log: [1] Current code logic is not only complicated, it also wrongly generate an redundantly long path in atomic/silverblue ***enviroment***. [2] The target is large ***engouth*** to hold to contain the vmcore. [3] This commit remove (***almose***) all special workaround for atomic, and treat [4] all bind mounts in any ***enviroment*** equally. [5] Use a ***heleper*** get_bind_mount_directory_from_path to get the bind mount [6] for atomic/silverblue ***enviroment***.
Hi, Lianbo
Thank you for the review, I'll fix the typos before merge it.
-
Kairui Song -
lijiang