Stephen Brennan <stephen.s.brennan@xxxxxxxxxx> writes: > Hi Jan, Amir, Al, > > Here's my v4 patch series that aims to eliminate soft lockups when updating > dentry flags in fsnotify. I've incorporated Jan's suggestion of simply > allowing the flag to be lazily cleared in the fsnotify_parent() function, > via Amir's patch. This allowed me to drop patch #2 from my previous series > (fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem). I > replaced it with "fsnotify: require inode lock held during child flag > update", patch #5 in this series. I also added "dnotify: move > fsnotify_recalc_mask() outside spinlock" to address the sleep-during-atomic > issues with dnotify. > > Jan expressed concerns about lock ordering of the inode rwsem with the > fsnotify group mutex. I built this with lockdep enabled (see below for the > lock debugging .config section -- I'm not too familiar with lockdep so I > wanted a sanity check). I ran all the fanotify, inotify, and dnotify tests > I could find in LTP, with no lockdep splats to be found. I don't know that > this can completely satisfy the concerns about lock ordering: I'm reading > through the code to better understand the concern about "the removal of > oneshot mark during modify event generation". But I'm encouraged by the > LTP+lockdep results. Of course, I forgot to append the .config section: # # Lock Debugging (spinlocks, mutexes, etc...) # CONFIG_LOCK_DEBUGGING_SUPPORT=y CONFIG_PROVE_LOCKING=y # CONFIG_PROVE_RAW_LOCK_NESTING is not set CONFIG_LOCK_STAT=y CONFIG_DEBUG_RT_MUTEXES=y CONFIG_DEBUG_SPINLOCK=y CONFIG_DEBUG_MUTEXES=y CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y CONFIG_DEBUG_RWSEMS=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_LOCKDEP=y CONFIG_LOCKDEP_BITS=15 CONFIG_LOCKDEP_CHAINS_BITS=16 CONFIG_LOCKDEP_STACK_TRACE_BITS=19 CONFIG_LOCKDEP_STACK_TRACE_HASH_BITS=14 CONFIG_LOCKDEP_CIRCULAR_QUEUE_BITS=12 CONFIG_DEBUG_LOCKDEP=y CONFIG_DEBUG_ATOMIC_SLEEP=y # CONFIG_DEBUG_LOCKING_API_SELFTESTS is not set # CONFIG_LOCK_TORTURE_TEST is not set # CONFIG_WW_MUTEX_SELFTEST is not set # CONFIG_SCF_TORTURE_TEST is not set CONFIG_CSD_LOCK_WAIT_DEBUG=y # end of Lock Debugging (spinlocks, mutexes, etc...) > > I also went ahead and did my negative dentry oriented testing. Of course > the fsnotify_parent() issue is fully resolved, and when I tested several > processes all using inotifywait on the same directory full of negative > dentries, I was able to use ftrace to confirm that > fsnotify_update_children_dentry_flags() was called exactly once for all > processes. No softlockups occurred! > > I originally wrote this series to make the last patch (#5) optional: if for > some reason we didn't think it was necessary to hold the inode rwsem, then > we could omit it -- the main penalty being the race condition described in > the patch description. I tested without the last patch and LTP passed also > with lockdep enabled, but of course when multiple tasks did an inotifywait > on the same directory (with many negative dentries) only the first waited > for the flag updates, the rest of the tasks immediately returned despite > the flags not being ready. > > I agree with Amir that as long as the lock ordering is fine, we should keep > patch #5. And if that's the case, I can reorder the series a bit to make it > a bit more logical, and eliminate logic in > fsnotify_update_children_dentry_flags() for handling d_move/cursor races, > which I promptly delete later in the series. > > 1. fsnotify: clear PARENT_WATCHED flags lazily > 2. fsnotify: Use d_find_any_alias to get dentry associated with inode > 3. dnotify: move fsnotify_recalc_mask() outside spinlock > 4. fsnotify: require inode lock held during child flag update > 5. fsnotify: allow sleepable child flag update > > Thanks for continuing to read this series, I hope we're making progress > toward a simpler way to fix these scaling issues! > > Stephen > > Amir Goldstein (1): > fsnotify: clear PARENT_WATCHED flags lazily > > Stephen Brennan (4): > fsnotify: Use d_find_any_alias to get dentry associated with inode > dnotify: move fsnotify_recalc_mask() outside spinlock > fsnotify: allow sleepable child flag update > fsnotify: require inode lock held during child flag update > > fs/notify/dnotify/dnotify.c | 28 ++++++--- > fs/notify/fsnotify.c | 101 ++++++++++++++++++++++--------- > fs/notify/fsnotify.h | 3 +- > fs/notify/mark.c | 40 +++++++++++- > include/linux/fsnotify_backend.h | 8 ++- > 5 files changed, 136 insertions(+), 44 deletions(-) > > -- > 2.34.1
