[PATCH 0/5] Clean up open CLAIM_PREVIOUS code path

[PATCH 0/5] Clean up open CLAIM_PREVIOUS code path

[Weston Andros Adamson]

My original patch was to just fix the NULL dereference, but Trond noticed
some other problems in _nfs4_opendata_reclaim_to_nfs4_state, so I went ahead
and fixed them.

I separated this into multiple patches for increased bisectability and made
sure the NULL dereference patch was first so that stable@ / distros can choose
to just fix the crash if they wish. 

I went a bit further than the issues Trond raised with the last patch, but I
think it's good for consistency and catching reference counting bugs as errors
and not BUG()s.

 -dros

Weston Andros Adamson (5):
  NFSv4: fix NULL dereference in open recover
  NFSv4: don't fail on missing fattr in open recover
  NFSv4: clean up state ref counting in open recover
  NFSv4: don't reprocess cached open CLAIM_PREVIOUS
  NFSv4: clean up nfs4_state reference counting

 fs/nfs/nfs4proc.c  | 39 +++++++++++++++++++++++++--------------
 fs/nfs/nfs4state.c |  3 ++-
 2 files changed, 27 insertions(+), 15 deletions(-)

-- 
1.7.12.4 (Apple Git-37)

[PATCH 1/5] NFSv4: fix NULL dereference in open recover

[Weston Andros Adamson]

_nfs4_opendata_reclaim_to_nfs4_state doesn't expect to see a cached
open CLAIM_PREVIOUS, but this can happen. An example is when there are
RDWR openers and RDONLY openers on a delegation stateid. The recovery
path will first try an open CLAIM_PREVIOUS for the RDWR openers, this
marks the delegation as not needing RECLAIM anymore, so the open
CLAIM_PREVIOUS for the RDONLY openers will not actually send an rpc.

The NULL dereference is due to _nfs4_opendata_reclaim_to_nfs4_state
returning PTR_ERR(rpc_status) when !rpc_done. When the open is
cached, rpc_done == 0 and rpc_status == 0, thus
_nfs4_opendata_reclaim_to_nfs4_state returns NULL - this is unexpected
by callers of nfs4_opendata_to_nfs4_state().

This can be reproduced easily by opening the same file two times on an
NFSv4.0 mount with delegations enabled, once as RDWR and once as RDONLY then
sleeping for a long time.  While the files are held open, kick off state
recovery and this NULL dereference will be hit every time.

An example OOPS:

