On 12/6/21 2:05 PM, Trond Myklebust wrote:
On Mon, 2021-12-06 at 12:36 -0800, dai.ngo@xxxxxxxxxx wrote:
On 12/6/21 12:05 PM, bfields@xxxxxxxxxxxx wrote:
On Mon, Dec 06, 2021 at 07:52:29PM +0000, Trond Myklebust wrote:
On Mon, 2021-12-06 at 18:39 +0000, Chuck Lever III wrote:
On Dec 6, 2021, at 12:59 PM, Dai Ngo <dai.ngo@xxxxxxxxxx>
wrote:
Add new callback, lm_expire_lock, to lock_manager_operations
to
allow
the lock manager to take appropriate action to resolve the
lock
conflict
if possible. The callback takes 2 arguments, file_lock of the
blocker
and a testonly flag:
testonly = 1 check and return true if lock conflict can be
resolved
else return false.
testonly = 0 resolve the conflict if possible, return true
if
conflict
was resolved esle return false.
Lock manager, such as NFSv4 courteous server, uses this
callback to
resolve conflict by destroying lock owner, or the NFSv4
courtesy
client
(client that has expired but allowed to maintains its states)
that
owns
the lock.
Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
Al, Jeff, as co-maintainers of record for fs/locks.c, can you
give
an Ack or Reviewed-by? I'd like to take this patch through the
nfsd
tree for v5.17. Thanks for your time!
---
fs/locks.c | 28 +++++++++++++++++++++++++---
include/linux/fs.h | 1 +
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 3d6fb4ae847b..0fef0a6322c7 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -954,6 +954,7 @@ posix_test_lock(struct file *filp, struct
file_lock *fl)
struct file_lock *cfl;
struct file_lock_context *ctx;
struct inode *inode = locks_inode(filp);
+ bool ret;
ctx = smp_load_acquire(&inode->i_flctx);
if (!ctx || list_empty_careful(&ctx->flc_posix)) {
@@ -962,11 +963,20 @@ posix_test_lock(struct file *filp,
struct
file_lock *fl)
}
spin_lock(&ctx->flc_lock);
+retry:
list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
- if (posix_locks_conflict(fl, cfl)) {
- locks_copy_conflock(fl, cfl);
- goto out;
+ if (!posix_locks_conflict(fl, cfl))
+ continue;
+ if (cfl->fl_lmops && cfl->fl_lmops-
lm_expire_lock
&&
+ cfl->fl_lmops-
lm_expire_lock(cfl,
1)) {
+ spin_unlock(&ctx->flc_lock);
+ ret = cfl->fl_lmops-
lm_expire_lock(cfl,
0);
+ spin_lock(&ctx->flc_lock);
+ if (ret)
+ goto retry;
}
+ locks_copy_conflock(fl, cfl);
How do you know 'cfl' still points to a valid object after you've
dropped the spin lock that was protecting the list?
Ugh, good point, I should have noticed that when I suggested this
approach....
Maybe the first call could instead return return some reference-
counted
object that a second call could wait on.
Better, maybe it could add itself to a list of such things and then
we
could do this in one pass.
I think we adjust this logic a little bit to cover race condition:
The 1st call to lm_expire_lock returns the client needs to be
expired.
Before we make the 2nd call, we save the 'lm_expire_lock' into a
local
variable then drop the spinlock, and use the local variable to make
the
2nd call so that we do not reference 'cfl'. The argument of the
second
is the opaque return value from the 1st call.
nfsd4_fl_expire_lock also needs some adjustment to support the above.
It's not just the fact that you're using 'cfl' in the actual call to
lm_expire_lock(), but you're also using it after retaking the spinlock.
I plan to do this:
checked_cfl = NULL;
spin_lock(&ctx->flc_lock);
retry:
list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
if (!posix_locks_conflict(fl, cfl))
continue;
if (check_cfl != cfg && cfl->fl_lmops && cfl->fl_lmops->lm_expire_lock) {
void *res_data;
res_data = cfl->fl_lmops->lm_expire_lock(cfl, 1);
if (res_data) {
func = cfl->fl_lmops->lm_expire_lock;
spin_unlock(&ctx->flc_lock);
func(res_data, 0);
spin_lock(&ctx->flc_lock);
checked_cfl = cfl;
goto retry;
}
locks_copy_conflock(fl, cfl);
goto out;
}
fl->fl_type = F_UNLCK;
Do you still see problems with this?
-Dai
