Re: [PATCH RFC v6 1/2] fs/lock: add new callback, lm_expire_lock, to lock_manager_operations

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2021-12-06 at 12:36 -0800, dai.ngo@oracle.com wrote:
> 
> On 12/6/21 12:05 PM, bfields@fieldses.org 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@oracle.com>
> > > > > 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@oracle.com>
> > > > 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.


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com