On Sun, May 5, 2019 at 4:05 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Sun, May 05, 2019 at 12:15:49PM +0300, 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. > > > > 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, > > > > The linked 5.1-rc1 performance regression report came with bad timing. > > Not sure if Linus is planning an rc8. If not, you will probably not > > see this before the 5.1 release and we shall have to queue it for 5.2 > > and backport to stable 5.1. > > > > I crafted the patch so it applies cleanly both to master and Al's > > for-next branch (there are some fsnotify changes in work.dcache). > > Charming... What about rename() and matching regressions there? rename() and create() do not take_dentry_name_snapshot(), because they are called with parent inode lock held. I have made an analysis of callers to d_delete() and found that all callers either hold parent inode lock or name is stable for another reason: https://lore.kernel.org/linux-fsdevel/20190104090357.GD22409@xxxxxxxxxxxxxx/ But Jan preferred to keep take_dentry_name_snapshot() to be safe. I think the right thing to do is assert that parent inode is locked or no rename op in d_delete() and take the lock in ceph/ocfs2 to conform to the standard. If that sounds good to you, I will follow up with a patch for next and then remove take_dentry_name_snapshot() from fsnotify_nameremove() hook. Thanks, Amir.
