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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
