From dc2758eaa213fc90ce7939ccc19d9c82ca170c03 Mon Sep 17 00:00:00 2001 From: Sasha Levin Date: Sun, 10 Mar 2024 09:21:37 -0400 Subject: [PATCH] Drop nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch Signed-off-by: Sasha Levin --- ...-take-fi_lock-in-nfsd_break_deleg_cb.patch | 97 ------------------- queue-5.10/series | 1 - ...-take-fi_lock-in-nfsd_break_deleg_cb.patch | 97 ------------------- queue-5.15/series | 1 - ...-take-fi_lock-in-nfsd_break_deleg_cb.patch | 97 ------------------- queue-5.4/series | 1 - 6 files changed, 294 deletions(-) delete mode 100644 queue-5.10/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch delete mode 100644 queue-5.15/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch delete mode 100644 queue-5.4/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch diff --git a/queue-5.10/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch b/queue-5.10/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch deleted file mode 100644 index 2fbf9296dff..00000000000 --- a/queue-5.10/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch +++ /dev/null @@ -1,97 +0,0 @@ -From 72721cc9efa6c9217b39095adbc30d0efd0b10db Mon Sep 17 00:00:00 2001 -From: Sasha Levin -Date: Mon, 5 Feb 2024 13:22:39 +1100 -Subject: nfsd: don't take fi_lock in nfsd_break_deleg_cb() - -From: NeilBrown - -[ Upstream commit 5ea9a7c5fe4149f165f0e3b624fe08df02b6c301 ] - -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 -Reviewed-by: Jeff Layton -Signed-off-by: Chuck Lever -Signed-off-by: Sasha Levin ---- - 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 16b073c637986..7ff1f85f1dd9a 100644 ---- a/fs/nfsd/nfs4state.c -+++ b/fs/nfsd/nfs4state.c -@@ -4617,10 +4617,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 ret; - } - -@@ -5049,12 +5047,13 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, - if (status) - goto out_clnt_odstate; - -+ status = -EAGAIN; -+ if (fp->fi_had_conflict) -+ goto out_unlock; -+ - spin_lock(&state_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(&state_lock); - --- -2.43.0 - diff --git a/queue-5.10/series b/queue-5.10/series index 3711cb3f289..72bb883bc2d 100644 --- a/queue-5.10/series +++ b/queue-5.10/series @@ -69,7 +69,6 @@ exit-fix-typo-in-comment-s-sub-theads-sub-threads.patch exit-wait_task_zombie-kill-the-no-longer-necessary-s.patch serial-max310x-unprepare-and-disable-clock-in-error-.patch drivers-hv-vmbus-drop-error-message-when-no-request-.patch -nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch regmap-allow-to-define-reg_update_bits-for-no-bus-co.patch regmap-add-bulk-read-write-callbacks-into-regmap_con.patch serial-max310x-make-accessing-revision-id-interface-.patch diff --git a/queue-5.15/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch b/queue-5.15/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch deleted file mode 100644 index 865ad5a0656..00000000000 --- a/queue-5.15/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch +++ /dev/null @@ -1,97 +0,0 @@ -From 717ab4f6347df5c3588c0e63148f784d76ec369c Mon Sep 17 00:00:00 2001 -From: Sasha Levin -Date: Mon, 5 Feb 2024 13:22:39 +1100 -Subject: nfsd: don't take fi_lock in nfsd_break_deleg_cb() - -From: NeilBrown - -[ Upstream commit 5ea9a7c5fe4149f165f0e3b624fe08df02b6c301 ] - -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 -Reviewed-by: Jeff Layton -Signed-off-by: Chuck Lever -Signed-off-by: Sasha Levin ---- - 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 07ea358036293..316cb64e15e97 100644 ---- a/fs/nfsd/nfs4state.c -+++ b/fs/nfsd/nfs4state.c -@@ -4703,10 +4703,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 ret; - } - -@@ -5195,12 +5193,13 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, - if (status) - goto out_unlock; - -+ status = -EAGAIN; -+ if (fp->fi_had_conflict) -+ goto out_unlock; -+ - spin_lock(&state_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(&state_lock); - --- -2.43.0 - diff --git a/queue-5.15/series b/queue-5.15/series index c0da96d22cd..28372652949 100644 --- a/queue-5.15/series +++ b/queue-5.15/series @@ -79,4 +79,3 @@ regmap-allow-to-define-reg_update_bits-for-no-bus-co.patch regmap-add-bulk-read-write-callbacks-into-regmap_con.patch serial-max310x-make-accessing-revision-id-interface-.patch serial-max310x-fix-io-data-corruption-in-batched-ope.patch -nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch diff --git a/queue-5.4/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch b/queue-5.4/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch deleted file mode 100644 index ad8cf6e187a..00000000000 --- a/queue-5.4/nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch +++ /dev/null @@ -1,97 +0,0 @@ -From 45b3428bc3d461644256799e2d05af43a3e5f67d Mon Sep 17 00:00:00 2001 -From: Sasha Levin -Date: Mon, 5 Feb 2024 13:22:39 +1100 -Subject: nfsd: don't take fi_lock in nfsd_break_deleg_cb() - -From: NeilBrown - -[ Upstream commit 5ea9a7c5fe4149f165f0e3b624fe08df02b6c301 ] - -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 -Reviewed-by: Jeff Layton -Signed-off-by: Chuck Lever -Signed-off-by: Sasha Levin ---- - 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 bca22325083c0..c479c45892075 100644 ---- a/fs/nfsd/nfs4state.c -+++ b/fs/nfsd/nfs4state.c -@@ -4506,10 +4506,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 ret; - } - -@@ -4907,12 +4905,13 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, - if (status) - goto out_clnt_odstate; - -+ status = -EAGAIN; -+ if (fp->fi_had_conflict) -+ goto out_unlock; -+ - spin_lock(&state_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(&state_lock); - --- -2.43.0 - diff --git a/queue-5.4/series b/queue-5.4/series index 44083829a60..770e7d64693 100644 --- a/queue-5.4/series +++ b/queue-5.4/series @@ -45,7 +45,6 @@ getrusage-use-__for_each_thread.patch getrusage-use-sig-stats_lock-rather-than-lock_task_s.patch exit-fix-typo-in-comment-s-sub-theads-sub-threads.patch exit-wait_task_zombie-kill-the-no-longer-necessary-s.patch -nfsd-don-t-take-fi_lock-in-nfsd_break_deleg_cb.patch serial-max310x-unprepare-and-disable-clock-in-error-.patch regmap-allow-to-define-reg_update_bits-for-no-bus-co.patch regmap-add-bulk-read-write-callbacks-into-regmap_con.patch -- 2.47.2