On Tue, May 7, 2019 at 10:12 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > 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). > Something like this: https://github.com/amir73il/linux/commits/fsnotify_nameremove Only partially tested. Obviously haven't tested all callers. Thanks, Amir.
