On Fri, May 10, 2024 at 6:21 PM Stephen Brennan <stephen.s.brennan@xxxxxxxxxx> wrote: > > Hi Amir, Jan, et al, Hi Stephen, > > It's been a while since I worked with you on the patch series[1] that aimed to > make __fsnotify_update_child_dentry_flags() a sleepable function. That work got > to a point that it was close to ready, but there were some locking issues which > Jan found, and the kernel test robot reported, and I didn't find myself able to > tackle them in the amount of time I had. > > But looking back on that series, I think I threw out the baby with the > bathwater. While I may not have resolved the locking issues associated with the > larger change, there was one patch which Amir shared, that probably resolves > more than 90% of the issues that people may see. I'm sending that here, since it > still applies to the latest master branch, and I think it's a very good idea. > > To refresh you, the underlying issue I was trying to resolve was when > directories have many dentries (frequently, a ton of negative dentries), the > __fsnotify_update_child_dentry_flags() operation can take a while, and it > happens under spinlock. > > Case #1 - if the directory has tens of millions of dentries, then you could get > a soft lockup from a single call to this function. I have seen some cases where > a single directory had this many dentries, but it's pretty rare. > > Case #2 - suppose you have a system with many CPUs and a busy directory. Suppose > the directory watch is removed. The caller will begin executing > __fsnotify_update_child_dentry_flags() to clear the PARENT_WATCHED flag, but in > parallel, many other CPUs could wind up in __fsnotify_parent() and decide that > they, too, must call __fsnotify_update_child_dentry_flags() to clear the flags. > These CPUs will all spin waiting their turn, at which point they'll re-do the > long (and likely, useless) call. Even if the original call only took a second or > two, if you have a dozen or so CPUs that end up in that call, some CPUs will > spin a long time. > > Amir's patch to clear PARENT_WATCHED flags lazily resolves that easily. In > __fsnotify_parent(), if callers notice that the parent is no longer watching, > they merely update the flags for the current dentry (not all the other > children). The __fsnotify_recalc_mask() function further avoids excess calls by > only updating children if the parent started watching. This easily handles case > #2 above. Perhaps case #1 could still cause issues, for the cases of truly huge > dentry counts, but we shouldn't let "perfect" get in the way of "good enough" :) > The story sounds good :) Only thing I am worried about is: was case #2 tested to prove that the patch really imploves in practice and not only in theory? I am not asking that you write a test for this or even a reproducer just evidence that you collected from a case where improvement is observed and measurable. Thanks, Amir. > [1]: https://lore.kernel.org/all/20221013222719.277923-1-stephen.s.brennan@xxxxxxxxxx/ > > Amir Goldstein (1): > fsnotify: clear PARENT_WATCHED flags lazily > > fs/notify/fsnotify.c | 26 ++++++++++++++++++++------ > fs/notify/fsnotify.h | 3 ++- > fs/notify/mark.c | 32 +++++++++++++++++++++++++++++--- > include/linux/fsnotify_backend.h | 8 +++++--- > 4 files changed, 56 insertions(+), 13 deletions(-) > > -- > 2.43.0 >