[   65.003602] BUG: unable to handle kernel NULL pointer dereference at 00000000
00000030
[   65.005312] IP: [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[   65.006820] PGD 7b0ea067 PUD 791ff067 PMD 0
[   65.008075] Oops: 0000 [#1] SMP
[   65.008802] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache
snd_ens1371 gameport nfsd snd_rawmidi snd_ac97_codec ac97_bus btusb snd_seq snd
_seq_device snd_pcm ppdev bluetooth auth_rpcgss coretemp snd_page_alloc crc32_pc
lmul crc32c_intel ghash_clmulni_intel microcode rfkill nfs_acl vmw_balloon serio
_raw snd_timer lockd parport_pc e1000 snd soundcore parport i2c_piix4 shpchp vmw
_vmci sunrpc ata_generic mperf pata_acpi mptspi vmwgfx ttm scsi_transport_spi dr
m mptscsih mptbase i2c_core
[   65.018684] CPU: 0 PID: 473 Comm: 192.168.10.85-m Not tainted 3.11.2-201.fc19
.x86_64 #1
[   65.020113] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop
Reference Platform, BIOS 6.00 07/31/2013
[   65.022012] task: ffff88003707e320 ti: ffff88007b906000 task.ti: ffff88007b906000
[   65.023414] RIP: 0010:[<ffffffffa037d6ee>]  [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[   65.025079] RSP: 0018:ffff88007b907d10  EFLAGS: 00010246
[   65.026042] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[   65.027321] RDX: 0000000000000050 RSI: 0000000000000001 RDI: 0000000000000000
[   65.028691] RBP: ffff88007b907d38 R08: 0000000000016f60 R09: 0000000000000000
[   65.029990] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
[   65.031295] R13: 0000000000000050 R14: 0000000000000000 R15: 0000000000000001
[   65.032527] FS:  0000000000000000(0000) GS:ffff88007f600000(0000) knlGS:0000000000000000
[   65.033981] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   65.035177] CR2: 0000000000000030 CR3: 000000007b27f000 CR4: 00000000000407f0
[   65.036568] Stack:
[   65.037011]  0000000000000000 0000000000000001 ffff88007b907d90 ffff88007a880220
[   65.038472]  ffff88007b768de8 ffff88007b907d48 ffffffffa037e4a5 ffff88007b907d80
[   65.039935]  ffffffffa036a6c8 ffff880037020e40 ffff88007a880000 ffff880037020e40
[   65.041468] Call Trace:
[   65.042050]  [<ffffffffa037e4a5>] nfs4_close_state+0x15/0x20 [nfsv4]
[   65.043209]  [<ffffffffa036a6c8>] nfs4_open_recover_helper+0x148/0x1f0 [nfsv4]
[   65.044529]  [<ffffffffa036a886>] nfs4_open_recover+0x116/0x150 [nfsv4]
[   65.045730]  [<ffffffffa036d98d>] nfs4_open_reclaim+0xad/0x150 [nfsv4]
[   65.046905]  [<ffffffffa037d979>] nfs4_do_reclaim+0x149/0x5f0 [nfsv4]
[   65.048071]  [<ffffffffa037e1dc>] nfs4_run_state_manager+0x3bc/0x670 [nfsv4]
[   65.049436]  [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
[   65.050686]  [<ffffffffa037de20>] ? nfs4_do_reclaim+0x5f0/0x5f0 [nfsv4]
[   65.051943]  [<ffffffff81088640>] kthread+0xc0/0xd0
[   65.052831]  [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
[   65.054697]  [<ffffffff8165686c>] ret_from_fork+0x7c/0xb0
[   65.056396]  [<ffffffff81088580>] ? insert_kthread_work+0x40/0x40
[   65.058208] Code: 5c 41 5d 5d c3 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 e5 41 57 41 89 f7 41 56 41 89 ce 41 55 41 89 d5 41 54 53 48 89 fb <4c> 8b 67 30 f0 41 ff 44 24 44 49 8d 7c 24 40 e8 0e 0a 2d e1 44
[   65.065225] RIP  [<ffffffffa037d6ee>] __nfs4_close+0x1e/0x160 [nfsv4]
[   65.067175]  RSP <ffff88007b907d10>
[   65.068570] CR2: 0000000000000030
[   65.070098] ---[ end trace 0d1fe4f5c7dd6f8b ]---

Cc: <stable@vger.kernel.org> #3.7+
Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d2b4845..000063e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1317,7 +1317,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 	struct nfs4_state *state = data->state;
 	int ret;
 
-	if (!data->rpc_done) {
+	/* allow cached opens (!rpc_done && !rpc_status) */
+	if (!data->rpc_done && data->rpc_status) {
 		ret = data->rpc_status;
 		goto err;
 	}
-- 
1.7.12.4 (Apple Git-37)

[PATCH 2/5] NFSv4: don't fail on missing fattr in open recover

[Weston Andros Adamson]

This is an unneeded check that could cause the client to fail to recover
opens.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 000063e..8140366 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1323,12 +1323,6 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 		goto err;
 	}
 
-	ret = -ESTALE;
-	if (!(data->f_attr.valid & NFS_ATTR_FATTR_TYPE) ||
-	    !(data->f_attr.valid & NFS_ATTR_FATTR_FILEID) ||
-	    !(data->f_attr.valid & NFS_ATTR_FATTR_CHANGE))
-		goto err;
-
 	ret = -ENOMEM;
 	state = nfs4_get_open_state(inode, data->owner);
 	if (state == NULL)
-- 
1.7.12.4 (Apple Git-37)

[PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Weston Andros Adamson]

There's already a valid state (the one being recovered), so just
reference it. Also clean up error paths to avoid ref leaks.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8140366..8ae1589 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 		goto err;
 	}
 
-	ret = -ENOMEM;
-	state = nfs4_get_open_state(inode, data->owner);
-	if (state == NULL)
+	/* referenced the passed state */
+	ret = -EINVAL;
+	if (state == NULL || !atomic_inc_not_zero(&state->count))
 		goto err;
 
 	ret = nfs_refresh_inode(inode, &data->f_attr);
 	if (ret)
-		goto err;
+		goto err_put;
 
 	nfs_setsecurity(inode, &data->f_attr, data->f_label);
 
@@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 			    data->o_arg.fmode);
 
 	return state;
+
+err_put:
+	nfs4_put_open_state(state);
+
 err:
 	return ERR_PTR(ret);
-
 }
 
 static struct nfs4_state *
-- 
1.7.12.4 (Apple Git-37)

[PATCH 4/5] NFSv4: don't reprocess cached open CLAIM_PREVIOUS

[Weston Andros Adamson]

Cached opens have already been handled by _nfs4_opendata_reclaim_to_nfs4_state
and can safely skip being reprocessed, but must still call update_open_stateid
to make sure that all active fmodes are recovered.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8ae1589..a3b78df 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1315,12 +1315,15 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 {
 	struct inode *inode = data->state->inode;
 	struct nfs4_state *state = data->state;
+	bool cached_open = false;
 	int ret;
 
-	/* allow cached opens (!rpc_done && !rpc_status) */
-	if (!data->rpc_done && data->rpc_status) {
-		ret = data->rpc_status;
-		goto err;
+	if (!data->rpc_done) {
+		if (data->rpc_status) {
+			ret = data->rpc_status;
+			goto err;
+		} else
+			cached_open = true;
 	}
 
 	/* referenced the passed state */
@@ -1328,6 +1331,10 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 	if (state == NULL || !atomic_inc_not_zero(&state->count))
 		goto err;
 
+	/* cached opens have already been processed */
+	if (cached_open)
+		goto update;
+
 	ret = nfs_refresh_inode(inode, &data->f_attr);
 	if (ret)
 		goto err_put;
@@ -1336,6 +1343,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
 
 	if (data->o_res.delegation_type != 0)
 		nfs4_opendata_check_deleg(data, state);
+
+update:
 	update_open_stateid(state, &data->o_res.stateid, NULL,
 			    data->o_arg.fmode);
 
-- 
1.7.12.4 (Apple Git-37)

[PATCH 5/5] NFSv4: clean up nfs4_state reference counting

[Weston Andros Adamson]

Use atomic_inc_not_zero() to avoid referencing a state that is currently
being freed.

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfs/nfs4proc.c  | 8 ++++++--
 fs/nfs/nfs4state.c | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a3b78df..005543d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1275,7 +1275,8 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
 out:
 	return ERR_PTR(ret);
 out_return_state:
-	atomic_inc(&state->count);
+	if (!atomic_inc_not_zero(&state->count))
+		return ERR_PTR(-EINVAL);
 	return state;
 }
 
@@ -1429,7 +1430,10 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
 	if (opendata == NULL)
 		return ERR_PTR(-ENOMEM);
 	opendata->state = state;
-	atomic_inc(&state->count);
+	if (!atomic_inc_not_zero(&state->count)) {
+		nfs4_opendata_put(opendata);
+		return ERR_PTR(-EINVAL);
+	}
 	return opendata;
 }
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index cc14cbb..1c71907 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1416,7 +1416,8 @@ restart:
 			continue;
 		if (state->state == 0)
 			continue;
-		atomic_inc(&state->count);
+		if (!atomic_inc_not_zero(&state->count))
+			continue;
 		spin_unlock(&sp->so_lock);
 		status = ops->recover_open(sp, state);
 		if (status >= 0) {
-- 
1.7.12.4 (Apple Git-37)

Re: [PATCH 1/5] NFSv4: fix NULL dereference in open recover

[William Dauchy]

On Mon, Oct 21, 2013 at 7:10 PM, Weston Andros Adamson <dros@netapp.com> wrote:
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d2b4845..000063e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1317,7 +1317,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>         struct nfs4_state *state = data->state;
>         int ret;
>
> -       if (!data->rpc_done) {
> +       /* allow cached opens (!rpc_done && !rpc_status) */
> +       if (!data->rpc_done && data->rpc_status) {
>                 ret = data->rpc_status;
>                 goto err;
>         }

I'm also getting this trace on a 3.10.x; Trond, could you validate this patch?

Thanks,
-- 
William

Re: [PATCH 5/5] NFSv4: clean up nfs4_state reference counting

[Myklebust, Trond]

On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
> Use atomic_inc_not_zero() to avoid referencing a state that is currently
> being freed.
> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
>  fs/nfs/nfs4proc.c  | 8 ++++++--
>  fs/nfs/nfs4state.c | 3 ++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index a3b78df..005543d 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1275,7 +1275,8 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
>  out:
>  	return ERR_PTR(ret);
>  out_return_state:
> -	atomic_inc(&state->count);
> +	if (!atomic_inc_not_zero(&state->count))
> +		return ERR_PTR(-EINVAL);
>  	return state;
>  }
>  
> @@ -1429,7 +1430,10 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
>  	if (opendata == NULL)
>  		return ERR_PTR(-ENOMEM);
>  	opendata->state = state;
> -	atomic_inc(&state->count);
> +	if (!atomic_inc_not_zero(&state->count)) {
> +		nfs4_opendata_put(opendata);
> +		return ERR_PTR(-EINVAL);
> +	}
>  	return opendata;
>  }
>  
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index cc14cbb..1c71907 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1416,7 +1416,8 @@ restart:
>  			continue;
>  		if (state->state == 0)
>  			continue;
> -		atomic_inc(&state->count);
> +		if (!atomic_inc_not_zero(&state->count))
> +			continue;
>  		spin_unlock(&sp->so_lock);
>  		status = ops->recover_open(sp, state);
>  		if (status >= 0) {

Is this patch needed? We should already be holding a reference to the
open_context, so AFAICS we know that the state->count is non-zero.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 5/5] NFSv4: clean up nfs4_state reference counting

[Weston Andros Adamson]

On Oct 28, 2013, at 2:45 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> Use atomic_inc_not_zero() to avoid referencing a state that is currently
>> being freed.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> fs/nfs/nfs4proc.c  | 8 ++++++--
>> fs/nfs/nfs4state.c | 3 ++-
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index a3b78df..005543d 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1275,7 +1275,8 @@ static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
>> out:
>> 	return ERR_PTR(ret);
>> out_return_state:
>> -	atomic_inc(&state->count);
>> +	if (!atomic_inc_not_zero(&state->count))
>> +		return ERR_PTR(-EINVAL);
>> 	return state;
>> }
>> 
>> @@ -1429,7 +1430,10 @@ static struct nfs4_opendata *nfs4_open_recoverdata_alloc(struct nfs_open_context
>> 	if (opendata == NULL)
>> 		return ERR_PTR(-ENOMEM);
>> 	opendata->state = state;
>> -	atomic_inc(&state->count);
>> +	if (!atomic_inc_not_zero(&state->count)) {
>> +		nfs4_opendata_put(opendata);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> 	return opendata;
>> }
>> 
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index cc14cbb..1c71907 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1416,7 +1416,8 @@ restart:
>> 			continue;
>> 		if (state->state == 0)
>> 			continue;
>> -		atomic_inc(&state->count);
>> +		if (!atomic_inc_not_zero(&state->count))
>> +			continue;
>> 		spin_unlock(&sp->so_lock);
>> 		status = ops->recover_open(sp, state);
>> 		if (status >= 0) {
> 
> Is this patch needed? We should already be holding a reference to the
> open_context, so AFAICS we know that the state->count is non-zero.

I have no evidence that this is needed, it was purely for consistency.  Feel free to drop this patch.

-dros

> 
> Cheers
>  Trond
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: [PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Myklebust, Trond]

On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
> There's already a valid state (the one being recovered), so just
> reference it. Also clean up error paths to avoid ref leaks.
> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8140366..8ae1589 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>  		goto err;
>  	}
>  
> -	ret = -ENOMEM;
> -	state = nfs4_get_open_state(inode, data->owner);
> -	if (state == NULL)
> +	/* referenced the passed state */
> +	ret = -EINVAL;
> +	if (state == NULL || !atomic_inc_not_zero(&state->count))
>  		goto err;

We already know that state != NULL, and that state->count != 0 here, so
I applied a simplified version of this patch that just does an
atomic_inc(&state->count) just before the function return.

>  
>  	ret = nfs_refresh_inode(inode, &data->f_attr);
>  	if (ret)
> -		goto err;
> +		goto err_put;
>  
>  	nfs_setsecurity(inode, &data->f_attr, data->f_label);
>  
> @@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>  			    data->o_arg.fmode);
>  
>  	return state;
> +
> +err_put:
> +	nfs4_put_open_state(state);
> +
>  err:
>  	return ERR_PTR(ret);
> -
>  }
>  
>  static struct nfs4_state *

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 4/5] NFSv4: don't reprocess cached open CLAIM_PREVIOUS

