Re: Poor NFSv4 first impressions

Re: Poor NFSv4 first impressions

[Daniel J Blueman]

Jeff Garzik wrote:
> Being a big user of NFS at home, and a big fan of NFSv4, it was high
> time that I converted my home network from NFSv3 to NFSv4.
>
> Unfortunately applications started breaking left and right.  vim
> noticeably malfunctioned, trying repeatedly to create a swapfile (sorta
> like a lockfile).  Mozilla Thunderbird would crash reproducibly whenever
> it tried anything remotely major with a mailbox, such as compressing
> folders (removing deleted messages).
[snip]

This has all the symptoms to an open EACCES NFSv4 bug in 2.6.18/19.
This is fixed in:

http://www.citi.umich.edu/projects/nfsv4/linux/kernel-patches/2.6.19-rc3-2/linux-2.6.19-rc3-CITI_NFS4_ALL-2.diff
(see http://www.citi.umich.edu/projects/nfsv4/linux/).

With this patch, I can run just great with NFSv4 home dir (etc)
mounts; without, I get the symptom of many 0-byte temporary/lock files
being created and often the inability to create files (!). Be sure to
allow callback delegation connections in through your firewall for the
extra performance ;-) .

Maybe it's too late for these fixes 2.6.19, but they should certainly
make 2.6.19.1 IMHO.
-- 
Daniel J Blueman

Re: Poor NFSv4 first impressions

[Bill Davidsen]

Daniel J Blueman wrote:
> Jeff Garzik wrote:
>> Being a big user of NFS at home, and a big fan of NFSv4, it was high
>> time that I converted my home network from NFSv3 to NFSv4.
>>
>> Unfortunately applications started breaking left and right.  vim
>> noticeably malfunctioned, trying repeatedly to create a swapfile (sorta
>> like a lockfile).  Mozilla Thunderbird would crash reproducibly whenever
>> it tried anything remotely major with a mailbox, such as compressing
>> folders (removing deleted messages).
> [snip]
> 
> This has all the symptoms to an open EACCES NFSv4 bug in 2.6.18/19.
> This is fixed in:
> 
> http://www.citi.umich.edu/projects/nfsv4/linux/kernel-patches/2.6.19-rc3-2/linux-2.6.19-rc3-CITI_NFS4_ALL-2.diff 
> 
> (see http://www.citi.umich.edu/projects/nfsv4/linux/).
> 
> With this patch, I can run just great with NFSv4 home dir (etc)
> mounts; without, I get the symptom of many 0-byte temporary/lock files
> being created and often the inability to create files (!). Be sure to
> allow callback delegation connections in through your firewall for the
> extra performance ;-) .
> 
> Maybe it's too late for these fixes 2.6.19, but they should certainly
> make 2.6.19.1 IMHO.

If NFSv4 really works that poorly without the patches, perhaps they 
should go in 2.6.19 at the start. I'm surprised others aren't having 
this problem, I thought there was more test use.


-- 
Bill Davidsen <davidsen@tmr.com>
   Obscure bug of 2004: BASH BUFFER OVERFLOW - if bash is being run by a
normal user and is setuid root, with the "vi" line edit mode selected,
and the character set is "big5," an off-by-one errors occurs during
wildcard (glob) expansion.

Re: Poor NFSv4 first impressions

[J. Bruce Fields]

On Mon, Nov 06, 2006 at 12:03:54PM +0000, Daniel J Blueman wrote:
> This has all the symptoms to an open EACCES NFSv4 bug in 2.6.18/19.
> This is fixed in:
> 
> http://www.citi.umich.edu/projects/nfsv4/linux/kernel-patches/2.6.19-rc3-2/linux-2.6.19-rc3-CITI_NFS4_ALL-2.diff
> (see http://www.citi.umich.edu/projects/nfsv4/linux/).
> 
> With this patch, I can run just great with NFSv4 home dir (etc)
> mounts; without, I get the symptom of many 0-byte temporary/lock files
> being created and often the inability to create files (!). Be sure to
> allow callback delegation connections in through your firewall for the
> extra performance ;-) .
> 
> Maybe it's too late for these fixes 2.6.19, but they should certainly
> make 2.6.19.1 IMHO.

