nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correcttly

PATCH In review Mi Jinlong 2 participants 6 messages Apr 6, 2011 – Apr 20, 2011

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Mi Jinlong

author

2011-04-06 09:05 UTC #0 lore raw

PATCH 1/1 nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correcttly

At the recent kernel(2.6.39-rc1), NFS server can't process OPEN with EXCLUSIVE4_1, because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.

According to RFC5661, at nfsd_create_v3, EXCLUSIVE4_1 should be processed as EXCLUSIVE4.

Signed-off-by Mi Jinlong <[email protected]>

---
 fs/nfsd/nfs4proc.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

fs/nfsd/nfs4proc.c +8 -3

index 5fcb139..5325490 100644
@@ -173,7 +173,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 {
 	struct svc_fh resfh;
 	__be32 status;
-	int created = 0;
+	int created = 0, createmode = 0;
 
 	fh_init(&resfh, NFS4_FHSIZE);
 	open->op_truncate = 0;

@@ -196,11 +196,16 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 
 		/*
 		 * Note: create modes (UNCHECKED,GUARDED...) are the same
-		 * in NFSv4 as in v3.
+		 * in NFSv4 as in v3 except EXCLUSIVE4_1.
 		 */
+		if (open->op_createmode == NFS4_CREATE_EXCLUSIVE4_1)
+			createmode = NFS4_CREATE_EXCLUSIVE;
+		else
+			createmode = open->op_createmode;
+
 		status = nfsd_create_v3(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
-					&resfh, open->op_createmode,
+					&resfh, createmode,
 					(u32 *)open->op_verf.data,
 					&open->op_truncate, &created);

Signature · 1 line

1.7.4.1

J. Bruce Fields

2011-04-07 19:50 UTC #1 lore raw

Quoted 1 line · Mi Jinlong

At the recent kernel(2.6.39-rc1),

(But this is not a regression, right? This has been a problem all along.)

Quoted 3 lines

NFS server can't process OPEN with EXCLUSIVE4_1,
because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.

Is our handling of the attributes correct in this case? (See e.g. the op_bmval[1] assignment a few lines down.)

--b.

Quoted 46 lines

According to RFC5661, at nfsd_create_v3, EXCLUSIVE4_1 should be processed as EXCLUSIVE4.

Signed-off-by Mi Jinlong <[email protected]>

---
 fs/nfsd/nfs4proc.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

fs/nfsd/nfs4proc.c +8 -3

index 5fcb139..5325490 100644
@@ -173,7 +173,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 {
 	struct svc_fh resfh;
 	__be32 status;
-	int created = 0;
+	int created = 0, createmode = 0;
 
 	fh_init(&resfh, NFS4_FHSIZE);
 	open->op_truncate = 0;

@@ -196,11 +196,16 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 
 		/*
 		 * Note: create modes (UNCHECKED,GUARDED...) are the same
-		 * in NFSv4 as in v3.
+		 * in NFSv4 as in v3 except EXCLUSIVE4_1.
 		 */
+		if (open->op_createmode == NFS4_CREATE_EXCLUSIVE4_1)
+			createmode = NFS4_CREATE_EXCLUSIVE;
+		else
+			createmode = open->op_createmode;
+
 		status = nfsd_create_v3(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
-					&resfh, open->op_createmode,
+					&resfh, createmode,
 					(u32 *)open->op_verf.data,
 					&open->op_truncate, &created);

Signature · 1 line

1.7.4.1

Mi Jinlong

author

2011-04-08 09:56 UTC #2 lore raw

J. Bruce Fields :

Quoted 5 lines · Mi Jinlong

Quoted 1 line
At the recent kernel(2.6.39-rc1),
(But this is not a regression, right? This has been a problem all along.)

Yes, I think it's just a problem.

Quoted 7 lines

Quoted 4 lines
NFS server can't process OPEN with EXCLUSIVE4_1,
because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.
Is our handling of the attributes correct in this case? (See e.g. the op_bmval[1] assignment a few lines down.)

There is no problem of the p_bmval[1] assignment a few lines down. According to rfc5661 18.16.3, EXCLUSIVE4_1 supports the setting of attributes at file creation, we don't need to set p_bmval[1] assignment as EXCLUSIVE.

I think we should have a fix at nfsd_create_v3(), not at do_open_lookup(). Please ignore the old patch, a new one is as following.

--
thanks,
Mi Jinlong

=============================================================================

J. Bruce Fields

2011-04-18 20:07 UTC #3 lore raw

Quoted 37 lines · Mi Jinlong

J. Bruce Fields :
Quoted 5 lines · Mi Jinlong
Quoted 1 line
At the recent kernel(2.6.39-rc1),
(But this is not a regression, right? This has been a problem all along.)
Yes, I think it's just a problem.
Quoted 7 lines
Quoted 4 lines
NFS server can't process OPEN with EXCLUSIVE4_1,
because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.
Is our handling of the attributes correct in this case? (See e.g. the op_bmval[1] assignment a few lines down.)
There is no problem of the p_bmval[1] assignment a few lines down. According to rfc5661 18.16.3, EXCLUSIVE4_1 supports the setting of attributes at file creation, we don't need to set p_bmval[1] assignment as EXCLUSIVE.
I think we should have a fix at nfsd_create_v3(), not at do_open_lookup(). Please ignore the old patch, a new one is as following.
--
thanks,
Mi Jinlong
=============================================================================
Quoted 1 line
From 7adf0213b525c02761022c7fee60f52012d32a9a Mon Sep 17 00:00:00 2001
From: Mi Jinlong <[email protected]>
Date: Mon, 4 Apr 2011 00:49:19 +0800
Subject: [PATCH] nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correcttly

Quoted 5 lines

NFS server can't process OPEN with EXCLUSIVE4_1, because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.
This patch rename nfsd_create_v3() to do_nfsd_create(),

Good idea.

Review

Quoted 2 lines

(quoted diff) +1 -1

-	if (createmode == NFS3_CREATE_EXCLUSIVE) {
+	if (createmode & NFS3_CREATE_EXCLUSIVE) {

Using & NFS3_CREATE_EXCLUSIVE is a little too clever; I'd rather just write out (createmode == NFS3_CREATE_EXCLUSIVE) || (createmode == NFS4_CREATE_EXCLUSIVE4_1). If that's too cumbersome, define a static inline helper nfsd_create_is_exclusive(createmode) in a header somewhere.

Review

Quoted 3 lines

(quoted diff) +3 -0

+#ifdef CONFIG_NFSD_V4
+		case NFS4_CREATE_EXCLUSIVE4_1:
+#endif

And I'd rather avoid these ifdef's in the main part of the code. Could we move the definition of NFS4_CREATE_EXCLUSIVE4_1 someplace common instead?

--b.

Mi Jinlong

author

2011-04-20 09:18 UTC #4 lore raw

J. Bruce Fields:

Quoted 40 lines · Mi Jinlong

Quoted 31 lines
J. Bruce Fields :
Quoted 4 lines · Mi Jinlong
Quoted 1 line
At the recent kernel(2.6.39-rc1),
(But this is not a regression, right? This has been a problem all along.)
Yes, I think it's just a problem.
Quoted 5 lines
Quoted 3 lines
NFS server can't process OPEN with EXCLUSIVE4_1,
because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.
Is our handling of the attributes correct in this case? (See e.g. the op_bmval[1] assignment a few lines down.)
There is no problem of the p_bmval[1] assignment a few lines down. According to rfc5661 18.16.3, EXCLUSIVE4_1 supports the setting of attributes at file creation, we don't need to set p_bmval[1] assignment as EXCLUSIVE.
I think we should have a fix at nfsd_create_v3(), not at do_open_lookup(). Please ignore the old patch, a new one is as following.
--
thanks,
Mi Jinlong
=============================================================================
Quoted 1 line
From 7adf0213b525c02761022c7fee60f52012d32a9a Mon Sep 17 00:00:00 2001
From: Mi Jinlong <[email protected]>
Date: Mon, 4 Apr 2011 00:49:19 +0800
Subject: [PATCH] nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correcttly
Quoted 5 lines
NFS server can't process OPEN with EXCLUSIVE4_1, because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.
This patch rename nfsd_create_v3() to do_nfsd_create(),
Good idea.

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +1 -1
-	if (createmode == NFS3_CREATE_EXCLUSIVE) {
+	if (createmode & NFS3_CREATE_EXCLUSIVE) {
Using & NFS3_CREATE_EXCLUSIVE is a little too clever; I'd rather just write out (createmode == NFS3_CREATE_EXCLUSIVE) || (createmode == NFS4_CREATE_EXCLUSIVE4_1). If that's too cumbersome, define a static inline helper nfsd_create_is_exclusive(createmode) in a header somewhere.

Got it.

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +3 -0
+#ifdef CONFIG_NFSD_V4
+		case NFS4_CREATE_EXCLUSIVE4_1:
+#endif
And I'd rather avoid these ifdef's in the main part of the code. Could we move the definition of NFS4_CREATE_EXCLUSIVE4_1 someplace common instead?

Maybe we don't need the ifdef here.

Signature · 4 lines

----
thanks
Mi Jinlong
============================================================

J. Bruce Fields

2011-04-20 21:00 UTC #5 lore raw

Quoted 8 lines · Mi Jinlong

J. Bruce Fields:
Quoted 3 lines
And I'd rather avoid these ifdef's in the main part of the code. Could we move the definition of NFS4_CREATE_EXCLUSIVE4_1 someplace common instead?
Maybe we don't need the ifdef here.

OK, thanks--applying this version for 2.6.40.

--b.

Quoted 127 lines

Signature · 5 lines

-- 
----
thanks
Mi Jinlong
============================================================

Quoted 1 line

From 7363f39c0159e78bafcab3a002a6808d375f49f3 Mon Sep 17 00:00:00 2001

From: Mi Jinlong <[email protected]>
Date: Thu, 20 Apr 2011 17:06:25 +0800
Subject: [PATCH] nfsd41: make sure nfs server process OPEN with EXCLUSIVE4_1 correcttly

NFS server can't process OPEN with EXCLUSIVE4_1, because NFS server call nfsd_create_v3 to create file instead implement a separate one. But nfsd_create_v3 can't process createmode is EXCLUSIVE4_1.

This patch rename nfsd_create_v3() to do_nfsd_create(), and add some codes about process EXCLUSIVE4_1.

Signed-off-by Mi Jinlong <[email protected]>

---
 fs/nfsd/nfs3proc.c |    2 +-
 fs/nfsd/nfs4proc.c |    4 ++--
 fs/nfsd/vfs.c      |   20 ++++++++++++++++----
 fs/nfsd/vfs.h      |    2 +-
 4 files changed, 20 insertions(+), 8 deletions(-)

fs/nfsd/nfs3proc.c +1 -1

index 2247fc9..9095f3c 100644
@@ -245,7 +245,7 @@ nfsd3_proc_create(struct svc_rqst *rqstp, struct nfsd3_createargs *argp,
 	}
 
 	/* Now create the file and set attributes */
-	nfserr = nfsd_create_v3(rqstp, dirfhp, argp->name, argp->len,
+	nfserr = do_nfsd_create(rqstp, dirfhp, argp->name, argp->len,
 				attr, newfhp,
 				argp->createmode, argp->verf, NULL, NULL);

fs/nfsd/nfs4proc.c +2 -2

index 5fcb139..e1d100f 100644
@@ -196,9 +196,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 
 		/*
 		 * Note: create modes (UNCHECKED,GUARDED...) are the same
-		 * in NFSv4 as in v3.
+		 * in NFSv4 as in v3 except EXCLUSIVE4_1.
 		 */
-		status = nfsd_create_v3(rqstp, current_fh, open->op_fname.data,
+		status = do_nfsd_create(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
 					&resfh, open->op_createmode,
 					(u32 *)open->op_verf.data,

fs/nfsd/vfs.c +16 -4

index 2e1cebd..fee2530 100644
@@ -1340,11 +1340,18 @@ out_nfserr:
 }
 
 #ifdef CONFIG_NFSD_V3
+
+static inline int nfsd_create_is_exclusive(int createmode)
+{
+	return createmode == NFS3_CREATE_EXCLUSIVE
+	       || createmode == NFS4_CREATE_EXCLUSIVE4_1;
+}
+
 /*
- * NFSv3 version of nfsd_create
+ * NFSv3 and NFSv4 version of nfsd_create
  */
 __be32
-nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
+do_nfsd_create(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 *created)

@@ -1389,7 +1396,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (err)
 		goto out;
 
-	if (createmode == NFS3_CREATE_EXCLUSIVE) {
+	if (nfsd_create_is_exclusive(createmode)) {
 		/* solaris7 gets confused (bugid 4218508) if these have
 		 * the high bit set, so just clear the high bits. If this is
 		 * ever changed to use different attrs for storing the

@@ -1430,6 +1437,11 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			    && dchild->d_inode->i_atime.tv_sec == v_atime
 			    && dchild->d_inode->i_size  == 0 )
 				break;
+		case NFS4_CREATE_EXCLUSIVE4_1:
+			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
+			    && dchild->d_inode->i_atime.tv_sec == v_atime
+			    && dchild->d_inode->i_size  == 0 )
+				goto set_attr;
 			 /* fallthru */
 		case NFS3_CREATE_GUARDED:
 			err = nfserr_exist;

@@ -1448,7 +1460,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	nfsd_check_ignore_resizing(iap);
 
-	if (createmode == NFS3_CREATE_EXCLUSIVE) {
+	if (nfsd_create_is_exclusive(createmode)) {
 		/* Cram the verifier into atime/mtime */
 		iap->ia_valid = ATTR_MTIME|ATTR_ATIME
 			| ATTR_MTIME_SET|ATTR_ATIME_SET;

fs/nfsd/vfs.h +1 -1

index 9a370a5..10dbe9f 100644
@@ -54,7 +54,7 @@ __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
 				int type, dev_t rdev, struct svc_fh *res);
 #ifdef CONFIG_NFSD_V3
 __be32		nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *);
-__be32		nfsd_create_v3(struct svc_rqst *, struct svc_fh *,
+__be32		do_nfsd_create(struct svc_rqst *, struct svc_fh *,
 				char *name, int len, struct iattr *attrs,
 				struct svc_fh *res, int createmode,
 				u32 *verifier, int *truncp, int *created);

Signature · 1 line

1.7.4.4