2022-07-27 18:34:11

by Steve Dickson

[permalink] [raw]
Subject: [PATCH] rpc-statd.service: Stop rpcbind and rpc.stat in an exit race

From: Benjamin Coddington <[email protected]>

When `systemctl stop rpcbind.socket` is run, the dependency means
that systemd first sends SIGTERM to rpcbind, then sigterm to rpc.statd.

On SIGTERM, rpcbind tears down /var/run/rpcbind.sock. However,
rpc-statd on SIGTERM attempts to unregister from rpcbind

systemd needs to wait for rpc.statd to exit before sending
SIGTERM to rpcbind

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2100395
Signed-off-by: Steve Dickson <[email protected]>
---
systemd/rpc-statd.service | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
index 095629f2..392750da 100644
--- a/systemd/rpc-statd.service
+++ b/systemd/rpc-statd.service
@@ -5,7 +5,7 @@ Conflicts=umount.target
Requires=nss-lookup.target rpcbind.socket
Wants=network-online.target
Wants=rpc-statd-notify.service
-After=network-online.target nss-lookup.target rpcbind.socket
+After=network-online.target nss-lookup.target rpcbind.service

PartOf=nfs-utils.service
IgnoreOnIsolate=yes
--
2.34.1


2022-07-28 11:20:40

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH] rpc-statd.service: Stop rpcbind and rpc.stat in an exit race

On 27 Jul 2022, at 13:24, Steve Dickson wrote:

> From: Benjamin Coddington <[email protected]>
>
> When `systemctl stop rpcbind.socket` is run, the dependency means
> that systemd first sends SIGTERM to rpcbind, then sigterm to rpc.statd.
>
> On SIGTERM, rpcbind tears down /var/run/rpcbind.sock. However,
> rpc-statd on SIGTERM attempts to unregister from rpcbind
>
> systemd needs to wait for rpc.statd to exit before sending
> SIGTERM to rpcbind
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2100395
> Signed-off-by: Steve Dickson <[email protected]>
> ---
> systemd/rpc-statd.service | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> index 095629f2..392750da 100644
> --- a/systemd/rpc-statd.service
> +++ b/systemd/rpc-statd.service
> @@ -5,7 +5,7 @@ Conflicts=umount.target
> Requires=nss-lookup.target rpcbind.socket
> Wants=network-online.target
> Wants=rpc-statd-notify.service
> -After=network-online.target nss-lookup.target rpcbind.socket
> +After=network-online.target nss-lookup.target rpcbind.service
>
> PartOf=nfs-utils.service
> IgnoreOnIsolate=yes
> --
> 2.34.1

Hey Steve - thanks for patchifying this - I should have sent a proper
patch, but the reason I didn't is that I didn't understand why we have the
rpcbind.socket unit, and why that unit isn't sufficient to enforce the
ordering.

I don't remember the history. Will changing the After= here create a
problem where rpcbind.service is up, but the socket isn't there yet, and
then rpc.statd comes up and can't find the socket?

Ben

2022-07-28 13:11:41

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] rpc-statd.service: Stop rpcbind and rpc.stat in an exit race

Hey!

On 7/28/22 7:15 AM, Benjamin Coddington wrote:
> On 27 Jul 2022, at 13:24, Steve Dickson wrote:
>
>> From: Benjamin Coddington <[email protected]>
>>
>> When `systemctl stop rpcbind.socket` is run, the dependency means
>> that systemd first sends SIGTERM to rpcbind, then sigterm to rpc.statd.
>>
>> On SIGTERM, rpcbind tears down /var/run/rpcbind.sock. However,
>> rpc-statd on SIGTERM attempts to unregister from rpcbind
>>
>> systemd needs to wait for rpc.statd to exit before sending
>> SIGTERM to rpcbind
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2100395
>> Signed-off-by: Steve Dickson <[email protected]>
>> ---
>> systemd/rpc-statd.service | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
>> index 095629f2..392750da 100644
>> --- a/systemd/rpc-statd.service
>> +++ b/systemd/rpc-statd.service
>> @@ -5,7 +5,7 @@ Conflicts=umount.target
>> Requires=nss-lookup.target rpcbind.socket
>> Wants=network-online.target
>> Wants=rpc-statd-notify.service
>> -After=network-online.target nss-lookup.target rpcbind.socket
>> +After=network-online.target nss-lookup.target rpcbind.service
>>
>> PartOf=nfs-utils.service
>> IgnoreOnIsolate=yes
>> --
>> 2.34.1
>
> Hey Steve - thanks for patchifying this - I should have sent a proper
> patch, but the reason I didn't is that I didn't understand why we have the
> rpcbind.socket unit, and why that unit isn't sufficient to enforce the
> ordering.
The whole idea be the rpcbind.socket unit was the Fedora or systemd
people did not want daemons running when the are not needed.
So they came up with "on-demand" daemons, meaning a daemon
would be started when a process connect to the socket.

>
> I don't remember the history. Will changing the After= here create a
> problem where rpcbind.service is up, but the socket isn't there yet, and
> then rpc.statd comes up and can't find the socket?
I don't think so... That After= line, in rpc-statd.service,
was a result of a bug fix I did for bz1419351 (commit 09e5c6c2a3).
Why I used rpcbind.socket instead of rpcbind.service is not
clear, since rpcbind.service Requires rpcbind.socket.

So I'm thinking this is a valid bug fix... Thanks
for doing the debugging!!!

steved.

2022-08-01 17:21:54

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH] rpc-statd.service: Stop rpcbind and rpc.stat in an exit race



On 7/27/22 1:24 PM, Steve Dickson wrote:
> Subject:
> [PATCH] rpc-statd.service: Stop rpcbind and rpc.stat in an exit race
> From:
> Steve Dickson <[email protected]>
> Date:
> 7/27/22, 1:24 PM
>
> To:
> Linux NFS Mailing list <[email protected]>
>
>
> From: Benjamin Coddington<[email protected]>
>
> When `systemctl stop rpcbind.socket` is run, the dependency means
> that systemd first sends SIGTERM to rpcbind, then sigterm to rpc.statd.
>
> On SIGTERM, rpcbind tears down /var/run/rpcbind.sock. However,
> rpc-statd on SIGTERM attempts to unregister from rpcbind
>
> systemd needs to wait for rpc.statd to exit before sending
> SIGTERM to rpcbind
>
> Fixes:https://bugzilla.redhat.com/show_bug.cgi?id=2100395
> Signed-off-by: Steve Dickson<[email protected]>
Committed...

steved.
> ---
> systemd/rpc-statd.service | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/systemd/rpc-statd.service b/systemd/rpc-statd.service
> index 095629f2..392750da 100644
> --- a/systemd/rpc-statd.service
> +++ b/systemd/rpc-statd.service
> @@ -5,7 +5,7 @@ Conflicts=umount.target
> Requires=nss-lookup.target rpcbind.socket
> Wants=network-online.target
> Wants=rpc-statd-notify.service
> -After=network-online.target nss-lookup.target rpcbind.socket
> +After=network-online.target nss-lookup.target rpcbind.service
>
> PartOf=nfs-utils.service
> IgnoreOnIsolate=yes
> -- 2.34.1
>