On Tue, May 11, 2010 at 10:39:25AM -0400, J. Bruce Fields wrote: > On Tue, May 11, 2010 at 10:27:27AM +0300, Benny Halevy wrote: > > On May. 10, 2010, 22:01 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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 (Sorry, I meant to say "would give the client the right to expect...", or words to that effect.) --b. > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
