Hi Tao,
On Wed, 10 Aug 2022 18:05:58 +0800
Tao Liu <ltao(a)redhat.com> wrote:
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
See my comment on squashfs patch.
> >
> > 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.
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" && 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
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" &&
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.
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 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"
> > ;;
>