nfs-utils: sm-notify does not remove its pid file.

PATCH In review Steve Dickson 2 participants 6 messages Dec 10, 2007 – Dec 12, 2007 2 messages not in the archive

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2007-12-10 19:25 UTC #0 lore raw

PATCH 1/1 nfs-utils: sm-notify does not remove its pid file.

It turns out that the sm-notify command, used to notify clients that they need to reclaim their locks because the server just changed state, does not clean up its pid file. The pid file is used to ensure only one instance of the process is running. This results in sm-notify exiting before notifying any clients on the second reboot.

I also changed the directory in which the pid file is created from /var/run into BASEDIR so the file can be removed after the after the user id is changed to a non-root uid.

steved.

commit 72e24403c298b7991eb04dd68ed2e9fcbe4baf70

Author: Steve Dickson <[email protected]>
Date:   Mon Dec 10 12:17:29 2007 -0500

Make sure sm-notify's pid file is removed when the process exits. Also moved the pid file from /var/run into BASEDIR so the file can be removed after the user id is changed to a non-root uid.

Signed-off-by Steve Dickson <[email protected]>

utils/statd/sm-notify.c +13 -1

index bb67c37..3f80e04 100644
@@ -32,6 +32,10 @@
 # endif
 #endif
 
+#ifndef PIDFILE
+#define PIDFILE  BASEDIR "/sm-notify.pid"
+#endif
+
 #define DEFAULT_SM_STATE_PATH	BASEDIR "/state"
 #define	DEFAULT_SM_DIR_PATH	BASEDIR "/sm"
 #define	DEFAULT_SM_BAK_PATH	DEFAULT_SM_DIR_PATH ".bak"

@@ -89,6 +93,7 @@ void			nsm_log(int fac, const char *fmt, ...);
 static int		record_pid(void);
 static void		drop_privs(void);
 static void set_kernel_nsm_state(int state);
+static void remove_pid(void);
 
 static struct nsm_host *	hosts = NULL;

@@ -155,6 +160,7 @@ usage:		fprintf(stderr,
 			/* already run, don't try again */
 			exit(0);
 	}
+	atexit(remove_pid);
 
 	if (opt_srcaddr) {
 		strncpy(nsm_hostname, opt_srcaddr, sizeof(nsm_hostname)-1);

@@ -736,13 +742,19 @@ static int record_pid(void)
 	int fd;
 
 	snprintf(pid, 20, "%d\n", getpid());
-	fd = open("/var/run/sm-notify.pid", O_CREAT|O_EXCL|O_WRONLY, 0600);
+	fd = open(PIDFILE, O_CREAT|O_EXCL|O_WRONLY, 0600);
 	if (fd < 0)
 		return 0;
 	write(fd, pid, strlen(pid));
 	close(fd);
 	return 1;
 }
+static void remove_pid()
+{
+	if (unlink(PIDFILE) < 0)
+		nsm_log(LOG_WARNING,
+			"Unable to remove %s: %s", PIDFILE, strerror(errno));
+}
 
 /* Drop privileges to match owner of state-directory
  * (in case a reply triggers some unknown bug).

Neil Brown

2007-12-10 22:40 UTC #1 lore raw

Quoted 9 lines · On Monday December 10, [email protected] wrote:

It turns out that the sm-notify command, used to notify clients that they need to reclaim their locks because the server just changed state, does not clean up its pid file. The pid file is used to ensure only one instance of the process is running. This results in sm-notify exiting before notifying any clients on the second reboot.
I also changed the directory in which the pid file is created from /var/run into BASEDIR so the file can be removed after the after the user id is changed to a non-root uid.

I was under the impression that /var/run was always cleaned out on reboot, so this shouldn't be a problem. Is my impression wrong?

And I think the point of the lock file was the sm-notify would only run once per reboot. You really wont it once per lockd-forgets-all-locks so a kill -9 on lockd should be combined with removing the lock file.

But I think that removing the lock file when sm-notify completes is wrong.

Note the comment in sm-notify.c:

/*
* Record pid in /var/run/sm-notify.pid
* This file should remain until a reboot, even if the
* program exits.
* If file already exists, fail.
*/

NeilBrown

Steve Dickson

author

2007-12-11 01:26 UTC #3 lore raw

In reply to a message not in the archive

Quoted 2 lines · Neil Brown

I was under the impression that /var/run was always cleaned out on reboot, so this shouldn't be a problem. Is my impression wrong?

I don't think there are any guarantees about this. I was under the impression that the individual init scripts were suppose to clean those up and I know I fixed a few bugs in that area. ;-)

Quoted 3 lines

And I think the point of the lock file was the sm-notify would only run once per reboot.

Why impose a limit? Why not recover lock at any point?

