HI Philipp,

Thanks for your comments.

On Tue, Mar 1, 2022 at 1:45 AM Philipp Rudo <prudo@redhat.com> wrote:
Hi Lichen,

On Fri, 25 Feb 2022 16:10:19 +0800
Lichen Liu <lichliu@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@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