Re: Make sm-notify faster if there are no servers to notify

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Dec 05, 2008 at 01:42:18PM -0500, Steve Dickson wrote:
> J. Bruce Fields wrote:
> > On Fri, Dec 05, 2008 at 08:26:44AM -0500, Steve Dickson wrote:
> >> J. Bruce Fields wrote:
> >>>> I think it would still be valuable to replace the 'sync' with two
> >>>> 'fsync's, one of the file, one on the directory.
> >>> Sure, may as well.--b.
> >>>
> >> Something similar to this:
> >>
> >> diff -up nfs-utils/utils/statd/sm-notify.c.orig nfs-utils/utils/statd/sm-notify.c
> >> --- nfs-utils/utils/statd/sm-notify.c.orig	2008-11-17 15:06:13.000000000 -0500
> >> +++ nfs-utils/utils/statd/sm-notify.c	2008-12-05 08:21:52.000000000 -0500
> >> @@ -211,12 +211,6 @@ usage:		fprintf(stderr,
> >>  	backup_hosts(_SM_DIR_PATH, _SM_BAK_PATH);
> >>  	get_hosts(_SM_BAK_PATH);
> >>  
> >> -	/* If there are not hosts to notify, just exit */
> >> -	if (!hosts) {
> >> -		nsm_log(LOG_DEBUG, "No hosts to notify; exiting");
> >> -		return 0;
> >> -	}
> > 
> > This was still a huge boot-time win in the common case, so now that
> > we've committed to it I'd rather not regress.  
> I thought the idea was the state had to be updated even when there
> are no hosts...

I would prefer to increment the state file even in the absence of hosts
(it might be useful e.g. to a client that missed a previous grace period
due to a network problem to know that the current boot instance is later
than the one it needed to reclaim in).  But I don't believe it's
necessary.

The more immediate problem is that the state file no longer gets created
at all on a new system.   (Unless the package install scripts do it.  I
haven't checked.)

So, we could either make sure the state file gets created some other
way, or delay the syncs as described before.

> >> @@ -730,13 +725,16 @@ nsm_get_state(int update)
> >>  				"Failed to write state to %s", newfile);
> >>  			exit(1);
> >>  		}
> >> +		fsync(fd);
> >>  		close(fd);
> >> +		fp = opendir(_SM_STATE_PATH);
> > 
> > Also, I think you meant to:
> > 
> > 		fsync(fp);
> > 		close(fp);
> No... I don't think so... the fd is an open() of the new file and the
> fp is a DIR stream... But there was a cut/paste error... The fsync()
> of the DIR stream was missing... here is is again...

OK.

> @@ -694,6 +688,7 @@ nsm_get_state(int update)
>  {
>  	char		newfile[PATH_MAX];
>  	int		fd, state;
> +	DIR 	*fp;
>  
>  	if ((fd = open(_SM_STATE_PATH, O_RDONLY)) < 0) {
>  		if (!opt_quiet) {
> @@ -730,13 +725,18 @@ nsm_get_state(int update)
>  				"Failed to write state to %s", newfile);
>  			exit(1);
>  		}
> +		fsync(fd);
>  		close(fd);
> +		fp = opendir(_SM_STATE_PATH);
>  		if (rename(newfile, _SM_STATE_PATH) < 0) {
>  			nsm_log(LOG_ERR,
>  				"Cannot create %s: %m", _SM_STATE_PATH);
>  			exit(1);
>  		}
> -		sync();

May as well move that opendir down here where it's used, though.

> +		if (fp != NULL) {
> +			fsync(dirfd(fp));
> +			closedir(fp);
> +		}
>  	}
>  
>  	return state;

--b.