Quoted 2 lines

But I think that removing the lock file when sm-notify completes is wrong.

A side effect of all this is if rpc.statd is restarted and that file is not cleaned up the client will never even try to recover any locks. That's definitely not a good thing... imho...

Quoted 9 lines

Note the comment in sm-notify.c:
/*
* Record pid in /var/run/sm-notify.pid
* This file should remain until a reboot, even if the
* program exits.
* If file already exists, fail.
*/

I guess I just don't understand why a pid file need to exist after a process is gone. Especially if its going to cause the next existence to imminently exit (potentially causing data corruption). That just does not seem very robust...

steved.

Neil Brown

2007-12-11 03:28 UTC #4 lore raw

Quoted 6 lines · On Monday December 10, [email protected] wrote:

Quoted 2 lines · Neil Brown
I was under the impression that /var/run was always cleaned out on reboot, so this shouldn't be a problem. Is my impression wrong?
I don't think there are any guarantees about this. I was under the impression that the individual init scripts were suppose to clean those up and I know I fixed a few bugs in that area. ;-)

Debian has /etc/init.d/bootclean which contains:

	cd /var/run ; .....
	....
        find . ! -xtype d ! -name utmp ! -name innd.pid \
                -print0 | xargs -0r rm -f -- \
		|| ....

so any files except a select few are removed.

SLES has /etc/init.d/boot.cleanup which has
find /var/run /var/lock -type f -print0 | xargs -0 -r rm -f 2>/dev/null

so again, all files get removed.

You seem to be saying that Redhat requires everything in /var/run to be explicitly cleaned up ??? Aren't standards wonderful :-)

Quoted 5 lines

Quoted 4 lines
And I think the point of the lock file was the sm-notify would only run once per reboot.
Why impose a limit? Why not recover lock at any point?

You only want to recover locks when they have been lost. Boot time is that time.

The particular situation I wanted to handle was that I wanted it to be
OK to start statd at any time. And I wanted sm-notify to run at boot
time if possible, but also to be run when statd was started if it hadn't run already.
An 'I have run already' file in /var/run seemed the obvious solution.

Quoted 6 lines

Quoted 3 lines
But I think that removing the lock file when sm-notify completes is wrong.
A side effect of all this is if rpc.statd is restarted and that file is not cleaned up the client will never even try to recover any locks. That's definitely not a good thing... imho...

There is no need for a client to recover locks if statd is restarted (is there). I think statd now stores all state in /var/lib/nfs/* so if it is killed and restarted, it should pick up exactly where it left off.

The time that you need to notify clients is when *lockd* is restarted. Or more significantly, when lockd arbitrarily drops it's locks.
i.e. if you "kill -9" lockd, then you need to do something with sm-notify and I acknowledge that isn't well documented nor is there an automatic procedure to make it happen. So there is room for improvement, but I don't think that running sm-notify every time that statd is started is an improvement.

Quoted 10 lines

Quoted 10 lines
Note the comment in sm-notify.c:
/*
* Record pid in /var/run/sm-notify.pid
* This file should remain until a reboot, even if the
* program exits.
* If file already exists, fail.
*/

Quoted 4 lines

I guess I just don't understand why a pid file need to exist after a process is gone. Especially if its going to cause the next existence to imminently exit (potentially causing data corruption). That just does not seem very robust...

Maybe making look like a pid file was a mistake. I'd be OK with changing the name to /var/run/sm-notify-has-run or similar. If we could store in the file some reflection of the state of lockd, that would be even better, but I don't think such a value is available at the moment.

What potential for data corruption do you see?

NeilBrown

Steve Dickson

author

2007-12-11 14:28 UTC #6 lore raw

In reply to a message not in the archive

Quoted 10 lines · Neil Brown

Debian has /etc/init.d/bootclean which contains:
	cd /var/run ; .....
	....
        find . ! -xtype d ! -name utmp ! -name innd.pid \
                -print0 | xargs -0r rm -f -- \
		|| ....
so any files except a select few are removed.

Quoted 7 lines

SLES has /etc/init.d/boot.cleanup which has
find /var/run /var/lock -type f -print0 | xargs -0 -r rm -f 2>/dev/null
so again, all files get removed.
You seem to be saying that Redhat requires everything in /var/run to be explicitly cleaned up ??? Aren't standards wonderful :-)

That appears to be the case... on both Fedora and RHEL...

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
And I think the point of the lock file was the sm-notify would only run once per reboot.
Why impose a limit? Why not recover lock at any point?
You only want to recover locks when they have been lost. Boot time is that time.

99.9% of time, I agree. But there are times when services are restarted without reboots.

Quoted 6 lines

