[PATCH] Access cache forgetting mode bits

[PATCH] Access cache forgetting mode bits

[Steve Dickson]

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

While debugging something else, I notice that every time
I did a ls on a nfs mounted directory a wto ACCESS rpc was
*always* happening (with a  2.4.21 kernel). Taking a closer
look it turns out the access cache  was dropping previous
access bits. Here is the scenario...

nfs_permission is called with mode = MAY_EXEC
the cached value is Zero so an ACCESS rpc is done.
after the rpc, the cached value is _set_ to MAY_EXEC

then nfs_permission is called with mode = MAY_READ
the cached value is MAY_EXEC, so an ACCESS rpc is done.
after the rpc, the cache value is _set_ to MAY_READ

again, nfs_permission is called with mode = MAY_EXEC
this time the cached value is MAY_READ so _again_ an RPC is done
after the rpc, the cached value is _set_ to MAY_EXEC

Its a vicious cycle...

So this patch changes the setting of the masked value
to the or-ing of the masked value. This cause the
previous bits to be remembered thus cutting down on
the number of otw rpcs...


SteveD.

[-- Attachment #2: linux-2.4.21-nfs-accesscache.patch --]
[-- Type: text/plain, Size: 364 bytes --]

--- linux-2.4.21/fs/nfs/dir.c.diff	2003-08-28 12:13:22.000000000 -0400
+++ linux-2.4.21/fs/nfs/dir.c	2003-08-28 12:13:47.000000000 -0400
@@ -1278,7 +1278,7 @@ nfs_permission(struct inode *inode, int 
 		if (cache->cred)
 			put_rpccred(cache->cred);
 		cache->cred = cred;
-		cache->mask = mask;
+		cache->mask |= mask;
 		cache->err = error;
 		return error;
 	}

Re: [PATCH] Access cache forgetting mode bits

[Andreas Gruenbacher]

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

Hi,

your idea idea is good, but the patch is incorrect in at least two ways:
(1) it merges the cached results of successful and unsuccessful
requests, (2) [access("file", R_OK) == 0] and [access("file", W_OK) ==
0] does not imply [access("file", R_OK | W_OK) == 0].

Luckily, [access("file", R_OK | W_OK) == 0] does imply both
[access("file", R_OK) == 0] and [access("file", W_OK) == 0] though. We
could take this to our advantage, and optimize for the common case. How
about the attached patch? In the worst case, with "weird" permissions
this will double the number of RPCs needed, but this will not happen on
"real" systems.


The NFS access cache has a problem if mounted file systems are accessed
by multiple users: The results of only a single user are cached. When
another user comes along, cache trashing similar to what Steve has
observed will occur.

Steve, you seem to be looking into the nfsacl extensions as well. ACLs
are not cached on the client side, so operations like `ls -l' (which
need to check which files have ACLs) also produce lots of RPCs. I don't
see this as a significant problem at this point.


On Thu, 2003-08-28 at 18:39, Steve Dickson wrote:
> While debugging something else, I notice that every time
> I did a ls on a nfs mounted directory a wto ACCESS rpc was
> *always* happening (with a  2.4.21 kernel). Taking a closer
> look it turns out the access cache  was dropping previous
> access bits. Here is the scenario...
> 
> nfs_permission is called with mode = MAY_EXEC
> the cached value is Zero so an ACCESS rpc is done.
> after the rpc, the cached value is _set_ to MAY_EXEC
> 
> then nfs_permission is called with mode = MAY_READ
> the cached value is MAY_EXEC, so an ACCESS rpc is done.
> after the rpc, the cache value is _set_ to MAY_READ
> 
> again, nfs_permission is called with mode = MAY_EXEC
> this time the cached value is MAY_READ so _again_ an RPC is done
> after the rpc, the cached value is _set_ to MAY_EXEC
> 
> Its a vicious cycle...
> 
> So this patch changes the setting of the masked value
> to the or-ing of the masked value. This cause the
> previous bits to be remembered thus cutting down on
> the number of otw rpcs...
> 
> 
> SteveD.
> 
> ______________________________________________________________________
> 
> --- linux-2.4.21/fs/nfs/dir.c.diff	2003-08-28 12:13:22.000000000 -0400
> +++ linux-2.4.21/fs/nfs/dir.c	2003-08-28 12:13:47.000000000 -0400
> @@ -1278,7 +1278,7 @@ nfs_permission(struct inode *inode, int 
>  		if (cache->cred)
>  			put_rpccred(cache->cred);
>  		cache->cred = cred;
> -		cache->mask = mask;
> +		cache->mask |= mask;
>  		cache->err = error;
>  		return error;
>  	}


Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SuSE Labs, SuSE Linux AG <http://www.suse.de/>


[-- Attachment #2: nfsd-optimistic-caching.diff --]
[-- Type: text/plain, Size: 961 bytes --]

Index: linux-2.4.21/fs/nfs/dir.c
===================================================================
--- linux-2.4.21.orig/fs/nfs/dir.c	2003-08-28 16:50:33.000000000 +0200
+++ linux-2.4.21/fs/nfs/dir.c	2003-08-28 23:19:27.000000000 +0200
@@ -1266,6 +1266,13 @@ nfs_permission(struct inode *inode, int 
 			/* Is the mask a subset of an accepted mask? */
 			if ((cache->mask & mask) == mask)
 				goto out_cached;
+			/* Try with expanded credentials. */
+			error = NFS_PROTO(inode)->access(inode, cred,
+							 mask | cache->mask);
+			if (!error || !cache->mask) {
+				mask |= cache->mask;
+				goto rpc_done;
+			}
 	       	} else {
 			/* ...or is it a superset of a rejected mask? */
 			if ((cache->mask & mask) == cache->mask)
@@ -1273,6 +1280,7 @@ nfs_permission(struct inode *inode, int 
 		}
 	}
 	error = NFS_PROTO(inode)->access(inode, cred, mask);
+rpc_done:
 	if (!error || error == -EACCES) {
 		cache->jiffies = jiffies;
 		if (cache->cred)

Re: [PATCH] Access cache forgetting mode bits

[Steve Dickson]

Andreas Gruenbacher wrote:

>your idea idea is good, but the patch is incorrect in at least two ways:
>(1) it merges the cached results of successful and unsuccessful
>requests, (2) [access("file", R_OK) == 0] and [access("file", W_OK) ==
>0] does not imply [access("file", R_OK | W_OK) == 0].
>
Good point!

>[....] How about the attached patch? In the worst case, with "weird" permissions
>this will double the number of RPCs needed, but this will not happen on
>"real" systems.
>
Yes... your patch does seem to work with "normal" cases.... which is 
probably the
best we can hope for...

>The NFS access cache has a problem if mounted file systems are accessed
>by multiple users: The results of only a single user are cached. When
>another user comes along, cache trashing similar to what Steve has
>observed will occur.
>  
>
True... its too bad the cred cache doesn't use uids and gids....

>Steve, you seem to be looking into the nfsacl extensions as well. ACLs
>are not cached on the client side, so operations like `ls -l' (which
>need to check which files have ACLs) also produce lots of RPCs. I don't
>see this as a significant problem at this point.
>
Caches are always a good idea wrt to NFS... but I guess only time will 
tell wrt nfsacls...

