[RFC PATCH 0/2] nfsd/lockd: v3/v4 lock conflict in presence of delegations

[RFC PATCH 0/2] nfsd/lockd: v3/v4 lock conflict in presence of delegations

[Olga Kornievskaia]

Hi folks,

I see that an NLM lock request when another client holds a delegation
triggers a delegation recall on the server. Is that normal?

Let's say it is normal. Such an NLM lock request fails with
"nlm_failed". Alternatively, if another client doesn't have a
delegation but holds a lock, NLM request gets nlm_blocked and gets a
callback. It seems incorrect that just holding a delegation would
prevent somebody from getting lock?

I believe the reason NLM v3 request (while there is a conflicting
delegation) fails is because break_lease returns -EAGAIN error and
nfsd_open calls nfserrno() which maps it into err_jukebox which gets
propagated to nlm_fopen() which maps any error (other than stale or
drop_it) to nlm_failed. Should nlm_fopen() have mapped jukebox error
into dropit (so that the client tries the lock again?).

Now, let me tie this to a server reboot. Why isn't nlm4svc_prov_lock()
checkin that it's in grace first thing before doing anything further?
When that v3 lock request arrives (during grace) when there is a v4
delegation given out, it prevents the server from triggering a
delegation recall (while in grace) and causing issues of failing the
lock. But I guess it's wasteful if there no conflicts.

Attached to this email disguised as a cover letter to a patch series
are 2 patches where I attempt to address the problem is a failing
NLM lock in presence of a v4 delegation.

I asked about NLM request triggering a delegation recall being normal
because during grace period, when v3 lock comes in and triggers a
delegation recall, the client sends local LOCK request (note it's no
a reclaim-lock because the client sent its RECLAIM_COMPLETE as it was
given a delegation), now this LOCK is failed with ERR_GRACE and the
client can't handle it. Be it a client issue for which I'm sending 
a separate patch. But it all revolves around the issue of whether
or not delegation recalls can happen during grace.

Olga Kornievskaia (2):
  nfsd: nfserr_jukebox in nlm_fopen should lead to a retry
  lockd: while grace prefer to fail with nlm_lck_denied_grace_period

 fs/lockd/svc4proc.c | 15 +++++++++++++--
 fs/nfsd/lockd.c     |  1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.47.1

[RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Olga Kornievskaia]

When v3 NLM request finds a conflicting delegation, it triggers
a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
of returning nlm_failed for when there is a conflicting delegation,
drop this NLM request so that the client retries. Once delegation
is recalled and if a local lock is claimed, a retry would lead to
nfsd returning a nlm_lck_blocked error or a successful nlm lock.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfsd/lockd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
index edc9f75dc75c..ad3e461f30c0 100644
--- a/fs/nfsd/lockd.c
+++ b/fs/nfsd/lockd.c
@@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
 	switch (nfserr) {
 	case nfs_ok:
 		return 0;
+	case nfserr_jukebox:
 	case nfserr_dropit:
 		return nlm_drop_reply;
 	case nfserr_stale:
-- 
2.47.1

[RFC PATCH 2/2] lockd: while grace prefer to fail with nlm_lck_denied_grace_period

[Olga Kornievskaia]

When nfsd is in grace and receives an NLM LOCK request which turns
out to have a conflicting delegation, return that the server is in
grace.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/lockd/svc4proc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 109e5caae8c7..7ac4af5c9875 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -141,8 +141,19 @@ __nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_res *resp)
 	resp->cookie = argp->cookie;
 
 	/* Obtain client and file */
-	if ((resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file)))
-		return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
+	resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file);
+	switch (resp->status) {
+	case 0:
+		break;
+	case nlm_drop_reply:
+		if (locks_in_grace(SVC_NET(rqstp))) {
+			resp->status = nlm_lck_denied_grace_period;
+			return rpc_success;
+		}
+		return nlm_drop_reply;
+	default:
+		return rpc_success;
+	}
 
 	/* Now try to lock the file */
 	resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
-- 
2.47.1

Re: [RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Jeff Layton]

