On Sun, 28 Jan 2024 14:07:49 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Sun, 28 Jan 2024 at 13:43, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > That's just wrong. > > > > Either you look things up under your own locks, in which case the SRCU > > dance is unnecessary and pointless. > > > > Or you use refcounts. > > > > In which case SRCU is also unnecessary and pointless. > > So from what I can see, you actually protect almost everything with > the eventfs_mutex, but the problem is that you then occasionally drop > that mutex in the middle. > > The one valid reason for dropping it is the readdir callback, which > does need to write to user space memory. > > But no, that's not a valid reason to use SRCU. It's a very *bad* > reason to use SRCU. > > The thing is, you can fix it two ways: > > - either refcount things properly, ie when you do that lookup under your lock: > > mutex_lock(&eventfs_mutex); > ei = READ_ONCE(ti->private); > if (ei && ei->is_freed) > ei = NULL; > mutex_unlock(&eventfs_mutex); > > you just go "I now have a ref" to the ei, and you increment the > refcount like you should, and then you dcrement it at the end when > you're done. > > Btw, what's with the READ_ONCE()? You have locking. > > The other option is to simply re-lookup the ei when you re-get the > eventfs_mutext anyway. > > Either of those cases, and the SRCU is entirely pointless. It really > looks wrong, because you seem to take that eventfs_mutex everywhere > anyway. The original code just used the mutex, but then we were hitting deadlocks because we used the mutex in the iput() logic. But this could have been due to the readdir logic causing the deadlocks. A lot of the design decisions were based on doing the dentry creation in the readdir code. Now that it's no longer there, I could go back and try taking the eventfs_mutex for the entirety of the lookup and see if lockdep complains again about also using it in the iput logic. Then yes, we can get rid of the SRCU as that was added as a way to get out of that deadlock. -- Steve
|