On Mon, 2024-02-05 at 13:22 +1100, NeilBrown wrote:
> A recent change to check_for_locks() changed it to take ->flc_lock while
> holding ->fi_lock. This creates a lock inversion (reported by lockdep)
> because there is a case where ->fi_lock is taken while holding
> ->flc_lock.
>
> ->flc_lock is held across ->fl_lmops callbacks, and
> nfsd_break_deleg_cb() is one of those and does take ->fi_lock. However
> it doesn't need to.
>
> Prior to v4.17-rc1~110^2~22 ("nfsd: create a separate lease for each
> delegation") nfsd_break_deleg_cb() would walk the ->fi_delegations list
> and so needed the lock. Since then it doesn't walk the list and doesn't
> need the lock.
>
> Two actions are performed under the lock. One is to call
> nfsd_break_one_deleg which calls nfsd4_run_cb(). These doesn't act on
> the nfs4_file at all, so don't need the lock.
>
> The other is to set ->fi_had_conflict which is in the nfs4_file.
> This field is only ever set here (except when initialised to false)
> so there is no possible problem will multiple threads racing when
> setting it.
>
> The field is tested twice in nfs4_set_delegation(). The first test does
> not hold a lock and is documented as an opportunistic optimisation, so
> it doesn't impose any need to hold ->fi_lock while setting
> ->fi_had_conflict.
>
> The second test in nfs4_set_delegation() *is* make under ->fi_lock, so
> removing the locking when ->fi_had_conflict is set could make a change.
> The change could only be interesting if ->fi_had_conflict tested as
> false even though nfsd_break_one_deleg() ran before ->fi_lock was
> unlocked. i.e. while hash_delegation_locked() was running.
> As hash_delegation_lock() doesn't interact in any way with nfs4_run_cb()
> there can be no importance to this interaction.
>
> So this patch removes the locking from nfsd_break_one_deleg() and moves
> the final test on ->fi_had_conflict out of the locked region to make it
> clear that locking isn't important to the test. It is still tested
> *after* vfs_setlease() has succeeded. This might be significant and as
> vfs_setlease() takes ->flc_lock, and nfsd_break_one_deleg() is called
> under ->flc_lock this "after" is a true ordering provided by a spinlock.
>
> Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 12534e12dbb3..8b112673d389 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5154,10 +5154,8 @@ nfsd_break_deleg_cb(struct file_lock *fl)
> */
> fl->fl_break_time = 0;
>
> - spin_lock(&fp->fi_lock);
> fp->fi_had_conflict = true;
> nfsd_break_one_deleg(dp);
> - spin_unlock(&fp->fi_lock);
> return false;
> }
>
> @@ -5771,13 +5769,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> if (status)
> goto out_unlock;
>
> + status = -EAGAIN;
> + if (fp->fi_had_conflict)
> + 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);
> + status = hash_delegation_locked(dp, fp);
> spin_unlock(&fp->fi_lock);
> spin_unlock(&clp->cl_lock);
> spin_unlock(&state_lock);
Love the detailed explanation:
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