On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> When v3 NLM request finds a conflicting delegation, it triggers
> a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
> then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
> of returning nlm_failed for when there is a conflicting delegation,
> drop this NLM request so that the client retries. Once delegation
> is recalled and if a local lock is claimed, a retry would lead to
> nfsd returning a nlm_lck_blocked error or a successful nlm lock.
> 
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/nfsd/lockd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> index edc9f75dc75c..ad3e461f30c0 100644
> --- a/fs/nfsd/lockd.c
> +++ b/fs/nfsd/lockd.c
> @@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
>  	switch (nfserr) {
>  	case nfs_ok:
>  		return 0;
> +	case nfserr_jukebox:
>  	case nfserr_dropit:
>  		return nlm_drop_reply;
>  	case nfserr_stale:

This works by triggering a RPC retransmission. That could time out on
soft mounts if it takes a while to return a delegation. Looking at the
NLM spec here:

    https://pubs.opengroup.org/onlinepubs/9629799/chap14.htm

What about returning NLM4_DENIED instead? The description there is:
     
NLM4_DENIED
    The call failed. For attempts to set a lock, this status implies
that if the client retries the call later, it may succeed. 

Presumably the client should redrive this effectively indefinitely that
way?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 2/2] lockd: while grace prefer to fail with nlm_lck_denied_grace_period

[Jeff Layton]

On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> When nfsd is in grace and receives an NLM LOCK request which turns
> out to have a conflicting delegation, return that the server is in
> grace.
> 
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/lockd/svc4proc.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 109e5caae8c7..7ac4af5c9875 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -141,8 +141,19 @@ __nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_res *resp)
>  	resp->cookie = argp->cookie;
>  
>  	/* Obtain client and file */
> -	if ((resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file)))
> -		return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
> +	resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file);
> +	switch (resp->status) {
> +	case 0:
> +		break;
> +	case nlm_drop_reply:
> +		if (locks_in_grace(SVC_NET(rqstp))) {
> +			resp->status = nlm_lck_denied_grace_period;
> +			return rpc_success;
> +		}
> +		return nlm_drop_reply;
> +	default:
> +		return rpc_success;
> +	}
>  
>  	/* Now try to lock the file */
>  	resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,

ACK to returning the right error code in this case, but you may want to
do this differently if you agree with me on patch #1.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Jeff Layton]

On Mon, 2025-08-11 at 15:10 -0400, Jeff Layton wrote:
> On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> > When v3 NLM request finds a conflicting delegation, it triggers
> > a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
> > then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
> > of returning nlm_failed for when there is a conflicting delegation,
> > drop this NLM request so that the client retries. Once delegation
> > is recalled and if a local lock is claimed, a retry would lead to
> > nfsd returning a nlm_lck_blocked error or a successful nlm lock.
> > 
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  fs/nfsd/lockd.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > index edc9f75dc75c..ad3e461f30c0 100644
> > --- a/fs/nfsd/lockd.c
> > +++ b/fs/nfsd/lockd.c
> > @@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> >  	switch (nfserr) {
> >  	case nfs_ok:
> >  		return 0;
> > +	case nfserr_jukebox:
> >  	case nfserr_dropit:
> >  		return nlm_drop_reply;
> >  	case nfserr_stale:
> 
> This works by triggering a RPC retransmission. That could time out on
> soft mounts if it takes a while to return a delegation. Looking at the
> NLM spec here:
> 
>     https://pubs.opengroup.org/onlinepubs/9629799/chap14.htm
> 
> What about returning NLM4_DENIED instead? The description there is:
>      
> NLM4_DENIED
>     The call failed. For attempts to set a lock, this status implies
> that if the client retries the call later, it may succeed. 
> 
> Presumably the client should redrive this effectively indefinitely that
> way?

Scratch that idea. The client treats that as a fatal error in
nlmclnt_lock():

        /*
         * EAGAIN doesn't make sense for sleeping locks, and in some
         * cases NLM_LCK_DENIED is returned for a permanent error.  So
         * turn it into an ENOLCK.
         */
        if (resp->status == nlm_lck_denied && (flags & FL_SLEEP))
                status = -ENOLCK;
        else
                status = nlm_stat_to_errno(resp->status);


Dropping the call might be next best option then.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Olga Kornievskaia]

