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!
