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

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

On Fri, Dec 05, 2008 at 02:12:27PM -0500, Steve Dickson wrote:
> J. Bruce Fields wrote:
> > On Fri, Dec 05, 2008 at 11:38:24AM -0500, bfields 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.  Let's just skip the
> >> sync()s/fsncy()s in the !hosts case--that looks to me like the simplest
> >> correct solution for now.
> > 
> > My argument for correctness: if we don't sync in that case, then on
> > reboot the rename that updates the state will either have happened or
> > (if a crash comes too soon) not.
> > 
> > It is OK for that update to not happen as long as we're assured it
> > happens before the first lock request is made or replied to, or the
> > first monitor request completes, as, in the absence of any notifies,
> > those are the only points at which the new state will be exposed to the
> > outside world.
> Doesn't the sync() have to happen before the file is first
> read by stated. Meaning before statd:main() calls load_state_number()?

I can't see why.  sync() of course can't have any effect on a subsequent
read of the file.

> > The first lock request will also require an upcall to statd.  So we're
> > OK as long as any monitor requests (from either the local kernel or
> > remote peers) do a sync.
> > 
> > And statd should be doing a sync before responding to any monitor
> > request.  As long as the SM_DIR is on the same filesystem as the state
> > file, that would do the job....  But now that I look, I see statd is
> > using an open with O_SYNC to ensure the new statd record hits stable
> > storage.  Which we can't count on being enough.
> > 
> > How about adding an explicit fsync() of the state file (and parent
> > directory) to statd's first succesful creation of a statd record,
> > together with a comment explaining this?  So around about line 194 in
> > utils/statd/monitor.c:sm_mon_1_svc()?
> If we do the sync()/fsync() here we will also have to update MY_STATE
> since that's what is the number used in the RPCs.

I don't believe that's true.

> But also I think
> doing the sync this late be a bit waste since there is real good
> chance the rename has already been sync-ed out by previous sync()
> during boot up... or am I missing something... 

The whole boot gets held up waiting for this one sync to complete; those
later sync's mainly only delay bringing up nfs.

--b.