SteveD.



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] Access cache forgetting mode bits

[Steve Dickson]

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

Hello,

I'm thinking there might even be a better way to cache these bits...

It seems the OS calls nfs_permission() to test each permission bit which 
in turn
causes an otw access rpc for every call (assuming the same person and same
bit is *not* asked for ) and even with Andreas patch's there are at 
least two
rpcs for one access call....

Now what if on the first access call, we get *all* of the access bits 
from the
server and cache them locally (i.e. in cache->mask). That way when 
nfs_permission()
is called again, all if the needed info is there... The attached patch 
does just that
and it really seems to cut down on the amount access rpc needed...

Thoughts?

SteveD.


[-- Attachment #2: linux-2.4.21-nfs-accesscache.patch --]
[-- Type: text/plain, Size: 364 bytes --]

--- linux-2.4.21/fs/nfs/dir.c.diff	2003-08-28 12:13:22.000000000 -0400
+++ linux-2.4.21/fs/nfs/dir.c	2003-08-28 12:13:47.000000000 -0400
@@ -1278,7 +1278,7 @@ nfs_permission(struct inode *inode, int 
 		if (cache->cred)
 			put_rpccred(cache->cred);
 		cache->cred = cred;
-		cache->mask = mask;
+		cache->mask |= mask;
 		cache->err = error;
 		return error;
 	}

Re: [PATCH] Access cache forgetting mode bits

[Andreas Gruenbacher]

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

On Sun, 2003-08-31 at 22:33, Steve Dickson wrote:
> Hello,
> 
> I'm thinking there might even be a better way to cache these bits...
> 
> It seems the OS calls nfs_permission() to test each permission

Individually you mean? That's not at all the case in general.

> [...] even with Andreas patch's there are at least two rpcs for one access call....

No, two different, consecutice permission checks will result in two
RPCs.

It is possible to optimize another common case: Directory accesses are
seldom denied, and users are usually granted both r and x at once. The
attached patch optimizes that; it's the same pattern as in my first
patch. I'm in doubt whether this optimization is worth it, though.

For non-directories, I don't think a common enough case exists that
would deserve optimization.

> what if on the first access call, we get *all* of the access bits 
> from the
> server and cache them locally (i.e. in cache->mask). That way when 
> nfs_permission()
> is called again, all if the needed info is there... The attached patch 
> does just that
> and it really seems to cut down on the amount access rpc needed...
>
> Thoughts?

You have again attached your first, buggy patch.


Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SuSE Labs, SuSE Linux AG <http://www.suse.de/>


[-- Attachment #2: nfs-more-optimistic.diff --]
[-- Type: text/plain, Size: 660 bytes --]

Index: linux-2.4.21/fs/nfs/dir.c
===================================================================
--- linux-2.4.21.orig/fs/nfs/dir.c	2003-09-01 00:46:26.000000000 +0200
+++ linux-2.4.21/fs/nfs/dir.c	2003-09-01 01:09:27.000000000 +0200
@@ -1278,6 +1278,13 @@ nfs_permission(struct inode *inode, int 
 			if ((cache->mask & mask) == cache->mask)
 				goto out_cached;
 		}
+	} else if (S_ISDIR(mode)) {
+		int try_mask = mask | S_IROTH | S_IXOTH;
+		error = NFS_PROTO(inode)->access(inode, cred, try_mask);
+		if (!error || mask == try_mask) {
+			mask = try_mask;
+			goto rpc_done;
+		}
 	}
 	error = NFS_PROTO(inode)->access(inode, cred, mask);
 rpc_done:

Re: [PATCH] Access cache forgetting mode bits

[Steve Dickson]

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

Andreas Gruenbacher wrote:

>On Sun, 2003-08-31 at 22:33, Steve Dickson wrote:
>  
>
>>
>>I'm thinking there might even be a better way to cache these bits...
>>
>>It seems the OS calls nfs_permission() to test each permission
>>    
>>
>
>Individually you mean? That's not at all the case in general.
>
Yes...  So at times there will be more than one bit set in the mask?
I know that's how it works in other OS but I was not sure with linux...

>>what if on the first access call, we get *all* of the access bits 
>>from the
>>server and cache them locally (i.e. in cache->mask). That way when 
>>nfs_permission()
>>is called again, all if the needed info is there... The attached patch 
>>does just that
>>and it really seems to cut down on the amount access rpc needed...
>>
>>Thoughts?
>>    
>>
>
>You have again attached your first, buggy patch.
>  
>
Oops... I hate when that happens....

SteveD.


[-- Attachment #2: linux-2.4.21-nfs-accesscache.patch --]
[-- Type: text/plain, Size: 3393 bytes --]

--- linux-2.4.21/fs/nfs/dir.c.orig	2003-08-31 13:04:30.000000000 -0400
+++ linux-2.4.21/fs/nfs/dir.c	2003-08-31 13:07:49.000000000 -0400
@@ -1259,17 +1259,21 @@ nfs_permission(struct inode *inode, int 
 
 	if ((server->flags & NFS_MOUNT_NOACL) || !NFS_PROTO(inode)->access)
 		goto out_notsup;
+	/*
+	 * See if we can use the cache
+	 */
 	cred = rpcauth_lookupcred(NFS_CLIENT(inode)->cl_auth, 0);
 	if (cache->cred == cred
 	    && time_before(jiffies, cache->jiffies + NFS_ATTRTIMEO(inode))) {
-		if (!cache->err) {
-			/* Is the mask a subset of an accepted mask? */
-			if ((cache->mask & mask) == mask)
-				goto out_cached;
-	       	} else {
-			/* ...or is it a superset of a rejected mask? */
-			if ((cache->mask & mask) == cache->mask)
-				goto out_cached;
+		/* Is the mask a subset of an accepted mask? */
+		if ((cache->mask & mask) == mask) {
+			cache->err  = 0;
+			goto out_cached;
+		} 
+		/* ...or is it a superset of a rejected mask? */
+		if ((~cache->mask & mask) == mask) {
+			cache->err  = -EACCES;
+			goto out_cached;
 		}
 	}
 	error = NFS_PROTO(inode)->access(inode, cred, mask);
@@ -1278,8 +1282,6 @@ nfs_permission(struct inode *inode, int 
 		if (cache->cred)
 			put_rpccred(cache->cred);
 		cache->cred = cred;
-		cache->mask = mask;
-		cache->err = error;
 		return error;
 	}
 	put_rpccred(cred);
--- linux-2.4.21/fs/nfs/nfs3proc.c.orig	2003-08-30 16:30:57.000000000 -0400
+++ linux-2.4.21/fs/nfs/nfs3proc.c	2003-08-31 12:58:11.000000000 -0400
@@ -121,10 +121,13 @@ static int
 nfs3_proc_access(struct inode *inode, struct rpc_cred *cred, int mode)
 {
 	struct nfs_fattr	fattr;
+	struct nfs_access_cache *cache = &NFS_I(inode)->cache_access;
 	struct nfs3_accessargs	arg = { NFS_FH(inode), 0 };
 	struct nfs3_accessres	res = { &fattr, 0 };
 	struct rpc_message msg = { NFS3PROC_ACCESS, &arg, &res, cred };
 	int	status;
+	static int may_write, may_exec;
+	__u32 access;
 
 	dprintk("NFS call  access\n");
 	fattr.valid = 0;
@@ -134,20 +137,45 @@ nfs3_proc_access(struct inode *inode, st
 	if (S_ISDIR(inode->i_mode)) {
 		if (mode & MAY_WRITE)
 			arg.access |= NFS3_ACCESS_MODIFY | NFS3_ACCESS_EXTEND | NFS3_ACCESS_DELETE;
+		may_write = NFS3_ACCESS_MODIFY| NFS3_ACCESS_EXTEND | NFS3_ACCESS_DELETE;
+
 		if (mode & MAY_EXEC)
 			arg.access |= NFS3_ACCESS_LOOKUP;
+		may_exec = NFS3_ACCESS_LOOKUP;
+
 	} else {
 		if (mode & MAY_WRITE)
 			arg.access |= NFS3_ACCESS_MODIFY | NFS3_ACCESS_EXTEND;
+		may_write = NFS3_ACCESS_MODIFY | NFS3_ACCESS_EXTEND;
+
 		if (mode & MAY_EXEC)
 			arg.access |= NFS3_ACCESS_EXECUTE;
+		may_exec = NFS3_ACCESS_EXECUTE;
 	}
+	/*
+	 * Save off whats needed and then ask for all of the mode bits
+	 * which will be cached
+	 */
+	access = arg.access;
+	arg.access = (NFS3_ACCESS_EXECUTE|NFS3_ACCESS_READ|NFS3_ACCESS_MODIFY| 
+		NFS3_ACCESS_EXTEND|NFS3_ACCESS_DELETE|NFS3_ACCESS_LOOKUP);
+
 	status = rpc_call_sync(NFS_CLIENT(inode), &msg, 0);
 	nfs_refresh_inode(inode, &fattr);
 	dprintk("NFS reply access\n");
 
-	if (status == 0 && (arg.access & res.access) != arg.access)
+	if (status == 0 && (access & res.access) != access)
 		status = -EACCES;
+
+	if ((!status || status == -EACCES)) {
+		cache->mask = 0;
+		if (res.access & NFS3_ACCESS_READ)
+			cache->mask |= MAY_READ;
+		if (res.access & may_write)
+			cache->mask |= MAY_WRITE;
+		if (res.access & may_exec)
+			cache->mask |= MAY_EXEC;
+	}
 	return status;
 }
 

Re: [PATCH] Access cache forgetting mode bits

[Trond Myklebust]

>>>>> " " == Steve Dickson <SteveD@RedHat.com> writes:

    >>> Thoughts?

What are you using to measure performance here? It would be nice to
see some figures to demonstrate the improvement.

Cheers,
  Trond


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] Access cache forgetting mode bits

[Steve Dickson]

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

Hi Trond,

Trond Myklebust wrote:

>What are you using to measure performance here? It would be nice to
>see some figures to demonstrate the improvement.
>  
>
Good point... If figured the Connectathon test suite (cthon03) would be good
since it has a large variety of tests and easy to run... But if there 
are other tests
that would be good just let me know...

Running cthon03 three times and using nfsstat to count the total number
of rpc and access here are the numbers...

Run 1              total calls  access calls
Original Code:   30230       1755    5%
Andreas's Patch: 30181       1676    5%
Latest Patch:       29671       1141    3%

Run 2              total calls  access calls
Original Code:   30064       1755    5%
Andreas's Patch: 30176       1676    5%
Latest Patch:       29978       1141    3%

Run 3              total calls  access calls
Original Code:   30286      1755    5%
Andreas's Patch: 29978      1676    5%
Latest Patch:       29410      1141    3%

Note: It is not a cut/paste error that each test run had the same
amount of access calls... the only number that really fluctuated
was the number of getattrs.... that's why I only ran the tests three
times... not much seem to be changing...

I've attached the raw data pruned of the v2 stats...

SteveD.




[-- Attachment #2: orig --]
[-- Type: text/plain, Size: 1770 bytes --]

Client rpc stats:
calls      retrans    authrefrsh
30230      15         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1587    5% 1199    3% 2428    8% 1755    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  40% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 407     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    4% 1       0% 0       0% 573     1% 

Client rpc stats:
calls      retrans    authrefrsh
30064      28         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1493    4% 1199    3% 2362    7% 1755    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  40% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 404     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    4% 1       0% 0       0% 570     1% 

Client rpc stats:
calls      retrans    authrefrsh
30286      24         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1520    5% 1199    3% 2554    8% 1755    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  40% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 406     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    4% 1       0% 0       0% 571     1% 


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

Client rpc stats:
calls      retrans    authrefrsh
30181      24         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1478    4% 1199    3% 2567    8% 1676    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3977   13% 12305  40% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 405     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    4% 1       0% 0       0% 573     1% 

Client rpc stats:
calls      retrans    authrefrsh
30176      24         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1574    5% 1199    3% 2477    8% 1676    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  40% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 401     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    4% 1       0% 0       0% 568     1% 

Client rpc stats:
calls      retrans    authrefrsh
29978      8          0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1515    5% 1199    3% 2335    7% 1676    5% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  41% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 403     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    5% 1       0% 0       0% 569     1% 


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

Client rpc stats:
calls      retrans    authrefrsh
29671      9          0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1644    5% 1199    4% 2432    8% 1141    3% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  41% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 403     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    5% 1       0% 0       0% 571     1% 

Client rpc stats:
calls      retrans    authrefrsh
29761      10         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1682    5% 1199    4% 2484    8% 1141    3% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12303  41% 890     2% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 405     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    5% 1       0% 0       0% 570     1% 

Client rpc stats:
calls      retrans    authrefrsh
29410      23         0       
Client nfs v3:
null       getattr    setattr    lookup     access     readlink   
0       0% 1492    5% 1199    4% 2318    7% 1141    3% 250     0% 
read       write      create     mkdir      symlink    mknod      
3976   13% 12304  41% 890     3% 175     0% 250     0% 0       0% 
remove     rmdir      rename     link       readdir    readdirplus
1389    4% 175     0% 403     1% 250     0% 0       0% 1121    3% 
fsstat     fsinfo     pathconf   commit     
1500    5% 1       0% 0       0% 576     1%