Re: [PATCH v2] nfsd: drop st_mutex and rp_mutex before calling move_to_close_lru()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/22/23 3:01 PM, NeilBrown wrote:
On Sat, 23 Dec 2023, dai.ngo@xxxxxxxxxx wrote:
On 12/21/23 6:12 PM, NeilBrown wrote:
move_to_close_lru() is currently called with ->st_mutex and .rp_mutex held.
This can lead to a deadlock as move_to_close_lru() waits for sc_count to
drop to 2, and some threads holding a reference might be waiting for either
mutex.  These references will never be dropped so sc_count will never
reach 2.
Yes, I think there is potential deadlock here since both nfs4_preprocess_seqid_op
and nfsd4_find_and_lock_existing_open can increment the sc_count but then
have to wait for the st_mutex which being held by move_to_close_lru.

There can be no harm in dropping ->st_mutex to before
move_to_close_lru() because the only place that takes the mutex is
nfsd4_lock_ol_stateid(), and it quickly aborts if sc_type is
NFS4_CLOSED_STID, which it will be before move_to_close_lru() is called.

Similarly dropping .rp_mutex is safe after the state is closed and so
no longer usable.  Another way to look at this is that nothing
significant happens between when nfsd4_close() now calls
nfsd4_cstate_clear_replay(), and where nfsd4_proc_compound calls
nfsd4_cstate_clear_replay() a little later.

See also
   https://lore.kernel.org/lkml/4dd1fe21e11344e5969bb112e954affb@xxxxxx/T/
where this problem was raised but not successfully resolved.

Signed-off-by: NeilBrown <neilb@xxxxxxx>
---

Sorry - I posted v1 a little hastily.  I need to drop rp_mutex as well
to avoid the deadlock.  This also is safe.

   fs/nfsd/nfs4state.c | 12 ++++++++----
   1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 40415929e2ae..453714fbcd66 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7055,7 +7055,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
   	return status;
   }
-static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
+static bool nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
   {
   	struct nfs4_client *clp = s->st_stid.sc_client;
   	bool unhashed;
@@ -7072,11 +7072,11 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
   		list_for_each_entry(stp, &reaplist, st_locks)
   			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
   		free_ol_stateid_reaplist(&reaplist);
+		return false;
   	} else {
   		spin_unlock(&clp->cl_lock);
   		free_ol_stateid_reaplist(&reaplist);
-		if (unhashed)
-			move_to_close_lru(s, clp->net);
+		return unhashed;
   	}
   }
@@ -7092,6 +7092,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
   	struct nfs4_ol_stateid *stp;
   	struct net *net = SVC_NET(rqstp);
   	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+	bool need_move_to_close_list;
dprintk("NFSD: nfsd4_close on file %pd\n",
   			cstate->current_fh.fh_dentry);
@@ -7114,8 +7115,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
   	 */
   	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
- nfsd4_close_open_stateid(stp);
+	need_move_to_close_list = nfsd4_close_open_stateid(stp);
   	mutex_unlock(&stp->st_mutex);
+	nfsd4_cstate_clear_replay(cstate);
should nfsd4_cstate_clear_replay be called only if need_move_to_close_list
is true?
It certain could be done like that.

    if (need_move_to_close_list) {
          nfsd4_cstate_clear_replay(cstate);
          move_to_close_lru(stp, net);
    }

It would make almost no behavioural difference as
need_to_move_close_list is never true for v4.1 and later and almost
always true for v4.0, and nfsd4_cstate_clear_replay() does nothing for
v4.1 and later.
The only time behaviour would interrestingly different is when
nfsd4_close_open_stateid() found the state was already unlocked.  Then
need_move_to_close_list would be false, but nfsd4_cstate_clear_replay()
wouldn't be a no-op.

I thought the code was a little simpler the way I wrote it.  We don't
need the need_move_to_close_list guard on nfsd4_cstate_clear_replay(),
so I left it unguarded.
But I'm happy to change it if you can give a good reason - or even if
you just think it is clearer the other way.

My thinking is that if move_to_close_lru is not called then why bother
to do nfsd4_cstate_clear_replay. It's just easier to understand and
safer (against future changes) than having to go through all possible
scenarios to make sure it's safe to call nfsd4_cstate_clear_replay
regardless.

-Dai


Thanks,
NeilBrown

-Dai

+	if (need_move_to_close_list)
+		move_to_close_lru(stp, net);
/* v4.1+ suggests that we send a special stateid in here, since the
   	 * clients should just ignore this anyway. Since this is not useful




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux