[PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[Chuck Lever]

Some well-known NFSv3 clients drop their directory entry caches when
they receive replies with no WCC data.  Without this data, they
employ extra READ, LOOKUP, and GETATTR requests to ensure their
directory entry caches are up to date, causing performance to suffer
needlessly.

In order to return WCC data, our server has to have both the pre-op
and the post-op attribute data on hand when a reply is XDR encoded.
The pre-op data is filled in when the incoming fh is locked, and the
post-op data is filled in when the fh is unlocked.

Unfortunately, for REMOVE, RMDIR, MKNOD, and MKDIR, the directory fh
is not unlocked until well after the reply has been XDR encoded.  This
means that encode_wcc_data() does not have wcc_data for the parent
directory, so none is returned to the client after these operations
complete.

By unlocking the parent directory fh immediately after the internal
operations for each NFS procedure is complete, the post-op data is
filled in before XDR encoding starts, so it can be returned to the
client properly.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

Bruce-

This patch seems to go a long way towards fixing the pre-wcc data
problem and related performance issues.  Perhaps this patch is also
appropriate for stable releases, though I haven't tested it with any.


 fs/nfsd/nfs3proc.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 3d68f45..9ae9331 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -271,7 +271,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp, struct nfsd3_createargs *argp,
 	fh_init(&resp->fh, NFS3_FHSIZE);
 	nfserr = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
 				    &argp->attrs, S_IFDIR, 0, &resp->fh);
-
+	fh_unlock(&resp->dirfh);
 	RETURN_STATUS(nfserr);
 }
 
@@ -327,7 +327,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp, struct nfsd3_mknodargs *argp,
 	type = nfs3_ftypes[argp->ftype];
 	nfserr = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
 				    &argp->attrs, type, rdev, &resp->fh);
-
+	fh_unlock(&resp->dirfh);
 	RETURN_STATUS(nfserr);
 }
 
@@ -348,6 +348,7 @@ nfsd3_proc_remove(struct svc_rqst *rqstp, struct nfsd3_diropargs *argp,
 	/* Unlink. -S_IFDIR means file must not be a directory */
 	fh_copy(&resp->fh, &argp->fh);
 	nfserr = nfsd_unlink(rqstp, &resp->fh, -S_IFDIR, argp->name, argp->len);
+	fh_unlock(&resp->fh);
 	RETURN_STATUS(nfserr);
 }
 
@@ -367,6 +368,7 @@ nfsd3_proc_rmdir(struct svc_rqst *rqstp, struct nfsd3_diropargs *argp,
 
 	fh_copy(&resp->fh, &argp->fh);
 	nfserr = nfsd_unlink(rqstp, &resp->fh, S_IFDIR, argp->name, argp->len);
+	fh_unlock(&resp->fh);
 	RETURN_STATUS(nfserr);
 }
 

Re: [PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[J. Bruce Fields]

On Tue, Jul 06, 2010 at 04:53:34PM -0400, Chuck Lever wrote:
> Some well-known NFSv3 clients drop their directory entry caches when
> they receive replies with no WCC data.

Can we include any more details?  (And/or a simple test case that
demonstrates the difference?)

> Without this data, they
> employ extra READ, LOOKUP, and GETATTR requests to ensure their
> directory entry caches are up to date, causing performance to suffer
> needlessly.
> 
> In order to return WCC data, our server has to have both the pre-op
> and the post-op attribute data on hand when a reply is XDR encoded.
> The pre-op data is filled in when the incoming fh is locked, and the
> post-op data is filled in when the fh is unlocked.
> 
> Unfortunately, for REMOVE, RMDIR, MKNOD, and MKDIR, the directory fh
> is not unlocked until well after the reply has been XDR encoded.

So it wasn't happening until an fh_put() was done in .pc_release()?

> This
> means that encode_wcc_data() does not have wcc_data for the parent
> directory, so none is returned to the client after these operations
> complete.
> 
> By unlocking the parent directory fh immediately after the internal
> operations for each NFS procedure is complete, the post-op data is
> filled in before XDR encoding starts, so it can be returned to the
> client properly.

Looks good to me, thanks, applying for 2.6.36.

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> Bruce-
> 
> This patch seems to go a long way towards fixing the pre-wcc data
> problem and related performance issues.  Perhaps this patch is also
> appropriate for stable releases, though I haven't tested it with any.

If it's a longstanding problem, I'm inclined to leave it be on the
grounds that people sticking with a stable release are resigned to the
performance they've always had....

--b.

> 
> 
>  fs/nfsd/nfs3proc.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 3d68f45..9ae9331 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -271,7 +271,7 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp, struct nfsd3_createargs *argp,
>  	fh_init(&resp->fh, NFS3_FHSIZE);
>  	nfserr = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
>  				    &argp->attrs, S_IFDIR, 0, &resp->fh);
> -
> +	fh_unlock(&resp->dirfh);
>  	RETURN_STATUS(nfserr);
>  }
>  
> @@ -327,7 +327,7 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp, struct nfsd3_mknodargs *argp,
>  	type = nfs3_ftypes[argp->ftype];
>  	nfserr = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
>  				    &argp->attrs, type, rdev, &resp->fh);
> -
> +	fh_unlock(&resp->dirfh);
>  	RETURN_STATUS(nfserr);
>  }
>  
> @@ -348,6 +348,7 @@ nfsd3_proc_remove(struct svc_rqst *rqstp, struct nfsd3_diropargs *argp,
>  	/* Unlink. -S_IFDIR means file must not be a directory */
>  	fh_copy(&resp->fh, &argp->fh);
>  	nfserr = nfsd_unlink(rqstp, &resp->fh, -S_IFDIR, argp->name, argp->len);
> +	fh_unlock(&resp->fh);
>  	RETURN_STATUS(nfserr);
>  }
>  
> @@ -367,6 +368,7 @@ nfsd3_proc_rmdir(struct svc_rqst *rqstp, struct nfsd3_diropargs *argp,
>  
>  	fh_copy(&resp->fh, &argp->fh);
>  	nfserr = nfsd_unlink(rqstp, &resp->fh, S_IFDIR, argp->name, argp->len);
> +	fh_unlock(&resp->fh);
>  	RETURN_STATUS(nfserr);
>  }
>  
> 