Yeah, bad patch management on my part, apologies, I should have pushed
it as soon as I noticed the problem.

Investigating the problem revealed some ugliness (and some races which
will need further work), and I had hoped to have a more complete fix
before now.  Oh well.

Two patches follow; the first does a very simple cleanup, the second
solves the immediate problem in the most straightforward way I can see,
but is a bit of a hack.

--b.

[PATCH 0/1] nfsd4: reindent do_open_lookup()

[J. Bruce Fields]

Minor rearrangement, cleanup of do_open_lookup().  No change in behavior.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4proc.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0a7bbdc..4a73f5b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -106,27 +106,25 @@ do_open_lookup(struct svc_rqst *rqstp, s
 					open->op_fname.len, &open->op_iattr,
 					&resfh, open->op_createmode,
 					(u32 *)open->op_verf.data, &open->op_truncate);
-	}
-	else {
+	} else {
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname.data, open->op_fname.len, &resfh);
 		fh_unlock(current_fh);
 	}
+	if (status)
+		goto out;
 
-	if (!status) {
-		set_change_info(&open->op_cinfo, current_fh);
+	set_change_info(&open->op_cinfo, current_fh);
 
-		/* set reply cache */
-		fh_dup2(current_fh, &resfh);
-		open->op_stateowner->so_replay.rp_openfh_len =
-			resfh.fh_handle.fh_size;
-		memcpy(open->op_stateowner->so_replay.rp_openfh,
-				&resfh.fh_handle.fh_base,
-				resfh.fh_handle.fh_size);
+	/* set reply cache */
+	fh_dup2(current_fh, &resfh);
+	open->op_stateowner->so_replay.rp_openfh_len = resfh.fh_handle.fh_size;
+	memcpy(open->op_stateowner->so_replay.rp_openfh,
+			&resfh.fh_handle.fh_base, resfh.fh_handle.fh_size);
 
-		status = do_open_permission(rqstp, current_fh, open, MAY_NOP);
-	}
+	status = do_open_permission(rqstp, current_fh, open, MAY_NOP);
 
+out:
 	fh_put(&resfh);
 	return status;
 }
-- 
1.4.3.3.g01929

Re: [PATCH 0/1] nfsd4: reindent do_open_lookup()

[J. Bruce Fields]

Uh, [PATCH 0/1] should be [PATCH 1/2].  Some days I can count, other
days....

--b.

[PATCH 2/2] nfsd4: fix open-create permissions

[J. Bruce Fields]

In the case where an open creates the file, we shouldn't be rechecking
permissions to open the file; the open succeeds regardless of what the
new file's mode bits say.

