On 12/04/2018 04:36 PM, Dave Young wrote:
On 11/23/18 at 02:09pm, Kairui Song wrote:
> Hi, thanks for the review!
> I can use something like:
> RUN+="/bin/sh -c "systemctl is-active kdump.service &>/dev/null
&&
> /usr/bin/systemd-run --no-block /usr/lib/udev/kdump-udev-throttler""
> In the udev rule to conditionally run the script. But I think this one look more
> hacky, use a script is cleaner. I didn't find another doable way by just update
> udev rules.
I feel above is better and easy to get when one read it first time.
But embed the logic in scripts one need time to understand the logic :)
But I have no strong opinion about this..
Pingfan and Bhupesh are cced, see what do they think
I am fine with both. But I prefer the
"RUN+="/usr/lib/udev/kdump-udev-throttler --no-block" Since it avoids to
duplicate the code.
>>
>> I can add more comment/message to make the usage of kdump-udev-throttler
>> cleaner. After this patch, kdump-udev-throttler still do the same
>> thing, it just have
>> extra ability to run itself in non-blocking mode, so just consider
>> calling "kdump-udev-throttler"
>> and "kdump-udev-throttler --no-block" have same effect, the only
>> difference is the
>> later one run in background so never blocks. And both will just exit
>> if kdump is not running.
>> So udev could just call kdump-udev-throttle, and don't need to bother with
>> systemd-run directly, also avoided starting a transient systemd-run service
>> if kdump is not running.
>> On Fri, Nov 23, 2018 at 1:29 PM Dave Young <dyoung(a)redhat.com> wrote:
>>>
>>> Hi Kairui,
>>> On 11/22/18 at 11:05pm, 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, so there won't be unnecessary logs.
>>>>
>>>> As udev rules don't support shell expressions, this patch reuse
>>>> the kdump-udev-throttler script, adding a --no-block option.
>>>> kdump-udev-throttler will call it self with systemd-run to run in non
>>>> blocking mode if --no-block is given, and exit before doing anything if
>>>> kdump is not started. Udev just call "kdump-udev-throttler
--no-block"
>>>> directly.
>>>>
>>>> Signed-off-by: Kairui Song <kasong(a)redhat.com>
>>>> ---
>>>> 98-kexec.rules | 2 +-
>>>> kdump-udev-throttler | 13 ++++++++++---
>>>> 2 files changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/98-kexec.rules b/98-kexec.rules
>>>> index 2f88c77..eca909e 100644
>>>> --- a/98-kexec.rules
>>>> +++ b/98-kexec.rules
>>>> @@ -7,6 +7,6 @@ GOTO="kdump_reload_end"
>>>>
>>>> LABEL="kdump_reload"
>>>>
>>>> -RUN+="/usr/bin/systemd-run --no-block
/usr/lib/udev/kdump-udev-throttler"
>>>> +RUN+="/usr/lib/udev/kdump-udev-throttler --no-block"
>>>
>>> This fix looks a hack, if it doable to conditionally run
"/usr/lib/udev/kdump-udev-throttler --no-block"
>>> in udev rule?
>>>
>>>>
>>>> LABEL="kdump_reload_end"
>>>> diff --git a/kdump-udev-throttler b/kdump-udev-throttler
>>>> index 6cbb99a..6899379 100755
>>>> --- a/kdump-udev-throttler
>>>> +++ b/kdump-udev-throttler
>>>> @@ -13,13 +13,20 @@
>>>> # 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
>>>> +# Don't do anything if kdump service is not started by systemd
>>>> systemctl is-active kdump.service &>/dev/null || exit 0
>>>>
>>>> +if [[ $1 == '--no-block' ]]; then
>>>> + systemd-run --no-block --quiet $0
>>>> + if [[ $? -ne 0 ]]; then
>>>> + echo "kdump-udev-throttler: Failed to call
systemd-run"
>>>> + exit 1
>>>> + fi
>>>> + exit 0
>>>> +fi
>>>> +
>>>> exec 9>$throttle_lock
>>>> if [ $? -ne 0 ]; then
>>>> echo "Failed to create the lock file! Fallback to
non-throttled kdump service restart"
>>>> --
>>>> 2.19.1
>>>>
>>>
>>> Thanks
>>> Dave
>>
>>
>>
>> --
>> Best Regards,
>> Kairui Song