> > > > http://marc.info/?l=linux-fsdevel&m=123831685111213&w=2 > > > > Looking over it, the > > > > + unsigned int lock_held = mutex_is_locked(&dir->i_mutex); > > ... > > + if (lock_held) { > > + /* Already pending, send to ->lookup() */ > > + d_drop(dentry); > > > > bit looks highly suspect. I'm guessing revalidate should never sleep, and > > always kick things off to ->lookup() (to do any waiting on upcall > > completion or whatever else) if the dentry isn't valid now...? > > Yeah, I've heard that before, ;) > > And that maybe the case, but that was what I first had. > > Sending everything to ->lookup() might be possible but it certainly > isn't that simple. > > Waiting in ->d_revaidate() isn't that different to waiting in ->lookup() > anyway as that must always be done without the directory mutex held. If > the lock isn't held when in ->d_revalidate() I can't really see any > reason not to handle that right their, possibly preventing the need to > go to ->lookup(). IIRC, the fundamental problem was that if ->d_revalidate() can be called either with or without the dir mutex, you won't know whether the mutex is locked by the caller or by some other thread (mutex_is_locked() doesn't tell you _who_ locked the mutex). And if waiting in d_revalidate means unlocking the mutex, that can't work, because you won't know if that's safe. Thus, no waiting allowed in revalidate? > There are several cases I need to deal with, apart from path walks > initiated by the daemon which don't cause any call backs, and so are > largely handled by trivially returning success. The cases are, an > expiring dentry that will go away which ->lookup() can't yet handle, an > expiring dentry that won't go away which ->lookup() should be able to > handle already, and a straight out mount request which ->lookup() should > also be able to handle. The tail end of the expire cases can progress > concurrently with a mount, which is further complicated by the two cases > of going away or not, so it's all a bit tricky. It sounds to me like revalidate should only return success in the trivial, non-blocking case. And the ->lookup() should be able to handle all (other) cases. I.e., things should still work correctly (perhaps more slowly) without any d_revalidate() at all. (It still looks like the main change needed is for lookup to use d_obtain_alias, instead of returning NULL unconditionally...) > In any case I need to get this to work without the change you proposed, > except for cases that result from the locking change, and I'm using > printks to track incorrect returns to identify those cases. So what I > need right now is consistent behaviour and I'm not quite there. Once I > have that I'll work on any issues resulting from the locking change. I suspect you want to work with the real_lookup patch applied... I don't think you can get consistent behavior without it. The whole goal here is to remove the wait-in-revalidate workaround that was compensating for that bug; without either the fix or the workaround you'll get occasional -ENOENT. sage -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
