HI Philipp,
Thanks for your comments.
On Tue, Mar 1, 2022 at 1:45 AM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Lichen,
On Fri, 25 Feb 2022 16:10:19 +0800
Lichen Liu <lichliu(a)redhat.com> wrote:
> There is a system-wide sync call at the end of mkdumprd, move it to
> kdumpctl after rebuild initrd and add another one for mkfadumprd.
> Sync only the $TARGET_INITRD to avoid a system-wide sync taking too
> long on a system with high disk activity.
Nice, looks better than the solution I had in mind.
There are two situations where the patch changes things slightly but in
my opinion that is not a problem.
1) mkfadump also calls mkdumprd to create a temporary initrd. But as it
is only a temporary file not syncing in that situation shouldn't
make a difference.
2) mkdumprd and mkfadumprd could in theory be called directly by a
user. But that is strongly discouraged in the man page. So this
shouldn't be a problem either.
Yes, I see the man page says "mkdumprd was not intended for casual use
outside
of the service initialization script for the kdump utility, and
should
not be run manually", so I think it shouldn't be a problem.
Two small comments below
> Signed-off-by: Lichen Liu <lichliu(a)redhat.com>
> ---
> kdumpctl | 2 ++
> mkdumprd | 1 -
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 9fd76ac..fa83c05 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -79,6 +79,7 @@ rebuild_fadump_initrd()
> return 1
> fi
>
> + sync -f "$TARGET_INITRD"
> return 0
> }
>
> @@ -100,6 +101,7 @@ rebuild_kdump_initrd()
> dwarn "Tips: If early kdump is enabled, also require
rebuilding the system initramfs to make the changes take effect for early
kdump."
> fi
>
> + sync -f "$TARGET_INITRD"
> return 0
> }
>
> diff --git a/mkdumprd b/mkdumprd
> index 593ec77..d5f8945 100644
> --- a/mkdumprd
> +++ b/mkdumprd
> @@ -460,5 +460,4 @@ fi
> dracut "${dracut_args[@]}" "$@"
>
> _rc=$?
> -sync
> exit $_rc
Without the sync there is no longer a need to define _rc. In fact you
can remove everything below the call to dracut here.
Thanks for pointing this out.
There is one more 'plain' sync in kdumpctl:restore_default_initrd which
should be updated. Although the code there looks somewhat broken to
me...
I missed this one, I'll send a v2 patch containing this change.
Thanks
> Philipp
--
Best regards,
Lichen Liu