Re: [PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[Chuck Lever]

On 07/ 7/10 05:10 PM, J. Bruce Fields wrote:
> On Tue, Jul 06, 2010 at 04:53:34PM -0400, Chuck Lever wrote:
>> Some well-known NFSv3 clients drop their directory entry caches when
>> they receive replies with no WCC data.
>
> Can we include any more details?  (And/or a simple test case that
> demonstrates the difference?)

I have a version of this patch with a much more comprehensive 
description which I can post later today.

As for a test case... among other things, I untarred a large tarball on 
a Mac OS 10.6 NFS client, and, once the server was fixed, it went as 
quickly as you would expect.  Before, it took much longer.

>> Without this data, they
>> employ extra READ, LOOKUP, and GETATTR requests to ensure their
>> directory entry caches are up to date, causing performance to suffer
>> needlessly.
>>
>> In order to return WCC data, our server has to have both the pre-op
>> and the post-op attribute data on hand when a reply is XDR encoded.
>> The pre-op data is filled in when the incoming fh is locked, and the
>> post-op data is filled in when the fh is unlocked.
>>
>> Unfortunately, for REMOVE, RMDIR, MKNOD, and MKDIR, the directory fh
>> is not unlocked until well after the reply has been XDR encoded.
>
> So it wasn't happening until an fh_put() was done in .pc_release()?

Correct.

Re: [PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[J. Bruce Fields]

On Thu, Jul 08, 2010 at 11:02:12AM -0400, Chuck Lever wrote:
> On 07/ 7/10 05:10 PM, J. Bruce Fields wrote:
> >On Tue, Jul 06, 2010 at 04:53:34PM -0400, Chuck Lever wrote:
> >>Some well-known NFSv3 clients drop their directory entry caches when
> >>they receive replies with no WCC data.
> >
> >Can we include any more details?  (And/or a simple test case that
> >demonstrates the difference?)
> 
> I have a version of this patch with a much more comprehensive
> description which I can post later today.

Thanks, that would be interesting.

(Though note the version you posted is already applied for 2.6.36.)

--b.

> As for a test case... among other things, I untarred a large tarball
> on a Mac OS 10.6 NFS client, and, once the server was fixed, it went
> as quickly as you would expect.  Before, it took much longer.
> 
> >>Without this data, they
> >>employ extra READ, LOOKUP, and GETATTR requests to ensure their
> >>directory entry caches are up to date, causing performance to suffer
> >>needlessly.
> >>
> >>In order to return WCC data, our server has to have both the pre-op
> >>and the post-op attribute data on hand when a reply is XDR encoded.
> >>The pre-op data is filled in when the incoming fh is locked, and the
> >>post-op data is filled in when the fh is unlocked.
> >>
> >>Unfortunately, for REMOVE, RMDIR, MKNOD, and MKDIR, the directory fh
> >>is not unlocked until well after the reply has been XDR encoded.
> >
> >So it wasn't happening until an fh_put() was done in .pc_release()?
> 
> Correct.

Re: [PATCH] NFSD: Fill in WCC data for REMOVE, RMDIR, MKNOD, and MKDIR

[J. Bruce Fields]

On Thu, Jul 08, 2010 at 11:02:12AM -0400, Chuck Lever wrote:
> On 07/ 7/10 05:10 PM, J. Bruce Fields wrote:
> >On Tue, Jul 06, 2010 at 04:53:34PM -0400, Chuck Lever wrote:
> >>Some well-known NFSv3 clients drop their directory entry caches when
> >>they receive replies with no WCC data.
> >
> >Can we include any more details?  (And/or a simple test case that
> >demonstrates the difference?)
> 
> I have a version of this patch with a much more comprehensive
> description which I can post later today.
> 
> As for a test case... among other things, I untarred a large tarball
> on a Mac OS 10.6 NFS client, and, once the server was fixed, it went
> as quickly as you would expect.  Before, it took much longer.

By the way, why isn't this reproduceable on the Linux client?

If it's ignoring whether updates are atomic--is that really OK?

--b.