[Myklebust, Trond]

On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
> Cached opens have already been handled by _nfs4_opendata_reclaim_to_nfs4_state
> and can safely skip being reprocessed, but must still call update_open_stateid
> to make sure that all active fmodes are recovered.
> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
>  fs/nfs/nfs4proc.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8ae1589..a3b78df 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1315,12 +1315,15 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>  {
>  	struct inode *inode = data->state->inode;
>  	struct nfs4_state *state = data->state;
> +	bool cached_open = false;
>  	int ret;
>  
> -	/* allow cached opens (!rpc_done && !rpc_status) */
> -	if (!data->rpc_done && data->rpc_status) {
> -		ret = data->rpc_status;
> -		goto err;
> +	if (!data->rpc_done) {
> +		if (data->rpc_status) {
> +			ret = data->rpc_status;
> +			goto err;
> +		} else
> +			cached_open = true;

This too was replaced by a simplified version. Once we get rid of the
state == NULL check, we can just replace the above with a 'goto update'

>  	}
>  
>  	/* referenced the passed state */
> @@ -1328,6 +1331,10 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>  	if (state == NULL || !atomic_inc_not_zero(&state->count))
>  		goto err;
>  
> +	/* cached opens have already been processed */
> +	if (cached_open)
> +		goto update;
> +
>  	ret = nfs_refresh_inode(inode, &data->f_attr);
>  	if (ret)
>  		goto err_put;
> @@ -1336,6 +1343,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>  
>  	if (data->o_res.delegation_type != 0)
>  		nfs4_opendata_check_deleg(data, state);
> +
> +update:
>  	update_open_stateid(state, &data->o_res.stateid, NULL,
>  			    data->o_arg.fmode);
>  

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 4/5] NFSv4: don't reprocess cached open CLAIM_PREVIOUS

[Weston Andros Adamson]

On Oct 28, 2013, at 5:08 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> Cached opens have already been handled by _nfs4_opendata_reclaim_to_nfs4_state
>> and can safely skip being reprocessed, but must still call update_open_stateid
>> to make sure that all active fmodes are recovered.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> fs/nfs/nfs4proc.c | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 8ae1589..a3b78df 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1315,12 +1315,15 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> {
>> 	struct inode *inode = data->state->inode;
>> 	struct nfs4_state *state = data->state;
>> +	bool cached_open = false;
>> 	int ret;
>> 
>> -	/* allow cached opens (!rpc_done && !rpc_status) */
>> -	if (!data->rpc_done && data->rpc_status) {
>> -		ret = data->rpc_status;
>> -		goto err;
>> +	if (!data->rpc_done) {
>> +		if (data->rpc_status) {
>> +			ret = data->rpc_status;
>> +			goto err;
>> +		} else
>> +			cached_open = true;
> 
> This too was replaced by a simplified version. Once we get rid of the
> state == NULL check, we can just replace the above with a 'goto update’

Ok - to both updates.  I guess I got a little too fancy.

-dros

> 
>> 	}
>> 
>> 	/* referenced the passed state */
>> @@ -1328,6 +1331,10 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> 	if (state == NULL || !atomic_inc_not_zero(&state->count))
>> 		goto err;
>> 
>> +	/* cached opens have already been processed */
>> +	if (cached_open)
>> +		goto update;
>> +
>> 	ret = nfs_refresh_inode(inode, &data->f_attr);
>> 	if (ret)
>> 		goto err_put;
>> @@ -1336,6 +1343,8 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> 
>> 	if (data->o_res.delegation_type != 0)
>> 		nfs4_opendata_check_deleg(data, state);
>> +
>> +update:
>> 	update_open_stateid(state, &data->o_res.stateid, NULL,
>> 			    data->o_arg.fmode);
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: [PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Weston Andros Adamson]

On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>> There's already a valid state (the one being recovered), so just
>> reference it. Also clean up error paths to avoid ref leaks.
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 8140366..8ae1589 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> 		goto err;
>> 	}
>> 
>> -	ret = -ENOMEM;
>> -	state = nfs4_get_open_state(inode, data->owner);
>> -	if (state == NULL)
>> +	/* referenced the passed state */
>> +	ret = -EINVAL;
>> +	if (state == NULL || !atomic_inc_not_zero(&state->count))
>> 		goto err;
> 
> We already know that state != NULL, and that state->count != 0 here, so
> I applied a simplified version of this patch that just does an
> atomic_inc(&state->count) just before the function return.

I just checked out the simplified patch - it looks good.

Acked-by: Weston Andros Adamson <dros@netapp.com>

-dros


> 
>> 
>> 	ret = nfs_refresh_inode(inode, &data->f_attr);
>> 	if (ret)
>> -		goto err;
>> +		goto err_put;
>> 
>> 	nfs_setsecurity(inode, &data->f_attr, data->f_label);
>> 
>> @@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>> 			    data->o_arg.fmode);
>> 
>> 	return state;
>> +
>> +err_put:
>> +	nfs4_put_open_state(state);
>> +
>> err:
>> 	return ERR_PTR(ret);
>> -
>> }
>> 
>> static struct nfs4_state *
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: [PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Weston Andros Adamson]

