Hi Dave,
It's no longer needed, actually, it's never used, the first version of
the udev rule update patch
introduced this variable, I forgot to remove it in later updates. So
just fix it btw.
On Thu, Dec 6, 2018 at 5:13 PM Dave Young <dyoung(a)redhat.com> wrote:
On 12/05/18 at 06:46pm, Kairui Song wrote:
> In commit 1c97aee and commit 227c185 udev rules was rewritten to use
> systemd-run to run in a non-blocking mode. The problem is that it's a
> bit noise, especially on machine bootup, systemd will always generate
> extra logs for service start, you might see your journal full of lines
> like these if you have many CPUs (each CPU generates a udev event on
> boot):
>
> ...
> Nov 22 22:23:05 localhost systemd[1]: Started /usr/lib/udev/kdump-udev-throttler.
> Nov 22 22:23:05 localhost systemd[1]: Started /usr/lib/udev/kdump-udev-throttler.
> Nov 22 22:23:05 localhost systemd[1]: Started /usr/lib/udev/kdump-udev-throttler.
> Nov 22 22:23:05 localhost systemd[1]: Started /usr/lib/udev/kdump-udev-throttler.
> ...
>
> While system is still booting up, kdump service is not started yet, so
> systemd-run calls will end up doing nothing, the throttler being called
> by systemd-run will just exit if kdump is not loaded.
>
> This patch avoid systemd-run from being called at first place if kdump
> service is not running by checking kdump.service status in udev rule,
> so there won't be unnecessary logs.
>
> Also remove the kdump service checking logic in kdump-udev-throttler as
> udev is the only expected callee of this script, if it's not being
> called at first place when kdump service is running, this checking will
> be redundant. And even if any user called this script manually, it will
> still work well as this script will call 'kdumpctl reload', it reload
> the kdump resource only if kdump is loaded already.
>
> Signed-off-by: Kairui Song <kasong(a)redhat.com>
> ---
> Update from V1:
> - Embed the conditional invocation logic in udev rule as suggested by
> Dave Young and Bhupesh Sharma
> - Add some comment in udev rule about why the conditional invocation is
> needed.
> - Remove the service status detect code in kdump-udev-throttle as it's
> no longer needed.
>
> 98-kexec.rules | 6 +++++-
> kdump-udev-throttler | 5 -----
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/98-kexec.rules b/98-kexec.rules
> index 2f88c77..b73b701 100644
> --- a/98-kexec.rules
> +++ b/98-kexec.rules
> @@ -7,6 +7,10 @@ GOTO="kdump_reload_end"
>
> LABEL="kdump_reload"
>
> -RUN+="/usr/bin/systemd-run --no-block
/usr/lib/udev/kdump-udev-throttler"
> +# If kdump is not loaded, calling kdump-udev-throttle will end up
> +# doing nothing, but systemd-run will always generate extra logs for
> +# each call, so trigger the kdump-udev-throttler only if kdump
> +# service is active to avoid unnecessary logs
> +RUN+="/bin/sh -c '/usr/bin/systemctl is-active kdump.service || exit 0;
/usr/bin/systemd-run --quiet --no-block /usr/lib/udev/kdump-udev-throttler'"
>
> LABEL="kdump_reload_end"
> diff --git a/kdump-udev-throttler b/kdump-udev-throttler
> index 6cbb99a..cd77a31 100755
> --- a/kdump-udev-throttler
> +++ b/kdump-udev-throttler
> @@ -13,12 +13,7 @@
> # In this way, we can make sure kdump service is restarted immediately
> # and for exactly once after udev events are settled.
>
> -
> throttle_lock="/var/lock/kdump-udev-throttle"
> -interval=2
> -
> -# Don't reload kdump service if kdump service is not started by systemd
> -systemctl is-active kdump.service &>/dev/null || exit 0
>
> exec 9>$throttle_lock
> if [ $? -ne 0 ]; then
> --
> 2.19.1
>
Thanks for the update, interval=2 is not needed any more?
Just want to make sure this is intended change.
Other than this,
Acked-by: Dave Young <dyoung(a)redhat.com>
Thanks
Dave
--
Best Regards,
Kairui Song