This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases: 1) Dumping VM's vmcore to a virtiofs shared directory; 2) When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash; 3) The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
Signed-off-by: Tao Liu ltao@redhat.com --- dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;; - ext[234] | xfs | btrfs | minix | nfs) + ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;; diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;; - ext[234] | xfs | btrfs | minix) + ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;; diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{ + if [ -n "$(kdump_get_conf_val virtiofs)" ]; then + return 0 + fi + + _path=$(kdump_get_conf_val path) + _path=${_path:-$DEFAULT_PATH} + if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; then + return 0 + fi + + return 1 +} + is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() { - [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix")" ] + [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|virtiofs")" ] } diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() { - [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh") ]] || is_mount_in_dracut_args + [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh|virtiofs") ]] || is_mount_in_dracut_args }
get_user_configured_dump_disk() { - local _target + local _target _fstype
- _target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw") + _target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|virtiofs") [[ -n $_target ]] && echo "$_target" && return
_target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")") - [[ -b $_target ]] && echo "$_target" + [[ -b $_target ]] && echo "$_target" && return + + _fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")") + [[ "$_fstype" == "virtiofs" ]] && echo "$_target" }
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path") - [[ -b $_target ]] && to_dev_name "$_target" + [[ -b $_target ]] && to_dev_name "$_target" && return + [ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && return }
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return + [ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && return # Then, must be nfs root echo "nfs" fi diff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;; - ext[234] | minix | btrfs | xfs | nfs | ssh) + ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;; @@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target. - # Currently we do not check also if ssh/nfs target is specified - if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then + # Currently we do not check also if ssh/nfs/virtiofs target is specified + if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fi
diff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;; - ext[234] | xfs | btrfs | minix | nfs) + ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800 Tao Liu ltao@redhat.com wrote:
This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases:
- Dumping VM's vmcore to a virtiofs shared directory;
- When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash;
- The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
Signed-off-by: Tao Liu ltao@redhat.com
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;;diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;
ext[234] | xfs | btrfs | minix)
ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]\+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{
- if [ -n "$(kdump_get_conf_val virtiofs)" ]; then
return 0- fi
- _path=$(kdump_get_conf_val path)
- _path=${_path:-$DEFAULT_PATH}
- if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; then
return 0- fi
- return 1
+}
I don't think this check is sufficient. I would expect it to be more the less identical to is_nfs_dump_target that also checks for the target being defined in the dracut args.
is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() {
- [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix")" ]
- [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|virtiofs")" ]
} diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() {
- [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh") ]] || is_mount_in_dracut_args
- [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh|virtiofs") ]] || is_mount_in_dracut_args
}
get_user_configured_dump_disk() {
- local _target
- local _target _fstype
- _target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw")
_target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|virtiofs") [[ -n $_target ]] && echo "$_target" && return
_target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")")
- [[ -b $_target ]] && echo "$_target"
- [[ -b $_target ]] && echo "$_target" && return
- _fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")")
- [[ "$_fstype" == "virtiofs" ]] && echo "$_target"
While at it, does it make sense to merge this function with the one below? The way I see it get_block_dump_target is the only user and it is kind of weird to split up the special handling into two places. Especially when combined both places are a reimplementation of is_virtio_dump_target.
}
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path")
- [[ -b $_target ]] && to_dev_name "$_target"
- [[ -b $_target ]] && to_dev_name "$_target" && return
- [ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && return
findmnt -> get_fs_type_from_target?
Although I'm not sure if this hunk is correct at all. My problem is that get_block_dump_target also gets called by get_all_kdump_crypt_dev which doesn't make much sense to me as I don't expect a virtiofs disc can be LUKS encrypted. Note: this shouldn't break anything as get_luks_crypt_dev immediately returns when the device isn't a block device. But still it feels odd to me.
}
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return
[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && return
findmnt -> get_fs_type_from_target?
# Then, must be nfs root echo "nfs"fi diff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs
I would move this line further up and group it the ext4 examples. At least for me this it makes it clearer that both cases are described in the "<fs type> <partition>" example provided in the comment above.
Site note: Does it make sense to mention the supported fs types in the comment above to reduce the confusion even more?
Thanks Philipp
auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;
ext[234] | minix | btrfs | xfs | nfs | ssh)
ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;@@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target.
- # Currently we do not check also if ssh/nfs target is specified
- if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then
- # Currently we do not check also if ssh/nfs/virtiofs target is specified
- if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fi
diff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;;
- ext[234] | xfs | btrfs | minix | nfs)
- ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
Oh and...
On Tue, 9 Aug 2022 17:27:35 +0200 Philipp Rudo prudo@redhat.com wrote:
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800 Tao Liu ltao@redhat.com wrote:
This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases:
- Dumping VM's vmcore to a virtiofs shared directory;
- When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash;
- The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
... what happens when you use this feature with an older version of dracut? Does it make sense to increase the minimal dracut version in the spec file?
Thanks Philipp
Signed-off-by: Tao Liu ltao@redhat.com
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;;diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;
ext[234] | xfs | btrfs | minix)
ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]\+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{
- if [ -n "$(kdump_get_conf_val virtiofs)" ]; then
return 0- fi
- _path=$(kdump_get_conf_val path)
- _path=${_path:-$DEFAULT_PATH}
- if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; then
return 0- fi
- return 1
+}
I don't think this check is sufficient. I would expect it to be more the less identical to is_nfs_dump_target that also checks for the target being defined in the dracut args.
is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() {
- [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix")" ]
- [ -n "$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|virtiofs")" ]
} diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() {
- [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh") ]] || is_mount_in_dracut_args
- [[ $(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|nfs|ssh|virtiofs") ]] || is_mount_in_dracut_args
}
get_user_configured_dump_disk() {
- local _target
- local _target _fstype
- _target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw")
_target=$(kdump_get_conf_val "ext[234]|xfs|btrfs|minix|raw|virtiofs") [[ -n $_target ]] && echo "$_target" && return
_target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")")
- [[ -b $_target ]] && echo "$_target"
- [[ -b $_target ]] && echo "$_target" && return
- _fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")")
- [[ "$_fstype" == "virtiofs" ]] && echo "$_target"
While at it, does it make sense to merge this function with the one below? The way I see it get_block_dump_target is the only user and it is kind of weird to split up the special handling into two places. Especially when combined both places are a reimplementation of is_virtio_dump_target.
}
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path")
- [[ -b $_target ]] && to_dev_name "$_target"
- [[ -b $_target ]] && to_dev_name "$_target" && return
- [ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && return
findmnt -> get_fs_type_from_target?
Although I'm not sure if this hunk is correct at all. My problem is that get_block_dump_target also gets called by get_all_kdump_crypt_dev which doesn't make much sense to me as I don't expect a virtiofs disc can be LUKS encrypted. Note: this shouldn't break anything as get_luks_crypt_dev immediately returns when the device isn't a block device. But still it feels odd to me.
}
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return
[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
# Then, must be nfs root echo "nfs"fi diff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs
I would move this line further up and group it the ext4 examples. At least for me this it makes it clearer that both cases are described in the "<fs type> <partition>" example provided in the comment above.
Site note: Does it make sense to mention the supported fs types in the comment above to reduce the confusion even more?
Thanks Philipp
auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;
ext[234] | minix | btrfs | xfs | nfs | ssh)
ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;@@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target.
- # Currently we do not check also if ssh/nfs target is specified
- if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then
- # Currently we do not check also if ssh/nfs/virtiofs target is specified
- if is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fi
diff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;;
- ext[234] | xfs | btrfs | minix | nfs)
- ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
Hi Philipp,
On Tue, Aug 9, 2022 at 11:27 PM Philipp Rudo prudo@redhat.com wrote:
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800 Tao Liu ltao@redhat.com wrote:
This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases:
- Dumping VM's vmcore to a virtiofs shared directory;
- When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash;
- The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
Signed-off-by: Tao Liu ltao@redhat.com
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;;diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;
ext[234] | xfs | btrfs | minix)
ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]\+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{
if [ -n "$(kdump_get_conf_val virtiofs)" ]; thenreturn 0fi_path=$(kdump_get_conf_val path)_path=${_path:-$DEFAULT_PATH}if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; thenreturn 0fireturn 1+}
I don't think this check is sufficient. I would expect it to be more the less identical to is_nfs_dump_target that also checks for the target being defined in the dracut args.
Agreed, will take is_nfs_dump_target as reference.
is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() {
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix")" ]
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|virtiofs")" ]} diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() {
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh") ]] || is_mount_in_dracut_args
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh\|virtiofs") ]] || is_mount_in_dracut_args}
get_user_configured_dump_disk() {
local _target
local _target _fstype
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw")
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|virtiofs") [[ -n $_target ]] && echo "$_target" && return _target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")")
[[ -b $_target ]] && echo "$_target"
[[ -b $_target ]] && echo "$_target" && return_fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")")[[ "$_fstype" == "virtiofs" ]] && echo "$_target"While at it, does it make sense to merge this function with the one below? The way I see it get_block_dump_target is the only user and it is kind of weird to split up the special handling into two places. Especially when combined both places are a reimplementation of is_virtio_dump_target.
Agreed, merging the 2 functions makes more sense.
}
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path")
[[ -b $_target ]] && to_dev_name "$_target"
[[ -b $_target ]] && to_dev_name "$_target" && return[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
Although I'm not sure if this hunk is correct at all. My problem is that get_block_dump_target also gets called by get_all_kdump_crypt_dev which doesn't make much sense to me as I don't expect a virtiofs disc can be LUKS encrypted. Note: this shouldn't break anything as get_luks_crypt_dev immediately returns when the device isn't a block device. But still it feels odd to me.
Before apply the patch: If we set "path /var/crash" in kdump.conf and the rootfs of the VM is virtiofs, ie myfs, then _path="/var/crash", _target="myfs". However myfs will not pass the [[ -b ]] check, so get_block_dump_target will return "".
Then in get_kdump_targets():
_target=$(get_block_dump_target) if [[ -n $_target ]]; then kdump_targets=$_target elif is_ssh_dump_target; then kdump_targets="ssh" else kdump_targets="nfs" fi
kdump_targets will become "nfs", it totally makes no sense. So I want get_block_dump_target to return "myfs", so here kdump_targets can be "myfs".
Another implementation here is, we leave get_block_dump_target to return "", then we add elif is_virtiofs_dump_target; then kdump_targets=$(get_virtiofs_dump_target). Then we can implement get_virtiofs_dump_target elsewhere.
My original thought was, let get_block_dump_target return "myfs" instead of "", for other functions which call get_block_dump_target can get the dump target info, which may be useful for future checking, better than nothing.
}
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return
[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
# Then, must be nfs root echo "nfs" fidiff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs
I would move this line further up and group it the ext4 examples. At least for me this it makes it clearer that both cases are described in the "<fs type> <partition>" example provided in the comment above.
Agreed.
Site note: Does it make sense to mention the supported fs types in the comment above to reduce the confusion even more?
Do you mean something like: # <fs type> <partition> # - Will mount -t <fs type> <partition> <mnt>, and copy # /proc/vmcore to <mnt>/<path>/%HOST_IP-%DATE/. # NOTE: <partition> can be a device node, label or uuid. # It's recommended to use persistent device names # such as /dev/vg/<devname>. # Otherwise it's suggested to use label or uuid. # Currently accepted fs types are "ext[234] | xfs | btrfs | minix | nfs | virtiofs"
Thanks, Tao Liu
Thanks Philipp
auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;
ext[234] | minix | btrfs | xfs | nfs | ssh)
ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;@@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target.
# Currently we do not check also if ssh/nfs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then
# Currently we do not check also if ssh/nfs/virtiofs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fidiff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
Hi Tao,
On Wed, 10 Aug 2022 18:05:58 +0800 Tao Liu ltao@redhat.com wrote:
Hi Philipp,
On Tue, Aug 9, 2022 at 11:27 PM Philipp Rudo prudo@redhat.com wrote:
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800 Tao Liu ltao@redhat.com wrote:
This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases:
- Dumping VM's vmcore to a virtiofs shared directory;
- When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash;
- The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
See my comment on squashfs patch.
Signed-off-by: Tao Liu ltao@redhat.com
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;;diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;
ext[234] | xfs | btrfs | minix)
ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]\+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{
if [ -n "$(kdump_get_conf_val virtiofs)" ]; thenreturn 0fi_path=$(kdump_get_conf_val path)_path=${_path:-$DEFAULT_PATH}if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; thenreturn 0fireturn 1+}
I don't think this check is sufficient. I would expect it to be more the less identical to is_nfs_dump_target that also checks for the target being defined in the dracut args.
Agreed, will take is_nfs_dump_target as reference.
thanks
is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() {
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix")" ]
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|virtiofs")" ]} diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() {
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh") ]] || is_mount_in_dracut_args
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh\|virtiofs") ]] || is_mount_in_dracut_args}
get_user_configured_dump_disk() {
local _target
local _target _fstype
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw")
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|virtiofs") [[ -n $_target ]] && echo "$_target" && return _target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")")
[[ -b $_target ]] && echo "$_target"
[[ -b $_target ]] && echo "$_target" && return_fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")")[[ "$_fstype" == "virtiofs" ]] && echo "$_target"While at it, does it make sense to merge this function with the one below? The way I see it get_block_dump_target is the only user and it is kind of weird to split up the special handling into two places. Especially when combined both places are a reimplementation of is_virtio_dump_target.
Agreed, merging the 2 functions makes more sense.
thanks
}
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path")
[[ -b $_target ]] && to_dev_name "$_target"
[[ -b $_target ]] && to_dev_name "$_target" && return[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
Although I'm not sure if this hunk is correct at all. My problem is that get_block_dump_target also gets called by get_all_kdump_crypt_dev which doesn't make much sense to me as I don't expect a virtiofs disc can be LUKS encrypted. Note: this shouldn't break anything as get_luks_crypt_dev immediately returns when the device isn't a block device. But still it feels odd to me.
Before apply the patch: If we set "path /var/crash" in kdump.conf and the rootfs of the VM is virtiofs, ie myfs, then _path="/var/crash", _target="myfs". However myfs will not pass the [[ -b ]] check, so get_block_dump_target will return "".
Then in get_kdump_targets():
_target=$(get_block_dump_target) if [[ -n $_target ]]; then kdump_targets=$_target elif is_ssh_dump_target; then kdump_targets="ssh" else kdump_targets="nfs" fi
kdump_targets will become "nfs", it totally makes no sense. So I want get_block_dump_target
Right, "nfs" doesn't make any sense. Although I must say that for me the above function is bad code. You should always check all cases explicitly and use the else-block as error path. Otherwise you can run in very bad bugs...
to return "myfs", so here kdump_targets can be "myfs".
Another implementation here is, we leave get_block_dump_target to return "", then we add elif is_virtiofs_dump_target; then kdump_targets=$(get_virtiofs_dump_target). Then we can implement get_virtiofs_dump_target elsewhere.
My original thought was, let get_block_dump_target return "myfs" instead of "", for other functions which call get_block_dump_target can get the dump target info, which may be useful for future checking, better than nothing.
After thinking about it your approach is probably the better one. Only thing better would be to make the OPT array available to all bash scripts but that would be a larger project...
}
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return
[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
# Then, must be nfs root echo "nfs" fidiff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs
I would move this line further up and group it the ext4 examples. At least for me this it makes it clearer that both cases are described in the "<fs type> <partition>" example provided in the comment above.
Agreed.
thanks
Site note: Does it make sense to mention the supported fs types in the comment above to reduce the confusion even more?
Do you mean something like: # <fs type> <partition> # - Will mount -t <fs type> <partition> <mnt>, and copy # /proc/vmcore to <mnt>/<path>/%HOST_IP-%DATE/. # NOTE: <partition> can be a device node, label or uuid. # It's recommended to use persistent device names # such as /dev/vg/<devname>. # Otherwise it's suggested to use label or uuid. # Currently accepted fs types are "ext[234] | xfs | btrfs | minix | nfs | virtiofs"
That's almost exactly what I had in mind. Although personally I would choose the human readable version, i.e.
Supported fs types: ext[234], xfs, btrfs, minix, virtiofs
Note, that I dropped nfs as that has an example for itself.
Thanks Philipp
Thanks, Tao Liu
Thanks Philipp
auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;
ext[234] | minix | btrfs | xfs | nfs | ssh)
ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;@@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target.
# Currently we do not check also if ssh/nfs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then
# Currently we do not check also if ssh/nfs/virtiofs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fidiff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
Hi Philipp,
On Thu, Aug 11, 2022 at 12:15 AM Philipp Rudo prudo@redhat.com wrote:
Hi Tao,
On Wed, 10 Aug 2022 18:05:58 +0800 Tao Liu ltao@redhat.com wrote:
Hi Philipp,
On Tue, Aug 9, 2022 at 11:27 PM Philipp Rudo prudo@redhat.com wrote:
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800 Tao Liu ltao@redhat.com wrote:
This patch add virtiofs support for kexec-tools by introducing a new option for /etc/kdump.conf:
virtiofs myfs
Where myfs is a variable tag name specified in qemu cmdline "-device vhost-user-fs-pci,tag=myfs".
The patch covers the following cases:
- Dumping VM's vmcore to a virtiofs shared directory;
- When the VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to its subdirectory, such as /var/crash;
- The combination of case 1 & 2: The VM's rootfs is a virtiofs shared directory and dumping the VM's vmcore to another virtiofs shared directory.
Note: This feature reqiures dracut >= 057
See my comment on squashfs patch.
I will bump the dracut version in v2 patch. If dracut < 57, case 1 can work, but case 2 & 3 will not work, it requires dracut to handle "root=virtiofs:myfs" kernel cmdline, which will be handled by 95virtiofs module of dracut.
Signed-off-by: Tao Liu ltao@redhat.com
dracut-kdump.sh | 2 +- dracut-module-setup.sh | 2 +- kdump-lib-initramfs.sh | 17 ++++++++++++++++- kdump-lib.sh | 15 ++++++++++----- kdump.conf | 1 + kdumpctl | 6 +++--- mkdumprd | 2 +- 7 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index f4456a1..5c980a8 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -519,7 +519,7 @@ read_kdump_confs() DUMP_INSTRUCTION="dump_fs $config_val" fi ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) config_val=$(get_mntpoint_from_target "$config_val") DUMP_INSTRUCTION="dump_fs $config_val" ;;diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh index 4c6096a..a790a93 100755 --- a/dracut-module-setup.sh +++ b/dracut-module-setup.sh @@ -673,7 +673,7 @@ kdump_install_conf() { _pdev=$(persistent_policy="by-id" kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;
ext[234] | xfs | btrfs | minix)
ext[234] | xfs | btrfs | minix | virtiofs) _pdev=$(kdump_get_persistent_dev "$_val") sed -i -e "s#^${_opt}[[:space:]]\+$_val#$_opt $_pdev#" "${initdir}/tmp/$$-kdump.conf" ;;diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 84e6bf7..895544d 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -110,6 +110,21 @@ is_raw_dump_target() [ -n "$(kdump_get_conf_val raw)" ] }
+is_virtiofs_dump_target() +{
if [ -n "$(kdump_get_conf_val virtiofs)" ]; thenreturn 0fi_path=$(kdump_get_conf_val path)_path=${_path:-$DEFAULT_PATH}if [ "$(df "$_path" --output=fstype | tail -1)" = "virtiofs" ]; thenreturn 0fireturn 1+}
I don't think this check is sufficient. I would expect it to be more the less identical to is_nfs_dump_target that also checks for the target being defined in the dracut args.
Agreed, will take is_nfs_dump_target as reference.
thanks
is_nfs_dump_target() { if [ -n "$(kdump_get_conf_val nfs)" ]; then @@ -129,5 +144,5 @@ is_nfs_dump_target()
is_fs_dump_target() {
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix")" ]
[ -n "$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|virtiofs")" ]} diff --git a/kdump-lib.sh b/kdump-lib.sh index f7b659e..0946196 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -81,18 +81,21 @@ to_dev_name()
is_user_configured_dump_target() {
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh") ]] || is_mount_in_dracut_args
[[ $(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|nfs\|ssh\|virtiofs") ]] || is_mount_in_dracut_args}
get_user_configured_dump_disk() {
local _target
local _target _fstype
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw")
_target=$(kdump_get_conf_val "ext[234]\|xfs\|btrfs\|minix\|raw\|virtiofs") [[ -n $_target ]] && echo "$_target" && return _target=$(get_dracut_args_target "$(kdump_get_conf_val "dracut_args")")
[[ -b $_target ]] && echo "$_target"
[[ -b $_target ]] && echo "$_target" && return_fstype=$(get_dracut_args_fstype "$(kdump_get_conf_val "dracut_args")")[[ "$_fstype" == "virtiofs" ]] && echo "$_target"While at it, does it make sense to merge this function with the one below? The way I see it get_block_dump_target is the only user and it is kind of weird to split up the special handling into two places. Especially when combined both places are a reimplementation of is_virtio_dump_target.
Agreed, merging the 2 functions makes more sense.
thanks
}
get_block_dump_target() @@ -109,7 +112,8 @@ get_block_dump_target() # Get block device name from local save path _path=$(get_save_path) _target=$(get_target_from_path "$_path")
[[ -b $_target ]] && to_dev_name "$_target"
[[ -b $_target ]] && to_dev_name "$_target" && return[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
Although I'm not sure if this hunk is correct at all. My problem is that get_block_dump_target also gets called by get_all_kdump_crypt_dev which doesn't make much sense to me as I don't expect a virtiofs disc can be LUKS encrypted. Note: this shouldn't break anything as get_luks_crypt_dev immediately returns when the device isn't a block device. But still it feels odd to me.
Before apply the patch: If we set "path /var/crash" in kdump.conf and the rootfs of the VM is virtiofs, ie myfs, then _path="/var/crash", _target="myfs". However myfs will not pass the [[ -b ]] check, so get_block_dump_target will return "".
Then in get_kdump_targets():
_target=$(get_block_dump_target) if [[ -n $_target ]]; then kdump_targets=$_target elif is_ssh_dump_target; then kdump_targets="ssh" else kdump_targets="nfs" fi
kdump_targets will become "nfs", it totally makes no sense. So I want get_block_dump_target
Right, "nfs" doesn't make any sense. Although I must say that for me the above function is bad code. You should always check all cases explicitly and use the else-block as error path. Otherwise you can run in very bad bugs...
Thanks, please see v2 if it looks better...
to return "myfs", so here kdump_targets can be "myfs".
Another implementation here is, we leave get_block_dump_target to return "", then we add elif is_virtiofs_dump_target; then kdump_targets=$(get_virtiofs_dump_target). Then we can implement get_virtiofs_dump_target elsewhere.
My original thought was, let get_block_dump_target return "myfs" instead of "", for other functions which call get_block_dump_target can get the dump target info, which may be useful for future checking, better than nothing.
After thinking about it your approach is probably the better one. Only thing better would be to make the OPT array available to all bash scripts but that would be a larger project...
Yes I agree.
}
is_dump_to_rootfs() @@ -125,6 +129,7 @@ get_failure_action_target() # Get rootfs device name _target=$(get_root_fs_device) [[ -b $_target ]] && to_dev_name "$_target" && return
[ "$(findmnt -k -n -r -S "$_target" -o fstype)" = "virtiofs" ] && echo "$_target" && returnfindmnt -> get_fs_type_from_target?
# Then, must be nfs root echo "nfs" fidiff --git a/kdump.conf b/kdump.conf index d4fc78b..fc2ebf1 100644 --- a/kdump.conf +++ b/kdump.conf @@ -176,6 +176,7 @@ #ssh user@my.server.com #ssh user@2001:db8::1:2:3:4 #sshkey /root/.ssh/kdump_id_rsa +#virtiofs myfs
I would move this line further up and group it the ext4 examples. At least for me this it makes it clearer that both cases are described in the "<fs type> <partition>" example provided in the comment above.
Agreed.
thanks
Site note: Does it make sense to mention the supported fs types in the comment above to reduce the confusion even more?
Do you mean something like: # <fs type> <partition> # - Will mount -t <fs type> <partition> <mnt>, and copy # /proc/vmcore to <mnt>/<path>/%HOST_IP-%DATE/. # NOTE: <partition> can be a device node, label or uuid. # It's recommended to use persistent device names # such as /dev/vg/<devname>. # Otherwise it's suggested to use label or uuid. # Currently accepted fs types are "ext[234] | xfs | btrfs | minix | nfs | virtiofs"
That's almost exactly what I had in mind. Although personally I would choose the human readable version, i.e.
Supported fs types: ext[234], xfs, btrfs, minix, virtiofs
Note, that I dropped nfs as that has an example for itself.
Sure, thanks for pointing it out, I will update it in v2.
Thanks, Tao Liu
Thanks Philipp
Thanks, Tao Liu
Thanks Philipp
auto_reset_crashkernel yes path /var/crash core_collector makedumpfile -l --message-level 7 -d 31 diff --git a/kdumpctl b/kdumpctl index 126ecb9..5b0e5fd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -239,7 +239,7 @@ parse_config() _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;
ext[234] | minix | btrfs | xfs | nfs | ssh)
ext[234] | minix | btrfs | xfs | nfs | ssh | virtiofs) _set_config _fstype "$config_opt" || return 1 config_opt=_target ;;@@ -478,8 +478,8 @@ check_fs_modified() fi
# No need to check in case of raw target.
# Currently we do not check also if ssh/nfs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target; then
# Currently we do not check also if ssh/nfs/virtiofs target is specifiedif is_ssh_dump_target || is_nfs_dump_target || is_raw_dump_target || is_virtiofs_dump_target; then return 0 fidiff --git a/mkdumprd b/mkdumprd index 3e250e0..958698e 100644 --- a/mkdumprd +++ b/mkdumprd @@ -391,7 +391,7 @@ while read -r config_opt config_val; do extra_modules) extra_modules="$extra_modules $config_val" ;;
ext[234] | xfs | btrfs | minix | nfs)
ext[234] | xfs | btrfs | minix | nfs | virtiofs) check_user_configured_target "$config_val" "$config_opt" add_mount "$config_val" "$config_opt" ;;
-
Philipp Rudo -
Tao Liu