Hi Stephen! On Thu 27-10-22 17:10:13, Stephen Brennan wrote: > Here is v3 of the patch series. I've taken all of the feedback, > thanks Amir, Christian, Hilf, et al. Differences are noted in each > patch. > > I caught an obvious and silly dentry reference leak: d_find_any_alias() > returns a reference, which I never called dput() on. With that change, I > no longer see the rpc_pipefs issue, but I do think I need more testing > and thinking through the third patch. Al, I'd love your feedback on that > one especially. > > Thanks, > Stephen > > Stephen Brennan (3): > fsnotify: Use d_find_any_alias to get dentry associated with inode > fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem > fsnotify: allow sleepable child flag update Thanks for the patches Stephen and I'm sorry for replying somewhat late. The first patch is a nobrainer. The other two patches ... complicate things somewhat more complicated than I'd like. I guess I can live with them if we don't find a better solution but I'd like to discuss a bit more about alternatives. So what would happen if we just clear DCACHE_FSNOTIFY_PARENT_WATCHED in __fsnotify_parent() for the dentry which triggered the event and does not have watched parent anymore and never bother with full children walk? I suppose your contention problems will be gone, we'll just pay the price of dget_parent() + fsnotify_inode_watches_children() for each child that falsely triggers instead of for only one. Maybe that's not too bad? After all any event upto this moment triggered this overhead as well... Am I missing something? AFAIU this would allow us to avoid the games with the new connector flag etc... We would probably still need to avoid softlockups when setting the flag DCACHE_FSNOTIFY_PARENT_WATCHED but that should be much simpler (we could use i_rwsem trick like you do). Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
