All of lore.kernel.org
 help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.de>
To: Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org, Olga Kornievskaia <kolga@netapp.com>,
	Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
	Christoph Hellwig <hch@lst.de>, Tom Haynes <loghyr@gmail.com>
Subject: [PATCH 02/13] nfsd: hold ->cl_lock for hash_delegation_locked()
Date: Mon, 29 Jan 2024 14:29:24 +1100	[thread overview]
Message-ID: <20240129033637.2133-3-neilb@suse.de> (raw)
In-Reply-To: <20240129033637.2133-1-neilb@suse.de>

The protocol for creating a new state in nfsd is to allocate the state
leaving it largely uninitialised, add that state to the ->cl_stateids
idr so as to reserve a state-id, then complete initialisation of the
state and only set ->sc_type to non-zero once the state is fully
initialised.

If a state is found in the idr with ->sc_type == 0, it is ignored.
The ->cl_lock lock is used to avoid races - it is held while checking
sc_type during lookup, and held when a non-zero value is stored in
->sc_type.

... except... hash_delegation_locked() finalises the initialisation of a
delegation state, but does NOT hold ->cl_lock.

So this patch takes ->cl_lock at the appropriate time w.r.t other locks,
and so ensures there are no races (which are extremely unlikely in any
case).
As ->fi_lock is often taken when ->cl_lock is held, we need to take
->cl_lock first of those two.
Currently ->cl_lock and state_lock are never both taken at the same time.
We need both for this patch so an arbitrary choice is needed concerning
which to take first.  As state_lock is more global, it might be more
contended, so take it first.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d377a0a56e45..051c3e99fac6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1312,6 +1312,7 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
 
 	lockdep_assert_held(&state_lock);
 	lockdep_assert_held(&fp->fi_lock);
+	lockdep_assert_held(&clp->cl_lock);
 
 	if (nfs4_delegation_exists(clp, fp))
 		return -EAGAIN;
@@ -5557,12 +5558,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		goto out_unlock;
 
 	spin_lock(&state_lock);
+	spin_lock(&clp->cl_lock);
 	spin_lock(&fp->fi_lock);
 	if (fp->fi_had_conflict)
 		status = -EAGAIN;
 	else
 		status = hash_delegation_locked(dp, fp);
 	spin_unlock(&fp->fi_lock);
+	spin_unlock(&clp->cl_lock);
 	spin_unlock(&state_lock);
 
 	if (status)
-- 
2.43.0


  parent reply	other threads:[~2024-01-29  3:37 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-29  3:29 [PATCH 00/13 v4] nfsd: support admin-revocation of v4 state NeilBrown
2024-01-29  3:29 ` [PATCH 01/13] nfsd: remove stale comment in nfs4_show_deleg() NeilBrown
2024-01-29  3:29 ` NeilBrown [this message]
2024-01-29  3:29 ` [PATCH 03/13] nfsd: don't call functions with side-effecting inside WARN_ON() NeilBrown
2024-01-29 11:18   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 04/13] nfsd: avoid race after unhash_delegation_locked() NeilBrown
2024-01-29  3:29 ` [PATCH 05/13] nfsd: split sc_status out of sc_type NeilBrown
2024-01-29 12:21   ` Jeff Layton
2024-01-29 14:04   ` Chuck Lever
2024-01-29  3:29 ` [PATCH 06/13] nfsd: prepare for supporting admin-revocation of state NeilBrown
2024-01-29  9:43   ` kernel test robot
2024-01-29 12:22   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 07/13] nfsd: allow state with no file to appear in /proc/fs/nfsd/clients/*/states NeilBrown
2024-01-29 12:23   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 08/13] nfsd: report in /proc/fs/nfsd/clients/*/states when state is admin-revoke NeilBrown
2024-01-29 12:24   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 09/13] nfsd: allow admin-revoked NFSv4.0 state to be freed NeilBrown
2024-01-29 12:29   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 10/13] nfsd: allow lock state ids to be revoked and then freed NeilBrown
2024-01-29 12:30   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 11/13] nfsd: allow open " NeilBrown
2024-01-29 12:31   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 12/13] nfsd: allow delegation " NeilBrown
2024-01-29 12:32   ` Jeff Layton
2024-01-29  3:29 ` [PATCH 13/13] nfsd: allow layout state to be admin-revoked NeilBrown
2024-01-29 12:38   ` Jeff Layton
2024-01-30  1:07     ` NeilBrown
2024-01-30  1:08 [PATCH 00/13 v5] nfsd: support admin-revocation of v4 state NeilBrown
2024-01-30  1:08 ` [PATCH 02/13] nfsd: hold ->cl_lock for hash_delegation_locked() NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240129033637.2133-3-neilb@suse.de \
    --to=neilb@suse.de \
    --cc=Dai.Ngo@oracle.com \
    --cc=chuck.lever@oracle.com \
    --cc=hch@lst.de \
    --cc=jlayton@kernel.org \
    --cc=kolga@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=loghyr@gmail.com \
    --cc=tom@talpey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.