The particular situation I wanted to handle was that I wanted it to be
OK to start statd at any time. And I wanted sm-notify to run at boot
time if possible, but also to be run when statd was started if it hadn't run already.
An 'I have run already' file in /var/run seemed the obvious solution.

I now do see the logical... But the fact that a file does or does not exist so should not be the deciding factor of whether a lock is or is not recovered. Shouldn't be the existence (or non-existence) of a lock that governs whether a lock is or is not recovered?

Quoted 10 lines

Quoted 5 lines
Quoted 2 lines
But I think that removing the lock file when sm-notify completes is wrong.
A side effect of all this is if rpc.statd is restarted and that file is not cleaned up the client will never even try to recover any locks. That's definitely not a good thing... imho...
There is no need for a client to recover locks if statd is restarted (is there). I think statd now stores all state in /var/lib/nfs/* so if it is killed and restarted, it should pick up exactly where it left off.

But if the 'nfslock' service is restarted (i.e. service nfslock restart), at least in Fedora and RHEL, the locks need to be reclaimed. Of course the init script can arbitrary remove the pid file, but isn't there (very small) chance that a sm-notify process could be still running, hung up in some type of network issue?

Quoted 9 lines

The time that you need to notify clients is when *lockd* is restarted. Or more significantly, when lockd arbitrarily drops it's locks.
i.e. if you "kill -9" lockd, then you need to do something with sm-notify and I acknowledge that isn't well documented nor is there an automatic procedure to make it happen. So there is room for improvement, but I don't think that running sm-notify every time that statd is started is an improvement.

Well, today, sm-notify is run every time statd is started. It just that sm-notify exits when the pid file exists. Instead maybe what should happen is sm-notify should exit when there is no work to be done (i.e. no locks to recover).

Of course this runs the risk of causing clients to unnecessarily reclaiming there locks, but, imho, that's okay! Its the price of restarted the nfslock service, something that should not happen too often anyways.

Also shouldn't there be some type of synchronization between statd and sm-notify? Meaning should statd wait until sm-notify is done doing its work (or at least creating the backup directory)?

Quoted 14 lines

Quoted 9 lines
Quoted 9 lines
Note the comment in sm-notify.c:
/*
* Record pid in /var/run/sm-notify.pid
* This file should remain until a reboot, even if the
* program exits.
* If file already exists, fail.
*/
Quoted 4 lines
I guess I just don't understand why a pid file need to exist after a process is gone. Especially if its going to cause the next existence to imminently exit (potentially causing data corruption). That just does not seem very robust...

Quoted 5 lines

Maybe making look like a pid file was a mistake. I'd be OK with changing the name to /var/run/sm-notify-has-run or similar. If we could store in the file some reflection of the state of lockd, that would be even better, but I don't think such a value is available at the moment.

Its just a pet peeve of mine for processes not to clean up after themselves. Leaving things around for other process to clean up just never seemed like a good idea to me.

Quoted 2 lines

What potential for data corruption do you see?

The nfslock service is restarted, the client locks are lost and no notification is sent out (because the existence of a pid file). This was what I was seeing when I started this thread.

steved.

Neil Brown

2007-12-12 23:11 UTC #7 lore raw

Quoted 11 lines · On Tuesday December 11, [email protected] wrote:

Quoted 10 lines · Neil Brown
Debian has /etc/init.d/bootclean which contains:
	cd /var/run ; .....
	....
        find . ! -xtype d ! -name utmp ! -name innd.pid \
                -print0 | xargs -0r rm -f -- \
		|| ....
so any files except a select few are removed.

Quoted 8 lines

Quoted 7 lines
SLES has /etc/init.d/boot.cleanup which has
find /var/run /var/lock -type f -print0 | xargs -0 -r rm -f 2>/dev/null
so again, all files get removed.
You seem to be saying that Redhat requires everything in /var/run to be explicitly cleaned up ??? Aren't standards wonderful :-)
That appears to be the case... on both Fedora and RHEL...

So after a crash and reboot there are lots of .pid file lying around from processes that obviously don't exist any more. That's sad.

What about /var/lock? That is arguably a better place to put it anyway.
Does /var/lock get cleaned of old files at reboot?

Quoted 11 lines

Quoted 9 lines
Quoted 6 lines
Quoted 5 lines
And I think the point of the lock file was the sm-notify would only run once per reboot.
Why impose a limit? Why not recover lock at any point?
You only want to recover locks when they have been lost. Boot time is that time.
99.9% of time, I agree. But there are times when services are restarted without reboots.

True. We need to make sure that when lockd is restarted, the sm-notify lock file is removed. It's a shame that everyone roles their own init.d files so we cannot usefully distribute something in nfs-utils.

Quoted 11 lines