On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <dros@netapp.com> wrote:

> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
> 
>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>> There's already a valid state (the one being recovered), so just
>>> reference it. Also clean up error paths to avoid ref leaks.
>>> 
>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>> ---
>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 8140366..8ae1589 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>> 		goto err;
>>> 	}
>>> 
>>> -	ret = -ENOMEM;
>>> -	state = nfs4_get_open_state(inode, data->owner);
>>> -	if (state == NULL)
>>> +	/* referenced the passed state */
>>> +	ret = -EINVAL;
>>> +	if (state == NULL || !atomic_inc_not_zero(&state->count))
>>> 		goto err;
>> 
>> We already know that state != NULL, and that state->count != 0 here, so
>> I applied a simplified version of this patch that just does an
>> atomic_inc(&state->count) just before the function return.
> 
> I just checked out the simplified patch - it looks good.
> 
> Acked-by: Weston Andros Adamson <dros@netapp.com>

Oh, besides redundant CC lines?

    Cc: stable@vger.kernel.org # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
    Cc: stable@vger.kernel.org # 3.7.x

-dros

> 
> -dros
> 
> 
>> 
>>> 
>>> 	ret = nfs_refresh_inode(inode, &data->f_attr);
>>> 	if (ret)
>>> -		goto err;
>>> +		goto err_put;
>>> 
>>> 	nfs_setsecurity(inode, &data->f_attr, data->f_label);
>>> 
>>> @@ -1340,9 +1340,12 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>> 			    data->o_arg.fmode);
>>> 
>>> 	return state;
>>> +
>>> +err_put:
>>> +	nfs4_put_open_state(state);
>>> +
>>> err:
>>> 	return ERR_PTR(ret);
>>> -
>>> }
>>> 
>>> static struct nfs4_state *
>> 
>> -- 
>> Trond Myklebust
>> Linux NFS client maintainer
>> 
>> NetApp
>> Trond.Myklebust@netapp.com
>> www.netapp.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Myklebust, Trond]

