On Mon, 2024-08-05 at 12:46 +0300, Sagi Grimberg wrote:
> Hey,
>
> I'm looking at the NFS client locking code, and it seems to me that it
> violates the spec
> by caching locks when holding a read delegation.
>
> From 18.10.4. IMPLEMENTATION
> --
> When a client holds an OPEN_DELEGATE_WRITE delegation, the client
> holding that delegation is assured that there are no opens by other
> clients. Thus, there can be no conflicting LOCK operations from such
> clients. Therefore, the client may be handling locking requests
> locally, without doing LOCK operations on the server. If it does
> that, it must be prepared to update the lock status on the server, by
> sending appropriate LOCK and LOCKU operations before returning the
> delegation.
>
> When one or more clients hold OPEN_DELEGATE_READ delegations, any
> LOCK operation where the server is implementing mandatory locking
> semantics MUST result in the recall of all such delegations. The
> LOCK operation may not be granted until all such delegations are
> returned or revoked. Except where this happens very quickly, one or
> more NFS4ERR_DELAY errors will be returned to requests made while the
> delegation remains outstanding.
> --
>
> I don't see how the second paragraph can be met if the client caches locks.
> I've added a set of changes to address this [1] (untested, its designed
> to illustrate the point).
>
> Any thoughts on this?
>
I don't think this is a bug.
First, we only implement advisory locking in the client. The server may
enforce mandatory locking, but it's up to the server to mediate that
properly by recalling delegations at the right time. The client only
cares that it got a delegation when it comes to locking.
Only one client can hold a write delegation, so the real question is
this: can two clients caching locks under read delegations set locks
that conflict with one another?
The answer is no. The protocol does not allow you to set write locks on
SHARE_ACCESS_READ stateids. In the case where we open SHARE_ACCESS_BOTH
and get back a read delegation, we _know_ that nothing else can hold a
read delegation on the file and be caching locks because we have the
file open for write.
> [1]
> --
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ce48e26dc825..290b7ff1cce0 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7444,7 +7444,7 @@ static int nfs4_lock_reclaim(struct nfs4_state
> *state, struct file_lock *request
>
> do {
> /* Cache the lock if possible... */
> - if (test_bit(NFS_DELEGATED_STATE, &state->flags) != 0)
> + if (nfs_have_write_delegation(state->inode))
> return 0;
> err = _nfs4_do_setlk(state, F_SETLK, request,
> NFS_LOCK_RECLAIM);
> if (err != -NFS4ERR_DELAY)
> @@ -7470,7 +7470,7 @@ static int nfs4_lock_expired(struct nfs4_state
> *state, struct file_lock *request
> return 0;
> }
> do {
> - if (test_bit(NFS_DELEGATED_STATE, &state->flags) != 0)
> + if (nfs_have_write_delegation(state->inode))
> return 0;
> err = _nfs4_do_setlk(state, F_SETLK, request,
> NFS_LOCK_EXPIRED);
> switch (err) {
> @@ -7516,7 +7516,7 @@ static int _nfs4_proc_setlk(struct nfs4_state
> *state, int cmd, struct file_lock
> goto out;
> mutex_lock(&sp->so_delegreturn_mutex);
> down_read(&nfsi->rwsem);
> - if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
> + if (nfs_have_write_delegation(state->inode)) {
> /* Yes: cache locks! */
> /* ...but avoid races with delegation recall... */
> request->c.flc_flags = flags & ~FL_SLEEP;
> --
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
