On Tue, May 7, 2019 at 7:19 PM Jan Kara <jack@xxxxxxx> wrote: > > On Sun 05-05-19 23:07:28, Amir Goldstein wrote: > > __fsnotify_parent() has an optimization in place to avoid unneeded > > take_dentry_name_snapshot(). When fsnotify_nameremove() was changed > > not to call __fsnotify_parent(), we left out the optimization. > > Kernel test robot reported a 5% performance regression in concurrent > > unlink() workload. > > > > Modify __fsnotify_parent() so that it can be called also to report > > directory modififcation events and use it from fsnotify_nameremove(). > > > > Reported-by: kernel test robot <rong.a.chen@xxxxxxxxx> > > Link: https://lore.kernel.org/lkml/20190505062153.GG29809@shao2-debian/ > > Link: https://lore.kernel.org/linux-fsdevel/20190104090357.GD22409@xxxxxxxxxxxxxx/ > > Fixes: 5f02a8776384 ("fsnotify: annotate directory entry modification events") > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > Jan, > > > > A nicer approach reusing __fsnotify_parent() instead of copying code > > from it. > > > > This version does not apply cleanly to Al's for-next branch (there are > > some fsnotify changes in work.dcache). The conflict is trivial and > > resolved on my fsnotify branch [1]. > > Hum, let me check if I understand the situation right. We've changed > fsnotify_nameremove() to not use fsnotify_parent() as we don't want to > report FS_EVENT_ON_CHILD with it anymore. We should use fsnotify_dirent() > as for all other directory event notification handlers but that's > problematic due to different locking context and possible instability of > parent. > Yes. Not only do we not want to report FS_EVENT_ON_CHILD with FS_DELETE, but we also need to report it to dir inode and to fs sb regardless of DCACHE_FSNOTIFY_PARENT_WATCHED. > Honestly I don't like the patch below much. How we are notifying parent > without sending FS_EVENT_ON_CHILD and modify behavior based on that flag > just looks subtle. I see, although please note that reporting FS_EVENT_ON_CHILD is strongly related to the "modified behavior", because unless this an a report of event on_child, DCACHE_FSNOTIFY_PARENT_WATCHED is not relevant. > So I'd rather move the fsnotify call to vfs_unlink(), > vfs_rmdir(), simple_unlink(), simple_rmdir(), and then those few callers of > d_delete() that remain as you suggest elsewhere in this thread. And then we > get more consistent context for fsnotify_nameremove() and could just use > fsnotify_dirent(). > Yes, I much prefer this solution myself and I will follow up with it, but it would not be honest to suggest said solution as a stable fix to the performance regression that was introduced in v5.1. I think is it better if you choose between lesser evil: v1 with ifdef CONFIG_FSNOTIFY to fix build issue v2 as subtle as it is OR another obviously safe stable fix that you can think of The change of cleansing d_delete() from fsnotify_nameremove() requires more research and is anyway not stable tree material - if not for the level of complexity, then because all the users of FS_DELETE from pseudo and clustered filesystems need more time to find regressions (we do not have test coverage for those in LTP). Thanks, Amir.
