Re: [Cluster-devel] [RFCv2 4/7] locks: update lock callback documentation

Hi,

On Wed, Aug 16, 2023 at 8:01 AM Jeff Layton <[email protected]> wrote:
>
> On Mon, 2023-08-14 at 17:11 -0400, Alexander Aring wrote:
> > This patch updates the existing documentation regarding recent changes
> > to vfs_lock_file() and lm_grant() is set. In case of lm_grant() is set
> > we only handle FILE_LOCK_DEFERRED in case of FL_SLEEP in fl_flags is not
> > set. This is the case of an blocking lock request. Non-blocking lock
> > requests, when FL_SLEEP is not set, are handled in a synchronized way.
> >
> > Signed-off-by: Alexander Aring <[email protected]>
> > ---
> >  fs/locks.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index df8b26a42524..a8e51f462b43 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2255,21 +2255,21 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
> >   * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
> >   * locks, the ->lock() interface may return asynchronously, before the lock has
> >   * been granted or denied by the underlying filesystem, if (and only if)
> > - * lm_grant is set. Callers expecting ->lock() to return asynchronously
> > - * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
> > - * the request is for a blocking lock. When ->lock() does return asynchronously,
> > - * it must return FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock
> > - * request completes.
> > - * If the request is for non-blocking lock the file system should return
> > - * FILE_LOCK_DEFERRED then try to get the lock and call the callback routine
> > - * with the result. If the request timed out the callback routine will return a
> > + * lm_grant and FL_SLEEP in fl_flags is set. Callers expecting ->lock() to return
> > + * asynchronously will only use F_SETLK, not F_SETLKW; When ->lock() does return
>
> Isn't the above backward? Shouldn't it say "Callers expecting ->lock()
> to return asynchronously will only use F_SETLKW, not F_SETLK" ?
>

So far I know lockd will always use F_SETLK only, if it's a blocking
or non-blocking request you need to evaluate FL_SLEEP. But if
lm_grant() is not set we are using a check on cmd if it's F_SETLK or
F_SETLKW to check if it's non-blocking or blocking.

If lm_grant() is set and checking on F_SETLKW should never be the
case, because it will never be true (speaking from lockd point of
view).

- Alex