Hi Philipp,
On Tue, Aug 9, 2022 at 11:27 PM 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
>
> 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.
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" && 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.
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" && 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.
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 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"
> ;;