NFSD: Re-initialize fh_post/pre_saved between two operations

PATCH In review Kinglong Mee 2 participants 6 messages Mar 26, 2014 – Mar 29, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Kinglong Mee

author

2014-03-26 09:12 UTC #0 lore raw

PATCH 2/2 NFSD: Re-initialize fh_post/pre_saved between two operations

Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."

When one compound RPC contains two or more SETATTR operation for one filehandle,the second SETATTR will cause the message.

Because after the first SETATTR, nfsd will not call fh_put() to release current filehandle, it means filehandle have unlocked with fh_post_saved = 1.
The second SETATTR find fh_post_saved = 1, and printk the message.

This patch re-initialize fh_post/pre_saved between two operations.

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)

fs/nfsd/nfs4proc.c +3 -0

index 550faf2..103d1ac 100644
@@ -1356,6 +1356,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			  !(opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
 			op->status = nfserr_moved;
 			goto encode_op;
+		} else {
+			current_fh->fh_post_saved = 0;
+			current_fh->fh_pre_saved = 0;
 		}
 
 		/* If op is non-idempotent */

Signature · 1 line

1.8.5.3

J. Bruce Fields

2014-03-28 21:19 UTC #1 lore raw

Quoted 2 lines · Kinglong Mee

Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."

Thanks for looking into this. I agree that we should clear fh_pre_saved and fh_post_saved between compound ops.

This is kind of non-obvious, though, so I think it would be worth moving these two assignments to a little helper function (how about "fh_clear_wcc_data()" for a name?) and adding a comment with the definition of the function, explaining why we need it.

Quoted 2 lines

When one compound RPC contains two or more SETATTR operation for one filehandle,the second SETATTR will cause the message.

Also worth noting that this affects any op that locks the filehandle (e.g. a compound with two LINK ops would probably trigger the same warning.)

--b.

Quoted 30 lines

Because after the first SETATTR, nfsd will not call fh_put() to release current filehandle, it means filehandle have unlocked with fh_post_saved = 1.
The second SETATTR find fh_post_saved = 1, and printk the message.
This patch re-initialize fh_post/pre_saved between two operations.
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/nfsd/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)
fs/nfsd/nfs4proc.c +3 -0
index 550faf2..103d1ac 100644
@@ -1356,6 +1356,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			  !(opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
 			op->status = nfserr_moved;
 			goto encode_op;
+		} else {
+			current_fh->fh_post_saved = 0;
+			current_fh->fh_pre_saved = 0;
 		}
 
 		/* If op is non-idempotent */
Signature · 1 line
1.8.5.3

Kinglong Mee

author

2014-03-28 22:42 UTC #2 lore raw

Quoted 11 lines · J. Bruce Fields

Quoted 2 lines · Kinglong Mee
Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."
Thanks for looking into this. I agree that we should clear fh_pre_saved and fh_post_saved between compound ops.
This is kind of non-obvious, though, so I think it would be worth moving these two assignments to a little helper function (how about "fh_clear_wcc_data()" for a name?) and adding a comment with the definition of the function, explaining why we need it.

That's great.
Thanks for your advice.

Quoted 7 lines

Quoted 3 lines
When one compound RPC contains two or more SETATTR operation for one filehandle,the second SETATTR will cause the message.
Also worth noting that this affects any op that locks the filehandle (e.g. a compound with two LINK ops would probably trigger the same warning.)

Yes, that's right.

Thanks,
Kinglong Mee

Quoted 34 lines

--b.
Quoted 30 lines
Because after the first SETATTR, nfsd will not call fh_put() to release current filehandle, it means filehandle have unlocked with fh_post_saved = 1.
The second SETATTR find fh_post_saved = 1, and printk the message.
This patch re-initialize fh_post/pre_saved between two operations.
Signed-off-by Kinglong Mee <[email protected]>
---
  fs/nfsd/nfs4proc.c | 3 +++
  1 file changed, 3 insertions(+)
fs/nfsd/nfs4proc.c +3 -1
index 550faf2..103d1ac 100644
@@ -1356,6 +1356,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
  			  !(opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
  			op->status = nfserr_moved;
  			goto encode_op;
+		} else {
+			current_fh->fh_post_saved = 0;
+			current_fh->fh_pre_saved = 0;
  		}
 
  		/* If op is non-idempotent */
--
1.8.5.3

J. Bruce Fields

2014-03-29 00:58 UTC #3 lore raw

Quoted 19 lines · Kinglong Mee

