Re: [PATCH 1/2] nfsd4: complete enforcement of 4.1 op ordering

["J. Bruce Fields" <bfields@citi.umich.edu>]

On Tue, Apr 27, 2010 at 06:44:51PM +0300, Benny Halevy wrote:
> On Apr. 27, 2010, 18:01 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > I think what we actually want is a mechanism with semantics like yours,
> > but with multiple RENEW values so we can track how many users there are
> > and have only the last one do the renew.
> > 
> > One possibility: 
> > 
> > 	- add a cl_users field, with:
> > 		cl_user>0 meaning: somebody's using this client right
> > 			now, don't expire it.  (Your RENEW state.)
> > 		cl_user<0 meaning: this client is already being expired,
> > 			don't try to use it (or any sessionid or other
> > 			state associated with it). (Your EXPIRED state.)
> > 		cl_user==0: fair game to either use or (if expiry time
> > 			has passed) to expire. (Your NORMAL state.)
> > 
> > 	- add a cl_renewme boolean, meaning: last user of this client
> > 	  (user to decrement to 0) should renew the client (reset the
> > 	  expiry time and move it to the back of the lru).
> > 
> > So:
> > 
> > 	- in laundromat: 
> > 		- atomically check whether cl_users is 0, and, if so,
> > 		  decrement to -1.  (If positive, skip expiring this
> > 		  client.)
> > 
> > 	- on looking up a sessionid (or, eventually, any state object
> > 	  associated with a client), call get_client(), which:
> > 		- atomically checks whether cl_users is >=0, and, if so,
> > 		  increments it.  If <0, fail to find the object and
> > 		  return an appropriate error (STALE?).
> > 	- on renew:
> > 		- BUG_ON(cl_user<=0)
> > 		- set cl_renewme
> > 	- on dropping reference to a sessionid (or, eventually, any
> > 	  state object associated with a client), call put_client(),
> > 	  which:
> > 		- atomically decrements cl_users, checks whether it hits
> > 		  zero, and (if so, and if cl_renewme set), renews the
> > 		  client.
> > 
> > One possible implementation: make cl_users atomic, add a per-client
> > spinlock, make the put_client() do an atomic_dec_and_lock(), etc.
> > 
> > --b.
> 
> Sounds good. I'll take a stab at it right away.

Great, thanks; let me know what I can do.

> (Funny that my original implementation uses a counter but IIRC
> I decided at the time it was too complicated but I agree it's much better)

You might also look at get_write_access/deny_write_access and friends as
an implementation of something similar.

--b.