On Mon, Aug 11, 2025 at 3:10 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> > When v3 NLM request finds a conflicting delegation, it triggers
> > a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
> > then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
> > of returning nlm_failed for when there is a conflicting delegation,
> > drop this NLM request so that the client retries. Once delegation
> > is recalled and if a local lock is claimed, a retry would lead to
> > nfsd returning a nlm_lck_blocked error or a successful nlm lock.
> >
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  fs/nfsd/lockd.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > index edc9f75dc75c..ad3e461f30c0 100644
> > --- a/fs/nfsd/lockd.c
> > +++ b/fs/nfsd/lockd.c
> > @@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> >       switch (nfserr) {
> >       case nfs_ok:
> >               return 0;
> > +     case nfserr_jukebox:
> >       case nfserr_dropit:
> >               return nlm_drop_reply;
> >       case nfserr_stale:
>
> This works by triggering a RPC retransmission. That could time out on
> soft mounts if it takes a while to return a delegation. Looking at the
> NLM spec here:
>
>     https://pubs.opengroup.org/onlinepubs/9629799/chap14.htm
>
> What about returning NLM4_DENIED instead? The description there is:
>
> NLM4_DENIED
>     The call failed. For attempts to set a lock, this status implies
> that if the client retries the call later, it may succeed.
>
> Presumably the client should redrive this effectively indefinitely that
> way?

I have previously tried "denied" but that lead to client failing with
no lock just like from nlm_failed instead of retrying. I think only
blocked (or block_grace) leads to a retry.


> --
> Jeff Layton <jlayton@kernel.org>
>

Re: [RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Jeff Layton]

On Mon, 2025-08-11 at 15:32 -0400, Olga Kornievskaia wrote:
> On Mon, Aug 11, 2025 at 3:10 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> > > When v3 NLM request finds a conflicting delegation, it triggers
> > > a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
> > > then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
> > > of returning nlm_failed for when there is a conflicting delegation,
> > > drop this NLM request so that the client retries. Once delegation
> > > is recalled and if a local lock is claimed, a retry would lead to
> > > nfsd returning a nlm_lck_blocked error or a successful nlm lock.
> > > 
> > > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > > ---
> > >  fs/nfsd/lockd.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > > index edc9f75dc75c..ad3e461f30c0 100644
> > > --- a/fs/nfsd/lockd.c
> > > +++ b/fs/nfsd/lockd.c
> > > @@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> > >       switch (nfserr) {
> > >       case nfs_ok:
> > >               return 0;
> > > +     case nfserr_jukebox:
> > >       case nfserr_dropit:
> > >               return nlm_drop_reply;
> > >       case nfserr_stale:
> > 
> > This works by triggering a RPC retransmission. That could time out on
> > soft mounts if it takes a while to return a delegation. Looking at the
> > NLM spec here:
> > 
> >     https://pubs.opengroup.org/onlinepubs/9629799/chap14.htm
> > 
> > What about returning NLM4_DENIED instead? The description there is:
> > 
> > NLM4_DENIED
> >     The call failed. For attempts to set a lock, this status implies
> > that if the client retries the call later, it may succeed.
> > 
> > Presumably the client should redrive this effectively indefinitely that
> > way?
> 
> I have previously tried "denied" but that lead to client failing with
> no lock just like from nlm_failed instead of retrying. I think only
> blocked (or block_grace) leads to a retry.
> 

Thanks, I noticed after I sent the above.

I think that the ideal thing to do would be to treat this like a
blocked lock and try to call the client back when the delegation is
returned. That may be difficult to do properly though, since you won't
have a proper lock request to block on.

For now, this is a reasonable solution, but it might be good to have a
comment that explains why the other options were unacceptable.

You can add this to both patches:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH 1/2] nfsd: nfserr_jukebox in nlm_fopen should lead to a retry

[Chuck Lever]

On 8/11/25 3:56 PM, Jeff Layton wrote:
> On Mon, 2025-08-11 at 15:32 -0400, Olga Kornievskaia wrote:
>> On Mon, Aug 11, 2025 at 3:10 PM Jeff Layton <jlayton@kernel.org> wrote:
>>>
>>> On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
>>>> When v3 NLM request finds a conflicting delegation, it triggers
>>>> a delegation recall and nfsd_open fails with EAGAIN. nfsd_open
>>>> then translates EAGAIN into nfserr_jukebox. In nlm_fopen, instead
>>>> of returning nlm_failed for when there is a conflicting delegation,
>>>> drop this NLM request so that the client retries. Once delegation
>>>> is recalled and if a local lock is claimed, a retry would lead to
>>>> nfsd returning a nlm_lck_blocked error or a successful nlm lock.
>>>>
>>>> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>>> ---
>>>>  fs/nfsd/lockd.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
>>>> index edc9f75dc75c..ad3e461f30c0 100644
>>>> --- a/fs/nfsd/lockd.c
>>>> +++ b/fs/nfsd/lockd.c
>>>> @@ -57,6 +57,7 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
>>>>       switch (nfserr) {
>>>>       case nfs_ok:
>>>>               return 0;
>>>> +     case nfserr_jukebox:
>>>>       case nfserr_dropit:
>>>>               return nlm_drop_reply;
>>>>       case nfserr_stale:
>>>
>>> This works by triggering a RPC retransmission. That could time out on
>>> soft mounts if it takes a while to return a delegation. Looking at the
>>> NLM spec here:
>>>
>>>     https://pubs.opengroup.org/onlinepubs/9629799/chap14.htm
>>>
>>> What about returning NLM4_DENIED instead? The description there is:
>>>
>>> NLM4_DENIED
>>>     The call failed. For attempts to set a lock, this status implies
>>> that if the client retries the call later, it may succeed.
>>>
>>> Presumably the client should redrive this effectively indefinitely that
>>> way?
>>
>> I have previously tried "denied" but that lead to client failing with
>> no lock just like from nlm_failed instead of retrying. I think only
>> blocked (or block_grace) leads to a retry.
>>
> 
> Thanks, I noticed after I sent the above.
> 
> I think that the ideal thing to do would be to treat this like a
> blocked lock and try to call the client back when the delegation is
> returned. That may be difficult to do properly though, since you won't
> have a proper lock request to block on.
> 
> For now, this is a reasonable solution, but it might be good to have a
> comment that explains why the other options were unacceptable.

+1 -- either a comment in the code, or please add the explanation to the
patch description.


> You can add this to both patches:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>


-- 
Chuck Lever

Re: [RFC PATCH 2/2] lockd: while grace prefer to fail with nlm_lck_denied_grace_period

[Olga Kornievskaia]

On Mon, Aug 11, 2025 at 3:42 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2025-08-11 at 14:18 -0400, Olga Kornievskaia wrote:
> > When nfsd is in grace and receives an NLM LOCK request which turns
> > out to have a conflicting delegation, return that the server is in
> > grace.
> >
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  fs/lockd/svc4proc.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > index 109e5caae8c7..7ac4af5c9875 100644
> > --- a/fs/lockd/svc4proc.c
> > +++ b/fs/lockd/svc4proc.c
> > @@ -141,8 +141,19 @@ __nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_res *resp)
> >       resp->cookie = argp->cookie;
> >
> >       /* Obtain client and file */
> > -     if ((resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file)))
> > -             return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
> > +     resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file);
> > +     switch (resp->status) {
> > +     case 0:
> > +             break;
> > +     case nlm_drop_reply:
> > +             if (locks_in_grace(SVC_NET(rqstp))) {
> > +                     resp->status = nlm_lck_denied_grace_period;
> > +                     return rpc_success;
> > +             }
> > +             return nlm_drop_reply;
> > +     default:
> > +             return rpc_success;
> > +     }
> >
> >       /* Now try to lock the file */
> >       resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
>
> ACK to returning the right error code in this case, but you may want to
> do this differently if you agree with me on patch #1.

I'm not sure what is the suggestion from the patch#1. Unless the
suggestion is for the server to return "denied" and then also for the
client to implement handling that error by retrying? So I wonder how
would that go? Should the client retry indefinitely or how long should
be waiting to retry the request instead of failing...

And also I'm somewhat uncomfortable in changing the client's
interpretation of the NLM's "denied" error code.


> --
> Jeff Layton <jlayton@kernel.org>
>