Oh and...
On Tue, 9 Aug 2022 17:27:35 +0200
Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
On Fri, 5 Aug 2022 17:40:10 +0800
Tao Liu <ltao(a)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:
> 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
... 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(a)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(a)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"
> ;;