2014-03-26 09:12:15

by Kinglong Mee

Subject: [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(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 550faf2..103d1ac 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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

2014-03-29 19:47:00

by J. Bruce Fields

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

On Sat, Mar 29, 2014 at 10:23:47AM +0800, Kinglong Mee wrote:
> 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().

Thanks, applying.--b.

>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 2 ++
> fs/nfsd/nfsfh.h | 14 +++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 550faf2..9849c61 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -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);
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 4775bc4..ad67964 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -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 */
>
> --
> 1.9.0
>

2014-03-28 22:42:04

by Kinglong Mee

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

于 2014/3/29 05:19, J. Bruce Fields 写道:
> On Wed, Mar 26, 2014 at 05:12:09PM +0800, Kinglong Mee wrote:
>> 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.

>
>> 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

>
> --b.
>
>>
>> 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(+)
>>
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 550faf2..103d1ac 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -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
>>
>

2014-03-28 21:19:21

by J. Bruce Fields

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

On Wed, Mar 26, 2014 at 05:12:09PM +0800, Kinglong Mee wrote:
> 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.

> 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.

>
> 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(+)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 550faf2..103d1ac 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -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
>

2014-03-29 00:58:31

by J. Bruce Fields

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

On Sat, Mar 29, 2014 at 06:41:54AM +0800, Kinglong Mee wrote:
>
>
> 于 2014/3/29 05:19, J. Bruce Fields 写道:
> >On Wed, Mar 26, 2014 at 05:12:09PM +0800, Kinglong Mee wrote:
> >>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.
>
> >
> >>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.

2014-03-29 02:23:57

by Kinglong Mee

Subject: [PATCH v2] 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(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 550faf2..9849c61 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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);
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 4775bc4..ad67964 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -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 */

--
1.9.0