On Oct 29, 2013, at 8:33 AM, Weston Andros Adamson <dros@netapp.com> wrote:

> 
> On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
>> 
>>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>>> There's already a valid state (the one being recovered), so just
>>>> reference it. Also clean up error paths to avoid ref leaks.
>>>> 
>>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>>> ---
>>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 8140366..8ae1589 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>>> 		goto err;
>>>> 	}
>>>> 
>>>> -	ret = -ENOMEM;
>>>> -	state = nfs4_get_open_state(inode, data->owner);
>>>> -	if (state == NULL)
>>>> +	/* referenced the passed state */
>>>> +	ret = -EINVAL;
>>>> +	if (state == NULL || !atomic_inc_not_zero(&state->count))
>>>> 		goto err;
>>> 
>>> We already know that state != NULL, and that state->count != 0 here, so
>>> I applied a simplified version of this patch that just does an
>>> atomic_inc(&state->count) just before the function return.
>> 
>> I just checked out the simplified patch - it looks good.
>> 
>> Acked-by: Weston Andros Adamson <dros@netapp.com>
> 
> Oh, besides redundant CC lines?
> 
>    Cc: stable@vger.kernel.org # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
>    Cc: stable@vger.kernel.org # 3.7.x

Those are there to indicate a dependency on the other patch. I believe this is in accordance with Documentation/stable-patches.txt...