This patch fixes the problem, but only by introducing yet another parameter
to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs3proc.c        |    2 +-
 fs/nfsd/nfs4proc.c        |    6 ++++--
 fs/nfsd/vfs.c             |    4 +++-
 include/linux/nfsd/nfsd.h |    2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 64db601..7f5bad0 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -258,7 +258,7 @@ nfsd3_proc_create(struct svc_rqst *rqstp
 	/* Now create the file and set attributes */
 	nfserr = nfsd_create_v3(rqstp, dirfhp, argp->name, argp->len,
 				attr, newfhp,
-				argp->createmode, argp->verf, NULL);
+				argp->createmode, argp->verf, NULL, NULL);
 
 	RETURN_STATUS(nfserr);
 }
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4a73f5b..50bc942 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -93,6 +93,7 @@ do_open_lookup(struct svc_rqst *rqstp, s
 {
 	struct svc_fh resfh;
 	__be32 status;
+	int created = 0;
 
 	fh_init(&resfh, NFS4_FHSIZE);
 	open->op_truncate = 0;
@@ -105,7 +106,7 @@ do_open_lookup(struct svc_rqst *rqstp, s
 		status = nfsd_create_v3(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
 					&resfh, open->op_createmode,
-					(u32 *)open->op_verf.data, &open->op_truncate);
+					(u32 *)open->op_verf.data, &open->op_truncate, &created);
 	} else {
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname.data, open->op_fname.len, &resfh);
@@ -122,7 +123,8 @@ do_open_lookup(struct svc_rqst *rqstp, s
 	memcpy(open->op_stateowner->so_replay.rp_openfh,
 			&resfh.fh_handle.fh_base, resfh.fh_handle.fh_size);
 
-	status = do_open_permission(rqstp, current_fh, open, MAY_NOP);
+	if (!created)
+		status = do_open_permission(rqstp, current_fh, open, MAY_NOP);
 
 out:
 	fh_put(&resfh);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f21e917..1a7ad8c 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1237,7 +1237,7 @@ __be32
 nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		char *fname, int flen, struct iattr *iap,
 		struct svc_fh *resfhp, int createmode, u32 *verifier,
-	        int *truncp)
+	        int *truncp, int *created)
 {
 	struct dentry	*dentry, *dchild = NULL;
 	struct inode	*dirp;
@@ -1331,6 +1331,8 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
 	if (host_err < 0)
 		goto out_nfserr;
+	if (created)
+		*created = 1;
 
 	if (EX_ISSYNC(fhp->fh_export)) {
 		err = nfserrno(nfsd_sync_dir(dentry));
diff --git a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h
index eb23114..edb54c3 100644
--- a/include/linux/nfsd/nfsd.h
+++ b/include/linux/nfsd/nfsd.h
@@ -89,7 +89,7 @@ __be32		nfsd_access(struct svc_rqst *, s
 __be32		nfsd_create_v3(struct svc_rqst *, struct svc_fh *,
 				char *name, int len, struct iattr *attrs,
 				struct svc_fh *res, int createmode,
-				u32 *verifier, int *truncp);
+				u32 *verifier, int *truncp, int *created);
 __be32		nfsd_commit(struct svc_rqst *, struct svc_fh *,
 				loff_t, unsigned long);
 #endif /* CONFIG_NFSD_V3 */
-- 
1.4.3.3.g01929

Fwd: [PATCH 2/2] nfsd4: fix open-create permissions

[Daniel J Blueman]

Linus, Trond,

What is the chance of this patch making it into the final 2.6.19?

WIthout it, there is a serious NFSv4 open() regression; I've been
running it on client and server for ~1 week under load and it resolves
the condition w/o side-effects. See the LKML thread "Poor NFSv4 first
impressions" for further details.

Daniel

---------- Forwarded message ----------
From: J. Bruce Fields <bfields@fieldses.org>
Date: 06-Nov-2006 16:24
Subject: [PATCH 2/2] nfsd4: fix open-create permissions
To: Daniel J Blueman <daniel.blueman@gmail.com>
Cc: nfsv4@linux-nfs.org, Linux Kernel <linux-kernel@vger.kernel.org>,
Jeff Garzik <jeff@garzik.org>, Neil Brown <neilb@suse.de>


In the case where an open creates the file, we shouldn't be rechecking
permissions to open the file; the open succeeds regardless of what the
new file's mode bits say.

This patch fixes the problem, but only by introducing yet another parameter
to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs3proc.c        |    2 +-
 fs/nfsd/nfs4proc.c        |    6 ++++--
 fs/nfsd/vfs.c             |    4 +++-
 include/linux/nfsd/nfsd.h |    2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 64db601..7f5bad0 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -258,7 +258,7 @@ nfsd3_proc_create(struct svc_rqst *rqstp
        /* Now create the file and set attributes */
        nfserr = nfsd_create_v3(rqstp, dirfhp, argp->name, argp->len,
                                attr, newfhp,
-                               argp->createmode, argp->verf, NULL);
+                               argp->createmode, argp->verf, NULL, NULL);

        RETURN_STATUS(nfserr);
 }
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 4a73f5b..50bc942 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -93,6 +93,7 @@ do_open_lookup(struct svc_rqst *rqstp, s
 {
        struct svc_fh resfh;
        __be32 status;
+       int created = 0;

        fh_init(&resfh, NFS4_FHSIZE);
        open->op_truncate = 0;
@@ -105,7 +106,7 @@ do_open_lookup(struct svc_rqst *rqstp, s
                status = nfsd_create_v3(rqstp, current_fh, open->op_fname.data,
                                        open->op_fname.len, &open->op_iattr,
                                        &resfh, open->op_createmode,
-                                       (u32 *)open->op_verf.data,
&open->op_truncate);
+                                       (u32 *)open->op_verf.data,
&open->op_truncate, &created);
        } else {
                status = nfsd_lookup(rqstp, current_fh,
                                     open->op_fname.data,
open->op_fname.len, &resfh);
@@ -122,7 +123,8 @@ do_open_lookup(struct svc_rqst *rqstp, s
        memcpy(open->op_stateowner->so_replay.rp_openfh,
                        &resfh.fh_handle.fh_base, resfh.fh_handle.fh_size);

-       status = do_open_permission(rqstp, current_fh, open, MAY_NOP);
+       if (!created)
+               status = do_open_permission(rqstp, current_fh, open, MAY_NOP);

 out:
        fh_put(&resfh);
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f21e917..1a7ad8c 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1237,7 +1237,7 @@ __be32
 nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
                char *fname, int flen, struct iattr *iap,
                struct svc_fh *resfhp, int createmode, u32 *verifier,
-               int *truncp)
+               int *truncp, int *created)
 {
        struct dentry   *dentry, *dchild = NULL;
        struct inode    *dirp;
@@ -1331,6 +1331,8 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
        host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
        if (host_err < 0)
                goto out_nfserr;
+       if (created)
+               *created = 1;

        if (EX_ISSYNC(fhp->fh_export)) {
                err = nfserrno(nfsd_sync_dir(dentry));
diff --git a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h
index eb23114..edb54c3 100644
--- a/include/linux/nfsd/nfsd.h
+++ b/include/linux/nfsd/nfsd.h
@@ -89,7 +89,7 @@ __be32                nfsd_access(struct svc_rqst *, s
 __be32         nfsd_create_v3(struct svc_rqst *, struct svc_fh *,
                                char *name, int len, struct iattr *attrs,
                                struct svc_fh *res, int createmode,
-                               u32 *verifier, int *truncp);
+                               u32 *verifier, int *truncp, int *created);
 __be32         nfsd_commit(struct svc_rqst *, struct svc_fh *,
                                loff_t, unsigned long);
 #endif /* CONFIG_NFSD_V3 */
--
1.4.3.3.g01929

-- 
Daniel J Blueman

Re: Fwd: [PATCH 2/2] nfsd4: fix open-create permissions

[Jeff Garzik]

Daniel J Blueman wrote:
> Linus, Trond,
> 
> What is the chance of this patch making it into the final 2.6.19?
> 
> WIthout it, there is a serious NFSv4 open() regression; I've been
> running it on client and server for ~1 week under load and it resolves
> the condition w/o side-effects. See the LKML thread "Poor NFSv4 first
> impressions" for further details.

strong ACK, provided that someone who knows the NFSv4 server code well 
(Neil B?) gives it an ACK.

	Jeff

Re: Fwd: [PATCH 2/2] nfsd4: fix open-create permissions

[Andrew Morton]

On Wed, 08 Nov 2006 08:33:50 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Daniel J Blueman wrote:
> > Linus, Trond,
> > 
> > What is the chance of this patch making it into the final 2.6.19?
> > 
> > WIthout it, there is a serious NFSv4 open() regression; I've been
> > running it on client and server for ~1 week under load and it resolves
> > the condition w/o side-effects. See the LKML thread "Poor NFSv4 first
> > impressions" for further details.
> 
> strong ACK, provided that someone who knows the NFSv4 server code well 
> (Neil B?) gives it an ACK.
> 

Neil has acked it.  This is in my for-2.6.19 queue, probably later today.

Re: Poor NFSv4 first impressions

[Pavel Machek]

> With this patch, I can run just great with NFSv4 home dir (etc)
> mounts; without, I get the symptom of many 0-byte temporary/lock files
> being created and often the inability to create files (!). Be sure to
> allow callback delegation connections in through your firewall for the
> extra performance ;-) .
> 
> Maybe it's too late for these fixes 2.6.19, but they should certainly
> make 2.6.19.1 IMHO.

If they are good enough for 2.6.19.1, they should _definitely_ go into
2.6.19.


								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html