Quoted 11 lines · J. Bruce Fields
Quoted 2 lines · Kinglong Mee
Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."
Thanks for looking into this. I agree that we should clear fh_pre_saved and fh_post_saved between compound ops.
This is kind of non-obvious, though, so I think it would be worth moving these two assignments to a little helper function (how about "fh_clear_wcc_data()" for a name?) and adding a comment with the definition of the function, explaining why we need it.
That's great.
Thanks for your advice.
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
When one compound RPC contains two or more SETATTR operation for one filehandle,the second SETATTR will cause the message.
Also worth noting that this affects any op that locks the filehandle (e.g. a compound with two LINK ops would probably trigger the same warning.)
Yes, that's right.

OK, thanks, I'll assume you're sending a revised patch.

--b.

Kinglong Mee

author

2014-03-29 02:23 UTC #4 lore raw

PATCH v2 1/1 NFSD: Clear wcc data between compound ops

Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."

When one compound RPC contains two or more ops that locks the filehandle,the second op will cause the message.

As two SETATTR ops, after the first SETATTR, nfsd will not call fh_put() to release current filehandle, it means filehandle have unlocked with fh_post_saved = 1.
The second SETATTR find fh_post_saved = 1, and printk the message.

v2: introduce helper fh_clear_wcc().

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4proc.c |  2 ++
 fs/nfsd/nfsfh.h    | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

fs/nfsd/nfs4proc.c +2 -0

index 550faf2..9849c61 100644
@@ -1358,6 +1358,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			goto encode_op;
 		}
 
+		fh_clear_wcc(current_fh);
+
 		/* If op is non-idempotent */
 		if (opdesc->op_flags & OP_MODIFIES_SOMETHING) {
 			plen = opdesc->op_rsize_bop(rqstp, op);

fs/nfsd/nfsfh.h +13 -1

index 4775bc4..ad67964 100644
@@ -133,6 +133,17 @@ fh_init(struct svc_fh *fhp, int maxsize)
 
 #ifdef CONFIG_NFSD_V3
 /*
+ * The wcc data stored in current_fh should be cleared
+ * between compound ops.
+ */
+static inline void
+fh_clear_wcc(struct svc_fh *fhp)
+{
+	fhp->fh_post_saved = 0;
+	fhp->fh_pre_saved = 0;
+}
+
+/*
  * Fill in the pre_op attr for the wcc data
  */
 static inline void

@@ -152,7 +163,8 @@ fill_pre_wcc(struct svc_fh *fhp)
 
 extern void fill_post_wcc(struct svc_fh *);
 #else
-#define	fill_pre_wcc(ignored)
+#define fh_clear_wcc(ignored)
+#define fill_pre_wcc(ignored)
 #define fill_post_wcc(notused)
 #endif /* CONFIG_NFSD_V3 */

Signature · 1 line

1.9.0

J. Bruce Fields

2014-03-29 19:47 UTC #5 lore raw

Re: [PATCH v2] NFSD: Clear wcc data between compound ops

Quoted 6 lines · Kinglong Mee

Testing NFS4.0 by pynfs, I got some messeages as,
"nfsd: inode locked twice during operation."
When one compound RPC contains two or more ops that locks the filehandle,the second op will cause the message.

Quoted 6 lines

As two SETATTR ops, after the first SETATTR, nfsd will not call fh_put() to release current filehandle, it means filehandle have unlocked with fh_post_saved = 1.
The second SETATTR find fh_post_saved = 1, and printk the message.
v2: introduce helper fh_clear_wcc().

Thanks, applying.--b.

Quoted 55 lines

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4proc.c |  2 ++
 fs/nfsd/nfsfh.h    | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

fs/nfsd/nfs4proc.c +2 -0

index 550faf2..9849c61 100644
@@ -1358,6 +1358,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
 			goto encode_op;
 		}
 
+		fh_clear_wcc(current_fh);
+
 		/* If op is non-idempotent */
 		if (opdesc->op_flags & OP_MODIFIES_SOMETHING) {
 			plen = opdesc->op_rsize_bop(rqstp, op);

fs/nfsd/nfsfh.h +13 -1

index 4775bc4..ad67964 100644
@@ -133,6 +133,17 @@ fh_init(struct svc_fh *fhp, int maxsize)
 
 #ifdef CONFIG_NFSD_V3
 /*
+ * The wcc data stored in current_fh should be cleared
+ * between compound ops.
+ */
+static inline void
+fh_clear_wcc(struct svc_fh *fhp)
+{
+	fhp->fh_post_saved = 0;
+	fhp->fh_pre_saved = 0;
+}
+
+/*
  * Fill in the pre_op attr for the wcc data
  */
 static inline void

@@ -152,7 +163,8 @@ fill_pre_wcc(struct svc_fh *fhp)
 
 extern void fill_post_wcc(struct svc_fh *);
 #else
-#define	fill_pre_wcc(ignored)
+#define fh_clear_wcc(ignored)
+#define fill_pre_wcc(ignored)
 #define fill_post_wcc(notused)
 #endif /* CONFIG_NFSD_V3 */

Signature · 1 line

1.9.0