[PATCH 1/2] NFSv4: Fix deadlock between nfs4_evict_inode() and nfs4_opendata_get_inode()

[PATCH 1/2] NFSv4: Fix deadlock between nfs4_evict_inode() and nfs4_opendata_get_inode()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If the inode is being evicted, but has to return a delegation first,
then it can cause a deadlock in the corner case where the server reboots
before the delegreturn completes, but while the call to iget5_locked() in
nfs4_opendata_get_inode() is waiting for the inode free to complete.
Since the open call still holds a session slot, the reboot recovery
cannot proceed.

In order to break the logjam, we can turn the delegation return into a
privileged operation for the case where we're evicting the inode. We
know that in that case, there can be no other state recovery operation
that conflicts.

Reported-by: zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>
Fixes: 5fcdfacc01f3 ("NFSv4: Return delegations synchronously in evict_inode")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4_fs.h  |  1 +
 fs/nfs/nfs4proc.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 065cb04222a1..543d916f79ab 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -205,6 +205,7 @@ struct nfs4_exception {
 	struct inode *inode;
 	nfs4_stateid *stateid;
 	long timeout;
+	unsigned char task_is_privileged : 1;
 	unsigned char delay : 1,
 		      recovering : 1,
 		      retry : 1;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d671b2884d5a..673809644981 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -589,6 +589,8 @@ int nfs4_handle_exception(struct nfs_server *server, int errorcode, struct nfs4_
 		goto out_retry;
 	}
 	if (exception->recovering) {
+		if (exception->task_is_privileged)
+			return -EDEADLOCK;
 		ret = nfs4_wait_clnt_recover(clp);
 		if (test_bit(NFS_MIG_FAILED, &server->mig_status))
 			return -EIO;
@@ -614,6 +616,8 @@ nfs4_async_handle_exception(struct rpc_task *task, struct nfs_server *server,
 		goto out_retry;
 	}
 	if (exception->recovering) {
+		if (exception->task_is_privileged)
+			return -EDEADLOCK;
 		rpc_sleep_on(&clp->cl_rpcwaitq, task, NULL);
 		if (test_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) == 0)
 			rpc_wake_up_queued_task(&clp->cl_rpcwaitq, task);
@@ -6417,6 +6421,7 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
 	struct nfs4_exception exception = {
 		.inode = data->inode,
 		.stateid = &data->stateid,
+		.task_is_privileged = data->args.seq_args.sa_privileged,
 	};
 
 	if (!nfs4_sequence_done(task, &data->res.seq_res))
@@ -6540,7 +6545,6 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
 	data = kzalloc(sizeof(*data), GFP_NOFS);
 	if (data == NULL)
 		return -ENOMEM;
-	nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1, 0);
 
 	nfs4_state_protect(server->nfs_client,
 			NFS_SP4_MACH_CRED_CLEANUP,
@@ -6571,6 +6575,12 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
 		}
 	}
 
+	if (!data->inode)
+		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
+				   1);
+	else
+		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
+				   0);
 	task_setup_data.callback_data = data;
 	msg.rpc_argp = &data->args;
 	msg.rpc_resp = &data->res;
-- 
2.31.1

[PATCH 2/2] NFSv4: Fix second deadlock in nfs4_evict_inode()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If the inode is being evicted but has to return a layout first, then
that too can cause a deadlock in the corner case where the server
reboots.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 673809644981..e25c16257545 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -9658,15 +9658,20 @@ int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp, bool sync)
 			&task_setup_data.rpc_client, &msg);
 
 	dprintk("--> %s\n", __func__);
+	lrp->inode = nfs_igrab_and_active(lrp->args.inode);
 	if (!sync) {
-		lrp->inode = nfs_igrab_and_active(lrp->args.inode);
 		if (!lrp->inode) {
 			nfs4_layoutreturn_release(lrp);
 			return -EAGAIN;
 		}
 		task_setup_data.flags |= RPC_TASK_ASYNC;
 	}
-	nfs4_init_sequence(&lrp->args.seq_args, &lrp->res.seq_res, 1, 0);
+	if (!lrp->inode)
+		nfs4_init_sequence(&lrp->args.seq_args, &lrp->res.seq_res, 1,
+				   1);
+	else
+		nfs4_init_sequence(&lrp->args.seq_args, &lrp->res.seq_res, 1,
+				   0);
 	task = rpc_run_task(&task_setup_data);
 	if (IS_ERR(task))
 		return PTR_ERR(task);
-- 
2.31.1

Re: [PATCH 1/2] NFSv4: Fix deadlock between nfs4_evict_inode() and nfs4_opendata_get_inode()

[zhangxiaoxu (A)]

在 2021/6/2 1:36, trondmy@kernel.org 写道:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> If the inode is being evicted, but has to return a delegation first,
> then it can cause a deadlock in the corner case where the server reboots
> before the delegreturn completes, but while the call to iget5_locked() in
> nfs4_opendata_get_inode() is waiting for the inode free to complete.
> Since the open call still holds a session slot, the reboot recovery
> cannot proceed.
> 
> In order to break the logjam, we can turn the delegation return into a
> privileged operation for the case where we're evicting the inode. We
> know that in that case, there can be no other state recovery operation
> that conflicts.
> 
it's looks good to me.