Quoted 7 lines
The particular situation I wanted to handle was that I wanted it to be
OK to start statd at any time. And I wanted sm-notify to run at boot
time if possible, but also to be run when statd was started if it hadn't run already.
An 'I have run already' file in /var/run seemed the obvious solution.
I now do see the logical... But the fact that a file does or does not exist so should not be the deciding factor of whether a lock is or is not recovered. Shouldn't be the existence (or non-existence) of a lock that governs whether a lock is or is not recovered?

Maybe... let's think it through.

sm-notify meets to needs.
1/ It tells clients that the server has lost locks, and that they
could recover them.
2/ It tells servers that the client has dropped locks, and it should
drop all locks from that server too.

For '1', it is probably safe to ask a client to reclaim locks at any time. I cannot see an obvious problem, but I would want to test it to be sure. I think this it is possible for a server to tell if a given client is currently holding locks or not.

For '2', it is only safe to tell a server to drop locks if you are certain you don't currently hold any locks on that server. I cannot think how you would manage that. I don't think there is anything in /proc/locks that would tell you.

If you want a more direct connection between lockd being restarted and sm-notify running, I think you would need to get lockd to record when it restarted and make that information available somewhere. And sm-notify would record when it last ran. And you would compare those times somehow.

Quoted 12 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
But I think that removing the lock file when sm-notify completes is wrong.
Quoted 8 lines
Quoted 3 lines
A side effect of all this is if rpc.statd is restarted and that file is not cleaned up the client will never even try to recover any locks. That's definitely not a good thing... imho...
There is no need for a client to recover locks if statd is restarted (is there). I think statd now stores all state in /var/lib/nfs/* so if it is killed and restarted, it should pick up exactly where it left off.

Quoted 5 lines

But if the 'nfslock' service is restarted (i.e. service nfslock restart), at least in Fedora and RHEL, the locks need to be reclaimed. Of course the init script can arbitrary remove the pid file, but isn't there (very small) chance that a sm-notify process could be still running, hung up in some type of network issue?

I don't think that would matter. Having two "sm-notify"s running at the same time isn't really the problem. The problem is moving things from sm to sm.bak or updating 'state' when that is not appropriate.

Quoted 10 lines

Quoted 10 lines
The time that you need to notify clients is when *lockd* is restarted. Or more significantly, when lockd arbitrarily drops it's locks.
i.e. if you "kill -9" lockd, then you need to do something with sm-notify and I acknowledge that isn't well documented nor is there an automatic procedure to make it happen. So there is room for improvement, but I don't think that running sm-notify every time that statd is started is an improvement.

Quoted 4 lines

Well, today, sm-notify is run every time statd is started. It just that sm-notify exits when the pid file exists. Instead maybe what should happen is sm-notify should exit when there is no work to be done (i.e. no locks to recover).

How would you measure whether there are locks to recover or not?

Quoted 9 lines

Of course this runs the risk of causing clients to unnecessarily reclaiming there locks, but, imho, that's okay! Its the price of restarted the nfslock service, something that should not happen too often anyways.
Also shouldn't there be some type of synchronization between statd and sm-notify? Meaning should statd wait until sm-notify is done doing its work (or at least creating the backup directory)?

There is. sm-notify moves stuff to the backup directory, then forks and exits. statd waits until sm-notify forks.
I guess that if sm-notify is run separately from statd there could be a race, though if sm-notify is configured to run separately you would expect that is because statd won't be run for a while.... maybe there is room for improvement there.

Quoted 16 lines

Quoted 16 lines
Quoted 15 lines
Quoted 11 lines
Note the comment in sm-notify.c:
/*
* Record pid in /var/run/sm-notify.pid
* This file should remain until a reboot, even if the
* program exits.
* If file already exists, fail.
*/
I guess I just don't understand why a pid file need to exist after a process is gone. Especially if its going to cause the next existence to imminently exit (potentially causing data corruption). That just does not seem very robust...

Quoted 8 lines

Quoted 5 lines
Maybe making look like a pid file was a mistake. I'd be OK with changing the name to /var/run/sm-notify-has-run or similar. If we could store in the file some reflection of the state of lockd, that would be even better, but I don't think such a value is available at the moment.
Its just a pet peeve of mine for processes not to clean up after themselves. Leaving things around for other process to clean up just never seemed like a good idea to me.

Don't think of it as needing clean up. Think of it as recording state. We often use files for one program to record state that another program might want to access.

Quoted 6 lines

Quoted 3 lines
What potential for data corruption do you see?
The nfslock service is restarted, the client locks are lost and no notification is sent out (because the existence of a pid file). This was what I was seeing when I started this thread.

So "service nfslock restart" should ensure to remove the pid file when it sends "kill -9" to lockd?

Quoted 4 lines

steved.

NeilBrown