[PATCH] NFSv3: cached permissions subset enhancement

[PATCH] NFSv3: cached permissions subset enhancement

[Joe Korty]

[NFSv3] cached permissions subset enhancement.

NFSv3 allows file permissions to be cached on the client
side.  The Linux client, when fetching these permissions,
makes the request for all permissions (rwx) in a single
operation.  However, for some NFSv3 server implementations
(the only one currently known is PowerMAX OS), an incoming
request for all rwx permissions in a single request,
when all are not actually set in the file, returns an
NFSERR_ACCES failure code to the client, rather than
the subset of permissions actually available for that file.

This patch modifies the Linux client side code to
individually fetch the r, w, and x permissions (combining
these for storage into the cache), if the original
single-request method fails.

This slower method will not affect performance of those
client/server pairs for which the original single-request
method works.  In particular there is no performance
penalty for linux/linux NFSv3 connections.

Author: Linda Dunaphant <ldunaphant@comcast.net>
Signed-off-by: Joe Korty <joe.korty@ccur.com>

Index: 2.6.27-rc4-git4/fs/nfs/dir.c
===================================================================
--- 2.6.27-rc4-git4.orig/fs/nfs/dir.c	2008-08-26 17:44:34.000000000 -0400
+++ 2.6.27-rc4-git4/fs/nfs/dir.c	2008-08-26 18:21:27.000000000 -0400
@@ -1870,6 +1870,8 @@
 {
 	struct nfs_access_entry cache;
 	int status;
+	int i;
+	int cmask = 0;
 
 	status = nfs_access_get_cached(inode, cred, &cache);
 	if (status == 0)
@@ -1880,8 +1882,27 @@
 	cache.cred = cred;
 	cache.jiffies = jiffies;
 	status = NFS_PROTO(inode)->access(inode, &cache);
-	if (status != 0)
+	if (status == -NFSERR_ACCES) {
+		//
+		// Try again - one mode at a time & combine at the end
+		//
+		for (i = 0; i <= MAY_READ; i++) {
+			cache.mask = 1 << i;
+			status = NFS_PROTO(inode)->access(inode, &cache);
+			if (status == 0)
+				cmask |= cache.mask;
+			else if (status != -NFSERR_ACCES) {
+				return status;
+			}
+		}
+
+		if (!cmask)
+			return -NFSERR_ACCES;
+
+		cache.mask = cmask;
+	} else if (status)
 		return status;
+
 	nfs_access_add_cache(inode, &cache);
 out:
 	if ((mask & ~cache.mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)

Re: [PATCH] NFSv3: cached permissions subset enhancement

[Myklebust, Trond]

On Aug 27, 2008, at 7:28, "Joe Korty" <joe.korty@ccur.com> wrote:

> [NFSv3] cached permissions subset enhancement.
>
> NFSv3 allows file permissions to be cached on the client
> side.  The Linux client, when fetching these permissions,
> makes the request for all permissions (rwx) in a single
> operation.  However, for some NFSv3 server implementations
> (the only one currently known is PowerMAX OS), an incoming
> request for all rwx permissions in a single request,
> when all are not actually set in the file, returns an
> NFSERR_ACCES failure code to the client, rather than
> the subset of permissions actually available for that file.
>
> This patch modifies the Linux client side code to
> individually fetch the r, w, and x permissions (combining
> these for storage into the cache), if the original
> single-request method fails.
>
> This slower method will not affect performance of those
> client/server pairs for which the original single-request
> method works.  In particular there is no performance
> penalty for linux/linux NFSv3 connections.
>
> Author: Linda Dunaphant <ldunaphant@comcast.net>
> Signed-off-by: Joe Korty <joe.korty@ccur.com>
>
> Index: 2.6.27-rc4-git4/fs/nfs/dir.c
> ===================================================================
> --- 2.6.27-rc4-git4.orig/fs/nfs/dir.c    2008-08-26 17:44:34.000000000 -0400
> +++ 2.6.27-rc4-git4/fs/nfs/dir.c    2008-08-26 18:21:27.000000000  
> -0400
> @@ -1870,6 +1870,8 @@
> {
>    struct nfs_access_entry cache;
>    int status;
> +    int i;
> +    int cmask = 0;
>
>    status = nfs_access_get_cached(inode, cred, &cache);
>    if (status == 0)
> @@ -1880,8 +1882,27 @@
>    cache.cred = cred;
>    cache.jiffies = jiffies;
>    status = NFS_PROTO(inode)->access(inode, &cache);
> -    if (status != 0)
> +    if (status == -NFSERR_ACCES) {
> +        //
> +        // Try again - one mode at a time & combine at the end
> +        //
> +        for (i = 0; i <= MAY_READ; i++) {
> +            cache.mask = 1 << i;
> +            status = NFS_PROTO(inode)->access(inode, &cache);
> +            if (status == 0)
> +                cmask |= cache.mask;
> +            else if (status != -NFSERR_ACCES) {
> +                return status;
> +            }
> +        }
> +
> +        if (!cmask)
> +            return -NFSERR_ACCES;
> +
> +        cache.mask = cmask;
> +    } else if (status)
>        return status;
> +
>    nfs_access_add_cache(inode, &cache);
> out:
>    if ((mask & ~cache.mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)

This isn't a good solution. The correct thing to do here is to resend  
the request for just those permissions that the VFS requested.

Cheers
   Trond

[PATCH] NFSv3: cached permissions subset enhancement, v2

[Joe Korty]

On Thu, Aug 28, 2008 at 12:03:47PM -0400, Myklebust, Trond wrote:

> This isn't a good solution. The correct thing to do here is to resend
> the request for just those permissions that the VFS requested.

Is there any chance that the cached permissions are shared amoung
multiple open(2)'s of the same file?  If sharing happens then we
really need to cache all of MAY_EXEC | MAY_WRITE | MAY_READ, rather
than just that subset asked for by the open(2) that happens to
be the one filling in the cache.

In any case, I've rewritten the patch per the above suggestion, and
in doing so noted an embarrasing bug in the original posting, one that
happened to touch all the values that it needed to, by accident:

-        for (i = 0; i <= MAY_READ; i++) {
-            cache.mask = 1 << i;
+	 for (i = 1; i <= MAY_READ; i *= 2) {
+            cache.mask = i;

     ---------- cut here ---------

[NFSv3] cached permissions subset enhancement.

NFSv3 allows file permissions to be cached on the client
side.  The Linux client, when fetching these permissions,
makes the request for all permissions (rwx) in a single
operation.  However, for some NFSv3 server implementations
(the only one currently known is PowerMAX OS), an incoming
request for all rwx permissions in a single request,
when all are not actually set in the file, returns an
NFSERR_ACCES failure code to the client, rather than
the subset of permissions actually available for that file.

This patch modifies the Linux client side code to
individually fetch the r, w, and x permissions (combining
these for storage into the cache), if the original
single-request method fails.

This slower method will not affect performance of those
client/server pairs for which the original single-request
method works.  In particular there is no performance
penalty for linux/linux NFSv3 connections.

Author: Linda Dunaphant <ldunaphant@comcast.net>
Reworked-by: Joe Korty <joe.korty@ccur.com>
Signed-off-by: Joe Korty <joe.korty@ccur.com>

Index: a/fs/nfs/dir.c
===================================================================
--- a.orig/fs/nfs/dir.c	2008-08-27 11:31:43.000000000 -0400
+++ a/fs/nfs/dir.c	2008-08-28 14:12:52.000000000 -0400
@@ -1902,8 +1902,34 @@
 	cache.cred = cred;
 	cache.jiffies = jiffies;
 	status = NFS_PROTO(inode)->access(inode, &cache);
-	if (status != 0)
+	if (status == -NFSERR_ACCES) {
+		/*
+		 * Try again - one mode at a time & combine at the end
+		 */
+		int cmask = 0;
+
+		if (mask & MAY_EXEC) {
+			cache.mask = MAY_EXEC;
+			if (!NFS_PROTO(inode)->access(inode, &cache))
+				cmask |= cache.mask;
+		}
+		if (mask & MAY_WRITE) {
+			cache.mask = MAY_WRITE;
+			if (!NFS_PROTO(inode)->access(inode, &cache))
+				cmask |= cache.mask;
+		}
+		if (mask & MAY_READ) {
+			cache.mask = MAY_READ;
+			if (!NFS_PROTO(inode)->access(inode, &cache))
+				cmask |= cache.mask;
+		}
+		if (!cmask)
+			return -NFSERR_ACCES;
+
+		cache.mask = cmask;
+	} else if (status)
 		return status;
+
 	nfs_access_add_cache(inode, &cache);
 out:
 	if ((cache.mask & mask) == mask)

Re: [PATCH] NFSv3: cached permissions subset enhancement, v2

[Joe Korty]

On Thu, Aug 28, 2008 at 02:28:48PM -0400, Joe Korty wrote:
> On Thu, Aug 28, 2008 at 12:03:47PM -0400, Myklebust, Trond wrote:
>> This isn't a good solution. The correct thing to do here is to resend
>> the request for just those permissions that the VFS requested.
> 
> Is there any chance that the cached permissions are shared amoung
> multiple open(2)'s of the same file?  If sharing happens then we
> really need to cache all of MAY_EXEC | MAY_WRITE | MAY_READ, rather
> than just that subset asked for by the open(2) that happens to
> be the one filling in the cache.
> 
> In any case, I've rewritten the patch per the above suggestion, and


Hi Trond,
I've tested the new patch with the test sequence mentioned in the
original customer bug report, and it seems to work.

Joe

Re: [PATCH] NFSv3: cached permissions subset enhancement, v2

[Peter Staubach]

Joe Korty wrote:
> On Thu, Aug 28, 2008 at 02:28:48PM -0400, Joe Korty wrote:
>   
>> On Thu, Aug 28, 2008 at 12:03:47PM -0400, Myklebust, Trond wrote:
>>     
>>> This isn't a good solution. The correct thing to do here is to resend
>>> the request for just those permissions that the VFS requested.
>>>       
>> Is there any chance that the cached permissions are shared amoung
>> multiple open(2)'s of the same file?  If sharing happens then we
>> really need to cache all of MAY_EXEC | MAY_WRITE | MAY_READ, rather
>> than just that subset asked for by the open(2) that happens to
>> be the one filling in the cache.
>>
>> In any case, I've rewritten the patch per the above suggestion, and
>>     
>
>
> Hi Trond,
> I've tested the new patch with the test sequence mentioned in the
> original customer bug report, and it seems to work.

It would also be good to pursue getting that NFS server fixed.
It is not protocol compliant.  That's not the way that the
ACCESS operation is specified to work.  The server explicitly
should not return NFS3ERR_ACCESS when access is denied.  The
status portion of the return is supposed to indicate the success
or failure of the ACCESS operation itself, not the access
rights that a process may have w.r.t. a file.  The bitmask is
to be used for that.

       ps