Re: [PATCH 0/8] nfsd4: keep the client from expiring while in use by nfs41 compounds

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

On Tue, May 11, 2010 at 10:27:27AM +0300, Benny Halevy wrote:
> On May. 10, 2010, 22:01 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> > On Mon, May 10, 2010 at 05:15:43PM +0300, Benny Halevy wrote:
> >> On May. 09, 2010, 19:55 +0300, "J. Bruce Fields" <bfields@citi.umich.edu> wrote:
> >>> On Sun, May 09, 2010 at 09:30:31AM +0300, Benny Halevy wrote:
> >>>> Correct.  The intentions are:
> >>>> 1. Make the laundromat process ignore clients that are in
> >>>> use by a 4.1 session.
> >>>> 2. Renew the client when the compound ends, rather than when it begins.
> >>>> 3. Unhash the client when it's expired explicitly but don't destroy it
> >>>> until there's no reference to it.
> >>> OK.  My one slight worry there is to make sure that code getting a
> >>> pointer to a client through a sessionid won't inadvertently assume it's
> >>> still hashed.
> >> That's a good point.
> >> In fact, the client should not be renewed when the compound is done
> >> if it was already explicitly expired.
> > 
> > Hm, and we've still got a lot of renew_client()'s sprinkled around that
> > will try to add the expired client back to client_lru.
> > 
> 
> For that I've already have a fix in my branch to not renew the client
> if it's marked as expired (cl_time == 0).

OK.

> >> Another way to deal with this which may be safer but is less optimal
> >> is to keep only the sessionid and look it up on each use. Then, using
> >> it while holding the state lock will make sure it's valid when used.
> > 
> > That seems overkill.  Instead of making ops look up the sessionid from
> > state each time I guess we could have a revalidate_sessionid() that
> > checked the associated client to see if it was still good.
> 
> Yeah.  Let me see if this is a quick fix and if so I'll send it as part
> of version 2 of this patchset, otherwise I'll send it as is.
> 
> > If we continue to reduce the scope of the state lock, isn't that going
> > to be a pain, though?  Will we end up having to do that sort of
> > revalidation every time we drop the state lock and reacquire it?
> 
> It's very little pain, just verifying that cl_time != 0.

It's not the trouble of the revalidation I worry about, so much as a)
remembering to do it every time, and b) always being in a position to
return an error if it fails.

For now it doesn't seem like a problem, so OK.  We can always reconsider
if it turns out to be.

> > Perhaps the simplest would be to make the clientid-destroyer wait; it
> > could set some sort of CLIENTID_DEAD flag on the client, wait for a
> > reference count to go to zero, then destroy the client.
> > 
> 
> I'm not sure about this.  Setting this flag is equivalent
> to what I propose to do today (with marking the client as expired AND
> unhashing it) as you want to prevent any more references to it.
> 
> The question is more about the semantics of the operation that explicitly
> destroys the old client, e.g. EXCHANGE_ID or DESTROY_SESSION.
> Can the client tolerate responses for the old clientid/sessionid
> after the client-destroying operation has succeeded?

I can't see what would give the client to expect any defined behavior
for a compound executed concurrently with an explicit destruction of the
associated clientid.

> > Then other code would be guaranteed that nothing will change underneath
> > them as long as they hold either the state lock or a reference to a
> > session.
> > 
> > So hopefully we'd only need worry about client shutdown in well-defined
> > places:
> > 	- when put()'ing a session, to check whether the client
> > 	  is ready to be destroyed now.
> 
> That's in v2.
> 
> > 	- when looking up a session, in which case we should check
> > 	  whether the client is dead and fail the lookup?
> 
> If the client is dead we won't find the sessionid as we unhash the session
> structure atomically with the client so that isn't an issue so that's also
> dealt with in v2.

OK, thanks.

--b.