but i have another confuse, how to ensure no writeback when evict nfs inode?
because flush writes to server when close?
but not all close will flush writes to server.
> Reported-by: zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>
> Fixes: 5fcdfacc01f3 ("NFSv4: Return delegations synchronously in evict_inode")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>   fs/nfs/nfs4_fs.h  |  1 +
>   fs/nfs/nfs4proc.c | 12 +++++++++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 065cb04222a1..543d916f79ab 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -205,6 +205,7 @@ struct nfs4_exception {
>   	struct inode *inode;
>   	nfs4_stateid *stateid;
>   	long timeout;
> +	unsigned char task_is_privileged : 1;
>   	unsigned char delay : 1,
>   		      recovering : 1,
>   		      retry : 1;
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d671b2884d5a..673809644981 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -589,6 +589,8 @@ int nfs4_handle_exception(struct nfs_server *server, int errorcode, struct nfs4_
>   		goto out_retry;
>   	}
>   	if (exception->recovering) {
> +		if (exception->task_is_privileged)
> +			return -EDEADLOCK;
>   		ret = nfs4_wait_clnt_recover(clp);
>   		if (test_bit(NFS_MIG_FAILED, &server->mig_status))
>   			return -EIO;
> @@ -614,6 +616,8 @@ nfs4_async_handle_exception(struct rpc_task *task, struct nfs_server *server,
>   		goto out_retry;
>   	}
>   	if (exception->recovering) {
> +		if (exception->task_is_privileged)
> +			return -EDEADLOCK;
>   		rpc_sleep_on(&clp->cl_rpcwaitq, task, NULL);
>   		if (test_bit(NFS4CLNT_MANAGER_RUNNING, &clp->cl_state) == 0)
>   			rpc_wake_up_queued_task(&clp->cl_rpcwaitq, task);
> @@ -6417,6 +6421,7 @@ static void nfs4_delegreturn_done(struct rpc_task *task, void *calldata)
>   	struct nfs4_exception exception = {
>   		.inode = data->inode,
>   		.stateid = &data->stateid,
> +		.task_is_privileged = data->args.seq_args.sa_privileged,
>   	};
>   
>   	if (!nfs4_sequence_done(task, &data->res.seq_res))
> @@ -6540,7 +6545,6 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
>   	data = kzalloc(sizeof(*data), GFP_NOFS);
>   	if (data == NULL)
>   		return -ENOMEM;
> -	nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1, 0);
>   
>   	nfs4_state_protect(server->nfs_client,
>   			NFS_SP4_MACH_CRED_CLEANUP,
> @@ -6571,6 +6575,12 @@ static int _nfs4_proc_delegreturn(struct inode *inode, const struct cred *cred,
>   		}
>   	}
>   
> +	if (!data->inode)
> +		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
> +				   1);
> +	else
> +		nfs4_init_sequence(&data->args.seq_args, &data->res.seq_res, 1,
> +				   0);
>   	task_setup_data.callback_data = data;
>   	msg.rpc_argp = &data->args;
>   	msg.rpc_resp = &data->res;
> 

Re: [PATCH 1/2] NFSv4: Fix deadlock between nfs4_evict_inode() and nfs4_opendata_get_inode()

[Trond Myklebust]

On Mon, 2021-06-07 at 15:49 +0800, zhangxiaoxu (A) wrote:
> 
> 
> 在 2021/6/2 1:36, trondmy@kernel.org 写道:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If the inode is being evicted, but has to return a delegation
> > first,
> > then it can cause a deadlock in the corner case where the server
> > reboots
> > before the delegreturn completes, but while the call to
> > iget5_locked() in
> > nfs4_opendata_get_inode() is waiting for the inode free to
> > complete.
> > Since the open call still holds a session slot, the reboot recovery
> > cannot proceed.
> > 
> > In order to break the logjam, we can turn the delegation return
> > into a
> > privileged operation for the case where we're evicting the inode.
> > We
> > know that in that case, there can be no other state recovery
> > operation
> > that conflicts.
> > 
> it's looks good to me.
> 
> but i have another confuse, how to ensure no writeback when evict nfs
> inode?
> because flush writes to server when close?
> but not all close will flush writes to server.

The struct nfs_open_context holds a reference to the dentry (which
holds a reference to the inode) and to the superblock. The struct
nfs_page that is tracking page dirtiness then holds a reference to the
nfs_open_context.

That mechanism ensures the inode cannot be evicted until all dirty
pages have been either flushed or cancelled. The only thing we need to
worry about is the delegation and the pNFS layout since neither one is
allowed to reference the inode in any way (because otherwise they would
prevent the memory reclaim mechanisms from working).

> > Reported-by: zhangxiaoxu (A) <zhangxiaoxu5@huawei.com>
> > Fixes: 5fcdfacc01f3 ("NFSv4: Return delegations synchronously in
> > evict_inode")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >   fs/nfs/nfs4_fs.h  |  1 +
> >   fs/nfs/nfs4proc.c | 12 +++++++++++-
> >   2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > index 065cb04222a1..543d916f79ab 100644
> > --- a/fs/nfs/nfs4_fs.h
> > +++ b/fs/nfs/nfs4_fs.h
> > @@ -205,6 +205,7 @@ struct nfs4_exception {
> >         struct inode *inode;
> >         nfs4_stateid *stateid;
> >         long timeout;
> > +       unsigned char task_is_privileged : 1;
> >         unsigned char delay : 1,
> >                       recovering : 1,
> >                       retry : 1;
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index d671b2884d5a..673809644981 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -589,6 +589,8 @@ int nfs4_handle_exception(struct nfs_server
> > *server, int errorcode, struct nfs4_
> >                 goto out_retry;
> >         }
> >         if (exception->recovering) {
> > +               if (exception->task_is_privileged)
> > +                       return -EDEADLOCK;
> >                 ret = nfs4_wait_clnt_recover(clp);
> >                 if (test_bit(NFS_MIG_FAILED, &server->mig_status))
> >                         return -EIO;
> > @@ -614,6 +616,8 @@ nfs4_async_handle_exception(struct rpc_task
> > *task, struct nfs_server *server,
> >                 goto out_retry;
> >         }
> >         if (exception->recovering) {
> > +               if (exception->task_is_privileged)
> > +                       return -EDEADLOCK;
> >                 rpc_sleep_on(&clp->cl_rpcwaitq, task, NULL);
> >                 if (test_bit(NFS4CLNT_MANAGER_RUNNING, &clp-
> > >cl_state) == 0)
> >                         rpc_wake_up_queued_task(&clp->cl_rpcwaitq,
> > task);
> > @@ -6417,6 +6421,7 @@ static void nfs4_delegreturn_done(struct
> > rpc_task *task, void *calldata)
> >         struct nfs4_exception exception = {
> >                 .inode = data->inode,
> >                 .stateid = &data->stateid,
> > +               .task_is_privileged = data-
> > >args.seq_args.sa_privileged,
> >         };
> >   
> >         if (!nfs4_sequence_done(task, &data->res.seq_res))
> > @@ -6540,7 +6545,6 @@ static int _nfs4_proc_delegreturn(struct
> > inode *inode, const struct cred *cred,
> >         data = kzalloc(sizeof(*data), GFP_NOFS);
> >         if (data == NULL)
> >                 return -ENOMEM;
> > -       nfs4_init_sequence(&data->args.seq_args, &data-
> > >res.seq_res, 1, 0);
> >   
> >         nfs4_state_protect(server->nfs_client,
> >                         NFS_SP4_MACH_CRED_CLEANUP,
> > @@ -6571,6 +6575,12 @@ static int _nfs4_proc_delegreturn(struct
> > inode *inode, const struct cred *cred,
> >                 }
> >         }
> >   
> > +       if (!data->inode)
> > +               nfs4_init_sequence(&data->args.seq_args, &data-
> > >res.seq_res, 1,
> > +                                  1);
> > +       else
> > +               nfs4_init_sequence(&data->args.seq_args, &data-
> > >res.seq_res, 1,
> > +                                  0);
> >         task_setup_data.callback_data = data;
> >         msg.rpc_argp = &data->args;
> >         msg.rpc_resp = &data->res;
> > 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 1/2] NFSv4: Fix deadlock between nfs4_evict_inode() and nfs4_opendata_get_inode()

[zhangxiaoxu (A)]

在 2021/6/7 21:51, Trond Myklebust 写道:
>> 在 2021/6/2 1:36,trondmy@kernel.org  写道:
>>> From: Trond Myklebust<trond.myklebust@hammerspace.com>
>>>
>>> If the inode is being evicted, but has to return a delegation
>>> first,
>>> then it can cause a deadlock in the corner case where the server
>>> reboots
>>> before the delegreturn completes, but while the call to
>>> iget5_locked() in
>>> nfs4_opendata_get_inode() is waiting for the inode free to
>>> complete.
>>> Since the open call still holds a session slot, the reboot recovery
>>> cannot proceed.
>>>
>>> In order to break the logjam, we can turn the delegation return
>>> into a
>>> privileged operation for the case where we're evicting the inode.
>>> We
>>> know that in that case, there can be no other state recovery
>>> operation
>>> that conflicts.
>>>
>> it's looks good to me.
>>
>> but i have another confuse, how to ensure no writeback when evict nfs
>> inode?
>> because flush writes to server when close?
>> but not all close will flush writes to server.
> The struct nfs_open_context holds a reference to the dentry (which
> holds a reference to the inode) and to the superblock. The struct
> nfs_page that is tracking page dirtiness then holds a reference to the
> nfs_open_context.
> 
> That mechanism ensures the inode cannot be evicted until all dirty
> pages have been either flushed or cancelled. The only thing we need to
> worry about is the delegation and the pNFS layout since neither one is
> allowed to reference the inode in any way (because otherwise they would
> prevent the memory reclaim mechanisms from working).
> 
Yes, it is.
Thank you very much.