| Message ID | 6378987a-e289-4173-9f09-093ba50ee75f@grimberg.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Question about NFS client locks and delegations | expand |
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; > -- >
On 05/08/2024 15:39, Jeff Layton wrote: > 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. Ah, this last part is the subtle part I was missing. Meaning that no 2 clients can hold a read delegation and SHARE_ACCESS_WRITE/SHARE_ACCESS_BOTH opens at the same time. I revisited the spec and it reinforces this in section 18.16.4 and the text reinforces this. IIRC it differs from SMB where leases are recalled only when a WRITE comes along that is considered a conflict, not the initially at OPEN time. A code comment would have been useful here... Thanks for the explanation Jeff!
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,
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? [1] -- 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; --