Cheers
  Trond

Re: [PATCH 3/5] NFSv4: clean up state ref counting in open recover

[Weston Andros Adamson]

On Oct 29, 2013, at 8:41 AM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> 
> On Oct 29, 2013, at 8:33 AM, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> 
>> On Oct 29, 2013, at 8:32 AM, Weston Andros Adamson <dros@netapp.com> wrote:
>> 
>>> On Oct 28, 2013, at 5:06 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
>>> 
>>>> On Mon, 2013-10-21 at 13:10 -0400, Weston Andros Adamson wrote:
>>>>> There's already a valid state (the one being recovered), so just
>>>>> reference it. Also clean up error paths to avoid ref leaks.
>>>>> 
>>>>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>>>>> ---
>>>>> fs/nfs/nfs4proc.c | 13 ++++++++-----
>>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>>> 
>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>> index 8140366..8ae1589 100644
>>>>> --- a/fs/nfs/nfs4proc.c
>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>> @@ -1323,14 +1323,14 @@ _nfs4_opendata_reclaim_to_nfs4_state(struct nfs4_opendata *data)
>>>>> 		goto err;
>>>>> 	}
>>>>> 
>>>>> -	ret = -ENOMEM;
>>>>> -	state = nfs4_get_open_state(inode, data->owner);
>>>>> -	if (state == NULL)
>>>>> +	/* referenced the passed state */
>>>>> +	ret = -EINVAL;
>>>>> +	if (state == NULL || !atomic_inc_not_zero(&state->count))
>>>>> 		goto err;
>>>> 
>>>> We already know that state != NULL, and that state->count != 0 here, so
>>>> I applied a simplified version of this patch that just does an
>>>> atomic_inc(&state->count) just before the function return.
>>> 
>>> I just checked out the simplified patch - it looks good.
>>> 
>>> Acked-by: Weston Andros Adamson <dros@netapp.com>
>> 
>> Oh, besides redundant CC lines?
>> 
>>   Cc: stable@vger.kernel.org # 3.7.x: a43ec98b72a: NFSv4: don't fail on missing
>>   Cc: stable@vger.kernel.org # 3.7.x
> 
> Those are there to indicate a dependency on the other patch. I believe this is in accordance with Documentation/stable-patches.txt...

Ah! Thanks,

-dros