[PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

Currently, when auth_reload is called, it only looks at the tv_sec field
of the mtime when deciding whether to invalidate the exports cache. It's
fairly simple to fool this by doing something like:

# exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo

With this, exportfs will show the export for /foo, but mountd will still have
whatever contents were in /etc/exports. The issue is that the etab is being
updated twice during the same second, and mountd is reading in the file in
between updates. When it goes to look at the file again, its timestamp matches
the timestamp of the cache, and it ends up keeping the cached contents even
though the file has changed.

While not all local filesystems provide sub-second timestamps, we might as
well fix this problem on those that do. The following patch changes
auth_reload to consider the tv_nsec field of the mtime when deciding whether
to invalidate the export cache. It also fixes up the callers to pass it a
pointer to a struct timespec for it to fill out.

I've not yet tested this on a filesystem that provides sub-second timestamps,
so I'm not clear on how well this works yet, but it seems to not break
anything on ext3 in some cursory testing.

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..54e7516 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -42,28 +42,34 @@ auth_init(char *exports)
 {
 
 	export_file = exports;
-	auth_reload();
+	auth_reload(NULL);
 	xtab_mount_write();
 }
 
-time_t
-auth_reload()
+void
+auth_reload(struct timespec *ts)
 {
 	struct stat		stb;
-	static time_t		last_modified = 0;
+	static struct timespec	last_modified;
 
 	if (stat(_PATH_ETAB, &stb) < 0)
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+	if (stb.st_mtim.tv_sec == last_modified.tv_sec &&
+	    stb.st_mtim.tv_nsec == last_modified.tv_nsec)
+		goto out;
 
+	last_modified.tv_sec = stb.st_mtim.tv_sec;
+	last_modified.tv_nsec = stb.st_mtim.tv_nsec;
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
 	// export_read(export_file);
 	xtab_export_read();
-
-	return last_modified;
+out:
+	if (ts) {
+		ts->tv_sec = stb.st_mtim.tv_sec;
+		ts->tv_nsec = stb.st_mtim.tv_nsec;
+	}
+	return;
 }
 
 static nfs_export *
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d068843..77c559f 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -90,7 +90,7 @@ void auth_unix_ip(FILE *f)
 	if (inet_aton(ipaddr, &addr)==0)
 		return;
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* addr is a valid, interesting address, find the domain name... */
 	client = client_compose(addr);
@@ -354,7 +354,7 @@ void nfsd_fh(FILE *f)
 		break;
 	}
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Now determine export point for this fsid/domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
@@ -575,7 +575,7 @@ void nfsd_export(FILE *f)
 	if (qword_get(&cp, path, strlen(lbuf)) <= 0)
 		goto out;
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* now find flags for this export point in this domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..96d25a1 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -241,7 +241,7 @@ bool_t
 mount_umntall_1_svc(struct svc_req *rqstp, void *argp, void *resp)
 {
 	/* Reload /etc/xtab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	mountlist_del_all((struct sockaddr_in *) svc_getcaller(rqstp->rq_xprt));
 	return 1;
@@ -299,7 +299,7 @@ mount_pathconf_2_svc(struct svc_req *rqstp, dirpath *path, ppathcnf *res)
 		p = "/";
 
 	/* Reload /etc/xtab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Resolve symlinks */
 	if (realpath(p, rpath) != NULL) {
@@ -373,7 +373,7 @@ get_rootfh(struct svc_req *rqstp, dirpath *path, mountstat3 *error, int v3)
 		p = "/";
 
 	/* Reload /var/lib/nfs/etab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Resolve symlinks */
 	if (realpath(p, rpath) != NULL) {
@@ -465,18 +465,20 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
+	static struct timespec	etime;
+	struct timespec		atime;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
 
-	atime = auth_reload();
-	if (elist && atime == etime)
+	auth_reload(&atime);
+	if (elist && atime.tv_sec == etime.tv_sec &&
+	    atime.tv_nsec == etime.tv_nsec)
 		return elist;
 
-	etime = atime;
+	etime.tv_sec = atime.tv_sec;
+	etime.tv_nsec = atime.tv_nsec;
 
 	for (e = elist; e != NULL; e = ne) {
 		ne = e->ex_next;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..58694d9 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,7 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+void 		auth_reload(struct timespec *ts);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Wed, Apr 25, 2007 at 01:39:20PM -0400, Jeff Layton wrote:
> Currently, when auth_reload is called, it only looks at the tv_sec field
> of the mtime when deciding whether to invalidate the exports cache. It's
> fairly simple to fool this by doing something like:
> 
> # exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo
> 
> With this, exportfs will show the export for /foo, but mountd will still have
> whatever contents were in /etc/exports. The issue is that the etab is being
> updated twice during the same second, and mountd is reading in the file in
> between updates. When it goes to look at the file again, its timestamp matches
> the timestamp of the cache, and it ends up keeping the cached contents even
> though the file has changed.
> 
> While not all local filesystems provide sub-second timestamps, we might as
> well fix this problem on those that do. The following patch changes
> auth_reload to consider the tv_nsec field of the mtime when deciding whether
> to invalidate the export cache. It also fixes up the callers to pass it a
> pointer to a struct timespec for it to fill out.
> 
> I've not yet tested this on a filesystem that provides sub-second timestamps,
> so I'm not clear on how well this works yet, but it seems to not break
> anything on ext3 in some cursory testing.
> 

No sooner than I post than I see a (minor) problem. The check for a NULL
pointer in auth_reload should be (ts != NULL) instead of (ts). Respun patch
follows:

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..274e560 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -42,28 +42,34 @@ auth_init(char *exports)
 {
 
 	export_file = exports;
-	auth_reload();
+	auth_reload(NULL);
 	xtab_mount_write();
 }
 
-time_t
-auth_reload()
+void
+auth_reload(struct timespec *ts)
 {
 	struct stat		stb;
-	static time_t		last_modified = 0;
+	static struct timespec	last_modified;
 
 	if (stat(_PATH_ETAB, &stb) < 0)
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+	if (stb.st_mtim.tv_sec == last_modified.tv_sec &&
+	    stb.st_mtim.tv_nsec == last_modified.tv_nsec)
+		goto out;
 
+	last_modified.tv_sec = stb.st_mtim.tv_sec;
+	last_modified.tv_nsec = stb.st_mtim.tv_nsec;
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
 	// export_read(export_file);
 	xtab_export_read();
-
-	return last_modified;
+out:
+	if (ts != NULL) {
+		ts->tv_sec = stb.st_mtim.tv_sec;
+		ts->tv_nsec = stb.st_mtim.tv_nsec;
+	}
+	return;
 }
 
 static nfs_export *
diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index d068843..77c559f 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -90,7 +90,7 @@ void auth_unix_ip(FILE *f)
 	if (inet_aton(ipaddr, &addr)==0)
 		return;
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* addr is a valid, interesting address, find the domain name... */
 	client = client_compose(addr);
@@ -354,7 +354,7 @@ void nfsd_fh(FILE *f)
 		break;
 	}
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Now determine export point for this fsid/domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
@@ -575,7 +575,7 @@ void nfsd_export(FILE *f)
 	if (qword_get(&cp, path, strlen(lbuf)) <= 0)
 		goto out;
 
-	auth_reload();
+	auth_reload(NULL);
 
 	/* now find flags for this export point in this domain */
 	for (i=0 ; i < MCL_MAXTYPES; i++) {
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..96d25a1 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -241,7 +241,7 @@ bool_t
 mount_umntall_1_svc(struct svc_req *rqstp, void *argp, void *resp)
 {
 	/* Reload /etc/xtab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	mountlist_del_all((struct sockaddr_in *) svc_getcaller(rqstp->rq_xprt));
 	return 1;
@@ -299,7 +299,7 @@ mount_pathconf_2_svc(struct svc_req *rqstp, dirpath *path, ppathcnf *res)
 		p = "/";
 
 	/* Reload /etc/xtab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Resolve symlinks */
 	if (realpath(p, rpath) != NULL) {
@@ -373,7 +373,7 @@ get_rootfh(struct svc_req *rqstp, dirpath *path, mountstat3 *error, int v3)
 		p = "/";
 
 	/* Reload /var/lib/nfs/etab if necessary */
-	auth_reload();
+	auth_reload(NULL);
 
 	/* Resolve symlinks */
 	if (realpath(p, rpath) != NULL) {
@@ -465,18 +465,20 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
+	static struct timespec	etime;
+	struct timespec		atime;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
 
-	atime = auth_reload();
-	if (elist && atime == etime)
+	auth_reload(&atime);
+	if (elist && atime.tv_sec == etime.tv_sec &&
+	    atime.tv_nsec == etime.tv_nsec)
 		return elist;
 
-	etime = atime;
+	etime.tv_sec = atime.tv_sec;
+	etime.tv_nsec = atime.tv_nsec;
 
 	for (e = elist; e != NULL; e = ne) {
 		ne = e->ex_next;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..58694d9 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,7 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+void 		auth_reload(struct timespec *ts);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Wed, Apr 25, 2007 at 02:09:34PM -0400, Jeff Layton wrote:
> On Wed, Apr 25, 2007 at 01:39:20PM -0400, Jeff Layton wrote:
> > Currently, when auth_reload is called, it only looks at the tv_sec field
> > of the mtime when deciding whether to invalidate the exports cache. It's
> > fairly simple to fool this by doing something like:
> > 
> > # exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo
> > 
> > With this, exportfs will show the export for /foo, but mountd will still have
> > whatever contents were in /etc/exports. The issue is that the etab is being
> > updated twice during the same second, and mountd is reading in the file in
> > between updates. When it goes to look at the file again, its timestamp matches
> > the timestamp of the cache, and it ends up keeping the cached contents even
> > though the file has changed.
> > 
> > While not all local filesystems provide sub-second timestamps, we might as
> > well fix this problem on those that do. The following patch changes
> > auth_reload to consider the tv_nsec field of the mtime when deciding whether
> > to invalidate the export cache. It also fixes up the callers to pass it a
> > pointer to a struct timespec for it to fill out.
> > 
> > I've not yet tested this on a filesystem that provides sub-second timestamps,
> > so I'm not clear on how well this works yet, but it seems to not break
> > anything on ext3 in some cursory testing.
> > 
> 
> No sooner than I post than I see a (minor) problem. The check for a NULL
> pointer in auth_reload should be (ts != NULL) instead of (ts). Respun patch
> follows:
> 

I tested this on a filesystem that does nanosecond timestamps (xfs), and it
seems to correct the original problem.
-- Jeff


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

Jeff Layton wrote:
> On Wed, Apr 25, 2007 at 02:09:34PM -0400, Jeff Layton wrote:
>> On Wed, Apr 25, 2007 at 01:39:20PM -0400, Jeff Layton wrote:
>>> Currently, when auth_reload is called, it only looks at the tv_sec field
>>> of the mtime when deciding whether to invalidate the exports cache. It's
>>> fairly simple to fool this by doing something like:
>>>
>>> # exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo
>>>
>>> With this, exportfs will show the export for /foo, but mountd will still have
>>> whatever contents were in /etc/exports. The issue is that the etab is being
>>> updated twice during the same second, and mountd is reading in the file in
>>> between updates. When it goes to look at the file again, its timestamp matches
>>> the timestamp of the cache, and it ends up keeping the cached contents even
>>> though the file has changed.
>>>
>>> While not all local filesystems provide sub-second timestamps, we might as
>>> well fix this problem on those that do. The following patch changes
>>> auth_reload to consider the tv_nsec field of the mtime when deciding whether
>>> to invalidate the export cache. It also fixes up the callers to pass it a
>>> pointer to a struct timespec for it to fill out.
>>>
>>> I've not yet tested this on a filesystem that provides sub-second timestamps,
>>> so I'm not clear on how well this works yet, but it seems to not break
>>> anything on ext3 in some cursory testing.
>>>
>> No sooner than I post than I see a (minor) problem. The check for a NULL
>> pointer in auth_reload should be (ts != NULL) instead of (ts). Respun patch
>> follows:
>>
> 
> I tested this on a filesystem that does nanosecond timestamps (xfs), and it
> seems to correct the original problem.

Hi Jeff-

I don't know much about the export logic, but why not use inode change 
notification instead of time stamps?

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 336 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
email;internet:chuck.lever@nospam-oracle.com
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel/
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Neil Brown]

On Wednesday April 25, chuck.lever@oracle.com wrote:
> 
> Hi Jeff-
> 
> I don't know much about the export logic, but why not use inode change 
> notification instead of time stamps?

inotify is 2.6 only I think.  I'm not sure I want to exclude 2.4
people just yet (or do I ??).

Given that exportfs replaces the file with a new one, would we need to
get notification on the directory, or just on the file?  Does it make
it more complicated, or easier?

Actually, mountd could open etab and hold it open. auth_reload could
stat "/var/lib/nfs/etab" and if it is a different file, then the
content is presumably different, while if it is the same file, we can
be certain the content is unchanged.

Further, we would be certain only to read each version once - zero
risk of re-parsing data when there is no change.

That might be an even better option .... anyone want to code it up??

Or is it wrong to depend on current exportfs behaviour?

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 02:56:42PM +1000, Neil Brown wrote:
> On Wednesday April 25, chuck.lever@oracle.com wrote:
> > 
> > Hi Jeff-
> > 
> > I don't know much about the export logic, but why not use inode change 
> > notification instead of time stamps?
> 
> inotify is 2.6 only I think.  I'm not sure I want to exclude 2.4
> people just yet (or do I ??).
> 

My feeling is that we should choose the best solution that fits in with
"current" kernels (for some definition of "current"). Most people running
old kernels aren't going to be running bleeding-edge nfs-utils anyway...


> Given that exportfs replaces the file with a new one, would we need to
> get notification on the directory, or just on the file?  Does it make
> it more complicated, or easier?
> 

I did a little looking at inotify and I think we can monitor more or less
anything that happens to the file. It might take a bit of experimentation to
get the right event mask, but it doesn't look to bad.

> Actually, mountd could open etab and hold it open. auth_reload could
> stat "/var/lib/nfs/etab" and if it is a different file, then the
> content is presumably different, while if it is the same file, we can
> be certain the content is unchanged.
> 
> Further, we would be certain only to read each version once - zero
> risk of re-parsing data when there is no change.
> 
> That might be an even better option .... anyone want to code it up??
> 
> Or is it wrong to depend on current exportfs behaviour?

That does seem like the simplest solution (and simple is good). I also don't
see an issue with depending on current exportfs behavior. If we are going to
depend on that though, then we should probably include some comments around
the code that implements that behavior to warn people who might want to change
it.

I'll take a crack at coding that up.

-- Jeff


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 08:05:37AM -0400, Jeff Layton wrote:
> On Thu, Apr 26, 2007 at 02:56:42PM +1000, Neil Brown wrote:
> > On Wednesday April 25, chuck.lever@oracle.com wrote:
> > > 
> > > Hi Jeff-
> > > 
> > > I don't know much about the export logic, but why not use inode change 
> > > notification instead of time stamps?
> > 
> > inotify is 2.6 only I think.  I'm not sure I want to exclude 2.4
> > people just yet (or do I ??).
> > 
> 
> My feeling is that we should choose the best solution that fits in with
> "current" kernels (for some definition of "current"). Most people running
> old kernels aren't going to be running bleeding-edge nfs-utils anyway...
> 
> 
> > Given that exportfs replaces the file with a new one, would we need to
> > get notification on the directory, or just on the file?  Does it make
> > it more complicated, or easier?
> > 
> 
> I did a little looking at inotify and I think we can monitor more or less
> anything that happens to the file. It might take a bit of experimentation to
> get the right event mask, but it doesn't look to bad.
> 
> > Actually, mountd could open etab and hold it open. auth_reload could
> > stat "/var/lib/nfs/etab" and if it is a different file, then the
> > content is presumably different, while if it is the same file, we can
> > be certain the content is unchanged.
> > 
> > Further, we would be certain only to read each version once - zero
> > risk of re-parsing data when there is no change.
> > 
> > That might be an even better option .... anyone want to code it up??
> > 
> > Or is it wrong to depend on current exportfs behaviour?
> 
> That does seem like the simplest solution (and simple is good). I also don't
> see an issue with depending on current exportfs behavior. If we are going to
> depend on that though, then we should probably include some comments around
> the code that implements that behavior to warn people who might want to change
> it.
> 
> I'll take a crack at coding that up.
> 
> -- Jeff
> 

I've had a look at this, and even took a pretty broad swipe at doing it. The
issue here is that a lot of the existing code is not well geared toward having
an already-open filehandle that gets passed around. Changing this is going to
mean a pretty invasive patch that adds quite a few new functions. I don't feel
confident that this method has enough advantages over your earlier patch to
make this worthwhile.

Neil, what do you think? Should I continue down this road, or should we just
go with what you've already written?

-- Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[J. Bruce Fields]

On Thu, Apr 26, 2007 at 12:23:04PM -0400, Jeff Layton wrote:
> I've had a look at this, and even took a pretty broad swipe at doing it. The
> issue here is that a lot of the existing code is not well geared toward having
> an already-open filehandle that gets passed around. Changing this is going to
> mean a pretty invasive patch that adds quite a few new functions. I don't feel
> confident that this method has enough advantages over your earlier patch to
> make this worthwhile.
> 
> Neil, what do you think? Should I continue down this road, or should we just
> go with what you've already written?

Wouldn't you get the same benefit just by storing (and comparing) the
inode number and mtime of etab, instead of just the mtime?

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 12:58:00PM -0400, J. Bruce Fields wrote:
> On Thu, Apr 26, 2007 at 12:23:04PM -0400, Jeff Layton wrote:
> > I've had a look at this, and even took a pretty broad swipe at doing it. The
> > issue here is that a lot of the existing code is not well geared toward having
> > an already-open filehandle that gets passed around. Changing this is going to
> > mean a pretty invasive patch that adds quite a few new functions. I don't feel
> > confident that this method has enough advantages over your earlier patch to
> > make this worthwhile.
> > 
> > Neil, what do you think? Should I continue down this road, or should we just
> > go with what you've already written?
> 
> Wouldn't you get the same benefit just by storing (and comparing) the
> inode number and mtime of etab, instead of just the mtime?
> 
> --b.

Not if the inode number is reused. That can occur on some filesystems if
nothing is holding it open. Then again, if we're opening a new file and
renaming it to the old name, then that might not be an issue here. Let me
play with that idea and see...

-- Jeff



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[J. Bruce Fields]

On Thu, Apr 26, 2007 at 01:00:56PM -0400, Jeff Layton wrote:
> Not if the inode number is reused. That can occur on some filesystems if
> nothing is holding it open. Then again, if we're opening a new file and
> renaming it to the old name, then that might not be an issue here. Let me
> play with that idea and see...

Hm.  Grepping around: I didn't realize ext2/3/4 seem to have a
GETVERSION ioctl that returns the generation number.  But I don't know
if there's any more filesystem-independent way to do that.

(Is there some better way?  I don't understand why people would work so
hard on change notification without having a simple way to poll for
changes as well--otherwise you lose track of everything the moment the
watcher goes away, and you end up with beagle having to reread your
entire filesystem after every reboot.)

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 01:24:44PM -0400, J. Bruce Fields wrote:
> On Thu, Apr 26, 2007 at 01:00:56PM -0400, Jeff Layton wrote:
> > Not if the inode number is reused. That can occur on some filesystems if
> > nothing is holding it open. Then again, if we're opening a new file and
> > renaming it to the old name, then that might not be an issue here. Let me
> > play with that idea and see...
> 
> Hm.  Grepping around: I didn't realize ext2/3/4 seem to have a
> GETVERSION ioctl that returns the generation number.  But I don't know
> if there's any more filesystem-independent way to do that.
> 
> (Is there some better way?  I don't understand why people would work so
> hard on change notification without having a simple way to poll for
> changes as well--otherwise you lose track of everything the moment the
> watcher goes away, and you end up with beagle having to reread your
> entire filesystem after every reboot.)
> 
> --b.

I hadn't considered using the inode generation, but I'm not sure that that
will be portable enough. I think though, that Neil's plan of holding the
inode open may work ok.

This patch seems to fix the problem as well. mountd holds the etab open, but
doesn't actually do any I/O on that fd. This makes sure that the inode number
of the new etab file won't be the same as the current version that mountd is
aware of. I also added a comment to xtab_write to warn people who might
consider changing how the files are written.

We might also consider adding back in some of the mtime logic to try and
catch programs that edit the file in place, but I'm not sure if that's
worthwhile.

Neil, what do you think?

-- Jeff


diff --git a/support/export/xtab.c b/support/export/xtab.c
index 0ddb251..292087b 100644
--- a/support/export/xtab.c
+++ b/support/export/xtab.c
@@ -80,6 +80,12 @@ xtab_export_read(void)
 	return xtab_read(_PATH_ETAB, 1);
 }
 
+/*
+ * mountd now keeps an open fd for the etab at all times to make sure that the
+ * inode number changes when the xtab_export_write is done. If you change the
+ * routine below such that the files are edited in place, then you'll need to
+ * fix the auth_reload logic as well...
+ */
 static int
 xtab_write(char *xtab, char *xtabtmp, int is_export)
 {
diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..a0a034d 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -14,6 +14,7 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <errno.h>
+#include <unistd.h>
 #include "misc.h"
 #include "nfslib.h"
 #include "exportfs.h"
@@ -46,24 +47,32 @@ auth_init(char *exports)
 	xtab_mount_write();
 }
 
-time_t
+ino_t
 auth_reload()
 {
 	struct stat		stb;
-	static time_t		last_modified = 0;
+	static ino_t		last_inode;
+	static int		last_fd;
+	int			fd;
 
-	if (stat(_PATH_ETAB, &stb) < 0)
+	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {
+		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
+	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+	} else if (stb.st_ino == last_inode) {
+		close(fd);
+		return last_inode;
+	} else {
+		close(last_fd);
+		last_fd = fd;
+		last_inode = stb.st_ino;
+	}
 
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
-	// export_read(export_file);
 	xtab_export_read();
 
-	return last_modified;
+	return last_inode;
 }
 
 static nfs_export *
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..eea8483 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -465,18 +465,18 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
+	static ino_t		einode;
+	ino_t			ainode;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
 
-	atime = auth_reload();
-	if (elist && atime == etime)
+	ainode = auth_reload();
+	if (elist && ainode == einode)
 		return elist;
 
-	etime = atime;
+	einode = ainode;
 
 	for (e = elist; e != NULL; e = ne) {
 		ne = e->ex_next;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..4e507e6 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,7 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+ino_t		auth_reload(void);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Neil Brown]

On Thursday April 26, jlayton@redhat.com wrote:
> 
> This patch seems to fix the problem as well. mountd holds the etab open, but
> doesn't actually do any I/O on that fd. This makes sure that the inode number
> of the new etab file won't be the same as the current version that mountd is
> aware of. I also added a comment to xtab_write to warn people who might
> consider changing how the files are written.
> 
> We might also consider adding back in some of the mtime logic to try and
> catch programs that edit the file in place, but I'm not sure if that's
> worthwhile.

I agree.

> 
> Neil, what do you think?

I was about to say "fine", but then saw the problem.
Because of the way get_exportlist uses the return value, that value
needs to unique over all time (or close to it).  So returning the
inode number won't work (as we have seen that ext3 cycles between
two).

A simple event counter could be returned though, which would give
something unique to get_exportlist.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, May 03, 2007 at 10:55:37AM +1000, Neil Brown wrote:
> On Thursday April 26, jlayton@redhat.com wrote:
> > 
> > This patch seems to fix the problem as well. mountd holds the etab open, but
> > doesn't actually do any I/O on that fd. This makes sure that the inode number
> > of the new etab file won't be the same as the current version that mountd is
> > aware of. I also added a comment to xtab_write to warn people who might
> > consider changing how the files are written.
> > 
> > We might also consider adding back in some of the mtime logic to try and
> > catch programs that edit the file in place, but I'm not sure if that's
> > worthwhile.
> 
> I agree.
> 
> > 
> > Neil, what do you think?
> 
> I was about to say "fine", but then saw the problem.
> Because of the way get_exportlist uses the return value, that value
> needs to unique over all time (or close to it).  So returning the
> inode number won't work (as we have seen that ext3 cycles between
> two).
> 
> A simple event counter could be returned though, which would give
> something unique to get_exportlist.
> 
> NeilBrown

Ahh, good catch. I wasn't considering that mountd might call into auth_reload
from other functions between get_exportlist calls. I'm not sure we even really
need a static counter here though. How about just having auth_reload return
true if the reload occurred?

Signed-off-by: Jeff Layton <jlayton@redhat.com>


diff --git a/support/export/xtab.c b/support/export/xtab.c
index 0ddb251..292087b 100644
--- a/support/export/xtab.c
+++ b/support/export/xtab.c
@@ -80,6 +80,12 @@ xtab_export_read(void)
 	return xtab_read(_PATH_ETAB, 1);
 }
 
+/*
+ * mountd now keeps an open fd for the etab at all times to make sure that the
+ * inode number changes when the xtab_export_write is done. If you change the
+ * routine below such that the files are edited in place, then you'll need to
+ * fix the auth_reload logic as well...
+ */
 static int
 xtab_write(char *xtab, char *xtabtmp, int is_export)
 {
diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..5638a05 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -14,6 +14,7 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <errno.h>
+#include <unistd.h>
 #include "misc.h"
 #include "nfslib.h"
 #include "exportfs.h"
@@ -46,24 +47,33 @@ auth_init(char *exports)
 	xtab_mount_write();
 }
 
-time_t
+/* Check the etab file. Reload it and return true if it changed */
+int
 auth_reload()
 {
 	struct stat		stb;
-	static time_t		last_modified = 0;
+	static ino_t		last_inode;
+	static int		last_fd;
+	int			fd;
 
-	if (stat(_PATH_ETAB, &stb) < 0)
+	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {
+		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
+	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+	} else if (stb.st_ino == last_inode) {
+		close(fd);
+		return 0;
+	} else {
+		close(last_fd);
+		last_fd = fd;
+		last_inode = stb.st_ino;
+	}
 
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
-	// export_read(export_file);
 	xtab_export_read();
 
-	return last_modified;
+	return 1;
 }
 
 static nfs_export *
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..c3324cf 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -465,19 +465,14 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
 
-	atime = auth_reload();
-	if (elist && atime == etime)
+	if (auth_reload() == 0)
 		return elist;
 
-	etime = atime;
-
 	for (e = elist; e != NULL; e = ne) {
 		ne = e->ex_next;
 		for (g = e->ex_groups; g != NULL; g = ng) {
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..d64c171 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,7 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+int		auth_reload(void);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, May 03, 2007 at 07:58:15AM -0400, Jeff Layton wrote:
> On Thu, May 03, 2007 at 10:55:37AM +1000, Neil Brown wrote:
> > On Thursday April 26, jlayton@redhat.com wrote:
> > > 
> > > This patch seems to fix the problem as well. mountd holds the etab open, but
> > > doesn't actually do any I/O on that fd. This makes sure that the inode number
> > > of the new etab file won't be the same as the current version that mountd is
> > > aware of. I also added a comment to xtab_write to warn people who might
> > > consider changing how the files are written.
> > > 
> > > We might also consider adding back in some of the mtime logic to try and
> > > catch programs that edit the file in place, but I'm not sure if that's
> > > worthwhile.
> > 
> > I agree.
> > 
> > > 
> > > Neil, what do you think?
> > 
> > I was about to say "fine", but then saw the problem.
> > Because of the way get_exportlist uses the return value, that value
> > needs to unique over all time (or close to it).  So returning the
> > inode number won't work (as we have seen that ext3 cycles between
> > two).
> > 
> > A simple event counter could be returned though, which would give
> > something unique to get_exportlist.
> > 
> > NeilBrown
> 
> Ahh, good catch. I wasn't considering that mountd might call into auth_reload
> from other functions between get_exportlist calls. I'm not sure we even really
> need a static counter here though. How about just having auth_reload return
> true if the reload occurred?
> 

Doh! That won't work for the same reason. Static counter it is...

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/support/export/xtab.c b/support/export/xtab.c
index 0ddb251..292087b 100644
--- a/support/export/xtab.c
+++ b/support/export/xtab.c
@@ -80,6 +80,12 @@ xtab_export_read(void)
 	return xtab_read(_PATH_ETAB, 1);
 }
 
+/*
+ * mountd now keeps an open fd for the etab at all times to make sure that the
+ * inode number changes when the xtab_export_write is done. If you change the
+ * routine below such that the files are edited in place, then you'll need to
+ * fix the auth_reload logic as well...
+ */
 static int
 xtab_write(char *xtab, char *xtabtmp, int is_export)
 {
diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..f7fe23d 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -14,6 +14,7 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <errno.h>
+#include <unistd.h>
 #include "misc.h"
 #include "nfslib.h"
 #include "exportfs.h"
@@ -46,24 +47,34 @@ auth_init(char *exports)
 	xtab_mount_write();
 }
 
-time_t
+unsigned int
 auth_reload()
 {
 	struct stat		stb;
-	static time_t		last_modified = 0;
-
-	if (stat(_PATH_ETAB, &stb) < 0)
+	static ino_t		last_inode;
+	static int		last_fd;
+	static unsigned int	counter;
+	int			fd;
+
+	if ((fd = open(_PATH_ETAB, O_RDONLY)) < 0) {
+		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
+	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+	} else if (stb.st_ino == last_inode) {
+		close(fd);
+		return counter;
+	} else {
+		close(last_fd);
+		last_fd = fd;
+		last_inode = stb.st_ino;
+	}
 
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
-	// export_read(export_file);
 	xtab_export_read();
+	++counter;
 
-	return last_modified;
+	return counter;
 }
 
 static nfs_export *
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..999f035 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -465,18 +465,18 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
+	static unsigned int	ecounter;
+	unsigned int		acounter;
 
-	atime = auth_reload();
-	if (elist && atime == etime)
+	acounter = auth_reload();
+	if (elist && acounter == ecounter)
 		return elist;
 
-	etime = atime;
+	ecounter = acounter;
 
 	for (e = elist; e != NULL; e = ne) {
 		ne = e->ex_next;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..31bacb5 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,7 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+unsigned int	auth_reload(void);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 01:00:56PM -0400, Jeff Layton wrote:
> On Thu, Apr 26, 2007 at 12:58:00PM -0400, J. Bruce Fields wrote:
> > On Thu, Apr 26, 2007 at 12:23:04PM -0400, Jeff Layton wrote:
> > > I've had a look at this, and even took a pretty broad swipe at doing it. The
> > > issue here is that a lot of the existing code is not well geared toward having
> > > an already-open filehandle that gets passed around. Changing this is going to
> > > mean a pretty invasive patch that adds quite a few new functions. I don't feel
> > > confident that this method has enough advantages over your earlier patch to
> > > make this worthwhile.
> > > 
> > > Neil, what do you think? Should I continue down this road, or should we just
> > > go with what you've already written?
> > 
> > Wouldn't you get the same benefit just by storing (and comparing) the
> > inode number and mtime of etab, instead of just the mtime?
> > 
> > --b.
> 
> Not if the inode number is reused. That can occur on some filesystems if
> nothing is holding it open. Then again, if we're opening a new file and
> renaming it to the old name, then that might not be an issue here. Let me
> play with that idea and see...
> 
> -- Jeff
> 

Yep, that won't fix the problem on my test rig (excuse the ugly reproducer
line and output):

# exportfs -ua ; pkill rpc.mountd ; sleep 1 ; exportfs -rv ; stat /var/lib/nfs/etab && rpc.mountd -d all && exportfs -uva ; stat /var/lib/nfs/etab && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo ; stat /var/lib/nfs/etab && exportfs -v && showmount -e 127.0.0.1
exporting *:/export
  File: `/var/lib/nfs/etab'
  Size: 165             Blocks: 16         IO Block: 4096   regular file
Device: fd00h/64768d    Inode: 2091908     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2007-04-26 13:11:17.000000000 -0400
Modify: 2007-04-26 13:11:17.000000000 -0400
Change: 2007-04-26 13:11:17.000000000 -0400
  File: `/var/lib/nfs/etab'
  Size: 0               Blocks: 8          IO Block: 4096   regular empty file
Device: fd00h/64768d    Inode: 2091920     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2007-04-26 13:11:17.000000000 -0400
Modify: 2007-04-26 13:11:17.000000000 -0400
Change: 2007-04-26 13:11:17.000000000 -0400
exporting 127.0.0.1:/foo
  File: `/var/lib/nfs/etab'
  Size: 162             Blocks: 16         IO Block: 4096   regular file
Device: fd00h/64768d    Inode: 2091908     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2007-04-26 13:11:17.000000000 -0400
Modify: 2007-04-26 13:11:17.000000000 -0400
Change: 2007-04-26 13:11:17.000000000 -0400
/foo            127.0.0.1(rw,wdelay,no_root_squash,no_subtree_check)
Export list for 127.0.0.1:
/foo 127.0.0.1

This was testing with Neil's latest patch, which does seem to fix the
issue.  Anyway, on this test with /var/lib/nfs on ext3, the exportfs -uva
changed the inode number, but the next exportfs command immediately changed
it back. So if Neil's patch wasn't present, then I don't think mountd would
have picked up the changes. I'm not sure that tracking the inode number alone
will give us any real benefit.

My earlier patch was focused on keeping the etab held open by mountd and doing
all I/O to that file via the held-open fd. Maybe that's overkill, and we should
keep most of the existing logic (where we open and close the file) and simply
have an 'extra' fd to keep inode numbers unique. I'll give that a try and see
if it's more reasonable...

-- Jeff


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[J. Bruce Fields]

On Thu, Apr 26, 2007 at 02:56:42PM +1000, Neil Brown wrote:
> Actually, mountd could open etab and hold it open. auth_reload could
> stat "/var/lib/nfs/etab" and if it is a different file, then the
> content is presumably different, while if it is the same file, we can
> be certain the content is unchanged.
> 
> Further, we would be certain only to read each version once - zero
> risk of re-parsing data when there is no change.
> 
> That might be an even better option .... anyone want to code it up??
> 
> Or is it wrong to depend on current exportfs behaviour?

Would it be worth continuing to check the mtime as well, at least for a
transitional period?  (And if we see the file modified without being
replaced, we could emit a warning asking the user to notify us how that
happened.)

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Neil Brown]

On Wednesday April 25, jlayton@redhat.com wrote:

Hi Jeff,

> Currently, when auth_reload is called, it only looks at the tv_sec field
> of the mtime when deciding whether to invalidate the exports cache. It's
> fairly simple to fool this by doing something like:
> 
> # exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo

Yes... real problem.  Not sure I like your solution.
It obviously helps but it isn't complete (e.g. if filesystem only has
1-second resolution) and I think we can do better.

You can trade performance for accuracy by checking the current time as
well.
Something like:

   now = time(0);
   stat(_PATH_ETAB, &stb);

   if (stb.st_mtime < last_loaded)
	return;
   last_loaded = now;
   // read file.

That way if the file gets changed, we then read the file every time we
want information until the second changes.  Then we read it once
more and hold on to that copy.
This might mean we read it several times in one second when only one
update was made, but at least it means we never miss an update.

Of course if the filesystem does provide better resolution, we should
use that, but we can only use it if we know that the resolution is -
and I don't think it is easy to extract that information.

But! We can get some hints from 'atime'.  I think it safe to assume
that the resolution of atime is the same as that of ctime. so maybe
something like:

    stat(_PATH_ETAB, &stb);
    if (tspec_cmp(last_loaded, stb.st_mtim) < 0)
		return;
    last_loaded = stb.st_atim;

(excuse the pseudo-code.  Assume tspec_cmp compares struct timespec
 with return value similar to strcmp).

Here, 'last_loaded' represents a time period of a size which matches
the time-resolution of the filesystem.  We know that time period ends
after the last time the file was modified, and it begins before the
last time we read the file.
If it is after the time-period when the file was change, we don't need
to read the file again, otherwise we do.

Now if the filesystem as mounted with noatime, this will leave us
always re-reading the file, so we need to assume a minimum resolution
of 1 second, and if atime < now, update atime to equal now.

And if the system clock was pushed back substantially, we would not
read the file until after it had caught up again, so we better leave the
"== last_modified" test in there.


Or maybe we could just note that 'exportfs' always replaced the file
when it updates it, and just check to see if the inode number is
different... though inode number re-use will defeat that (ext3
alternated between two) and we don't have access to a generation
number... better stick with time stamps.

So I would like the propose the following enhancement of your patch.

Do you agree?

Thanks,
NeilBrown

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 183c9ea..7c99ee3 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -46,24 +46,44 @@ auth_init(char *exports)
 	xtab_mount_write();
 }
 
-time_t
+struct timespec
 auth_reload()
 {
+	/* "load_loaded" is an estimate of the last time we
+	 * read the file, and importantly has the same resolution as
+	 * the filesystem stores timestamps in.  We only avoid reading
+	 * if last_loaded is certain to be after the last time the file
+	 * was modified.
+	 * "last_modified" is used to guard against the system clock being
+	 * set back, which would confuse the last_loaded calculations.
+	 */
 	struct stat		stb;
-	static time_t		last_modified = 0;
+	static struct timespec	last_modified;
+	static struct timespec	last_loaded;
+	time_t			now = time(0);
 
 	if (stat(_PATH_ETAB, &stb) < 0)
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	if (stb.st_mtime == last_modified)
-		return last_modified;
-	last_modified = stb.st_mtime;
+
+	if (timespec_cmp(&stb.st_mtim, &last_loaded) < 0 &&
+	    timespec_cmp(&stb.st_mtim, &last_modified) == 0)
+		return last_loaded;
+
+	last_loaded = stb.st_atim;
+	last_modified = stb.st_mtim;
+
+	/* careful of -o noatime */
+	if (last_loaded.tv_sec < now) {
+		last_loaded.tv_sec = now;
+		last_loaded.tv_nsec = 0;
+	}
 
 	export_freeall();
 	memset(&my_client, 0, sizeof(my_client));
 	// export_read(export_file);
 	xtab_export_read();
 
-	return last_modified;
+	return last_loaded;
 }
 
 static nfs_export *
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 04141d1..c16f344 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -465,15 +465,15 @@ static exports
 get_exportlist(void)
 {
 	static exports		elist = NULL;
-	static time_t		etime = 0;
-	time_t			atime;
+	static struct timespec	etime;
+	struct timespec		atime;
 	struct exportnode	*e, *ne;
 	struct groupnode	*g, *ng, *c, **cp;
 	nfs_export		*exp;
 	int			i;
 
 	atime = auth_reload();
-	if (elist && atime == etime)
+	if (elist && timespec_cmp(&atime, &etime) == 0)
 		return elist;
 
 	etime = atime;
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index b539278..3a780fa 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -40,7 +40,16 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
 void		auth_init(char *export_file);
-time_t		auth_reload(void);
+
+static inline long int timespec_cmp(struct timespec *t1, struct timespec *t2)
+{
+	if (t1->tv_sec == t2->tv_sec)
+		return t1->tv_nsec - t2->tv_nsec;
+	else
+		return t1->tv_sec - t2->tv_sec;
+}
+
+struct timespec	auth_reload(void);
 nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
 					char *path);
 void		auth_export(nfs_export *exp);

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] nfs-utils: make auth_reload respect sub-second timestamps on etab

[Jeff Layton]

On Thu, Apr 26, 2007 at 02:50:34PM +1000, Neil Brown wrote:
> On Wednesday April 25, jlayton@redhat.com wrote:
> 
> Hi Jeff,
> 
> > Currently, when auth_reload is called, it only looks at the tv_sec field
> > of the mtime when deciding whether to invalidate the exports cache. It's
> > fairly simple to fool this by doing something like:
> > 
> > # exportfs -rv && rpc.mountd && exportfs -uva && exportfs -iv -o no_root_squash,rw 127.0.0.1:/foo
> 
> Yes... real problem.  Not sure I like your solution.
> It obviously helps but it isn't complete (e.g. if filesystem only has
> 1-second resolution) and I think we can do better.
> 
> You can trade performance for accuracy by checking the current time as
> well.
> Something like:
> 
>    now = time(0);
>    stat(_PATH_ETAB, &stb);
> 
>    if (stb.st_mtime < last_loaded)
> 	return;
>    last_loaded = now;
>    // read file.
> 
> That way if the file gets changed, we then read the file every time we
> want information until the second changes.  Then we read it once
> more and hold on to that copy.
> This might mean we read it several times in one second when only one
> update was made, but at least it means we never miss an update.
> 
> Of course if the filesystem does provide better resolution, we should
> use that, but we can only use it if we know that the resolution is -
> and I don't think it is easy to extract that information.
> 
> But! We can get some hints from 'atime'.  I think it safe to assume
> that the resolution of atime is the same as that of ctime. so maybe
> something like:
> 
>     stat(_PATH_ETAB, &stb);
>     if (tspec_cmp(last_loaded, stb.st_mtim) < 0)
> 		return;
>     last_loaded = stb.st_atim;
> 
> (excuse the pseudo-code.  Assume tspec_cmp compares struct timespec
>  with return value similar to strcmp).
> 
> Here, 'last_loaded' represents a time period of a size which matches
> the time-resolution of the filesystem.  We know that time period ends
> after the last time the file was modified, and it begins before the
> last time we read the file.
> If it is after the time-period when the file was change, we don't need
> to read the file again, otherwise we do.
> 
> Now if the filesystem as mounted with noatime, this will leave us
> always re-reading the file, so we need to assume a minimum resolution
> of 1 second, and if atime < now, update atime to equal now.
> 
> And if the system clock was pushed back substantially, we would not
> read the file until after it had caught up again, so we better leave the
> "== last_modified" test in there.
> 
> 
> Or maybe we could just note that 'exportfs' always replaced the file
> when it updates it, and just check to see if the inode number is
> different... though inode number re-use will defeat that (ext3
> alternated between two) and we don't have access to a generation
> number... better stick with time stamps.
> 
> So I would like the propose the following enhancement of your patch.
> 
> Do you agree?
> 
> Thanks,
> NeilBrown
> 
> diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
> index 183c9ea..7c99ee3 100644
> --- a/utils/mountd/auth.c
> +++ b/utils/mountd/auth.c
> @@ -46,24 +46,44 @@ auth_init(char *exports)
>  	xtab_mount_write();
>  }
>  
> -time_t
> +struct timespec
>  auth_reload()
>  {
> +	/* "load_loaded" is an estimate of the last time we
> +	 * read the file, and importantly has the same resolution as
> +	 * the filesystem stores timestamps in.  We only avoid reading
> +	 * if last_loaded is certain to be after the last time the file
> +	 * was modified.
> +	 * "last_modified" is used to guard against the system clock being
> +	 * set back, which would confuse the last_loaded calculations.
> +	 */
>  	struct stat		stb;
> -	static time_t		last_modified = 0;
> +	static struct timespec	last_modified;
> +	static struct timespec	last_loaded;
> +	time_t			now = time(0);
>  
>  	if (stat(_PATH_ETAB, &stb) < 0)
>  		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
> -	if (stb.st_mtime == last_modified)
> -		return last_modified;
> -	last_modified = stb.st_mtime;
> +
> +	if (timespec_cmp(&stb.st_mtim, &last_loaded) < 0 &&
> +	    timespec_cmp(&stb.st_mtim, &last_modified) == 0)
> +		return last_loaded;
> +
> +	last_loaded = stb.st_atim;
> +	last_modified = stb.st_mtim;
> +
> +	/* careful of -o noatime */
> +	if (last_loaded.tv_sec < now) {
> +		last_loaded.tv_sec = now;
> +		last_loaded.tv_nsec = 0;
> +	}
>  
>  	export_freeall();
>  	memset(&my_client, 0, sizeof(my_client));
>  	// export_read(export_file);
>  	xtab_export_read();
>  
> -	return last_modified;
> +	return last_loaded;
>  }
>  
>  static nfs_export *
> diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
> index 04141d1..c16f344 100644
> --- a/utils/mountd/mountd.c
> +++ b/utils/mountd/mountd.c
> @@ -465,15 +465,15 @@ static exports
>  get_exportlist(void)
>  {
>  	static exports		elist = NULL;
> -	static time_t		etime = 0;
> -	time_t			atime;
> +	static struct timespec	etime;
> +	struct timespec		atime;
>  	struct exportnode	*e, *ne;
>  	struct groupnode	*g, *ng, *c, **cp;
>  	nfs_export		*exp;
>  	int			i;
>  
>  	atime = auth_reload();
> -	if (elist && atime == etime)
> +	if (elist && timespec_cmp(&atime, &etime) == 0)
>  		return elist;
>  
>  	etime = atime;
> diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
> index b539278..3a780fa 100644
> --- a/utils/mountd/mountd.h
> +++ b/utils/mountd/mountd.h
> @@ -40,7 +40,16 @@ bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
>  
>  void		mount_dispatch(struct svc_req *, SVCXPRT *);
>  void		auth_init(char *export_file);
> -time_t		auth_reload(void);
> +
> +static inline long int timespec_cmp(struct timespec *t1, struct timespec *t2)
> +{
> +	if (t1->tv_sec == t2->tv_sec)
> +		return t1->tv_nsec - t2->tv_nsec;
> +	else
> +		return t1->tv_sec - t2->tv_sec;
> +}
> +
> +struct timespec	auth_reload(void);
>  nfs_export *	auth_authenticate(char *what, struct sockaddr_in *sin,
>  					char *path);
>  void		auth_export(nfs_export *exp);

No quarrel with your modification of the patch. It does seem like it would
close some holes in mine. That said, I'm starting to think that either Chuck's
suggestion to use inotify or your suggestion to hold the file open might be
a better way to go here. Let me think on that and I'll respond to the other
thread...

-- Jeff


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs