On Sun, May 5, 2019 at 4:47 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Sun, May 05, 2019 at 04:19:02PM +0300, Amir Goldstein wrote: > > > 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. > > Any messing with the locking in ceph_fill_trace() would have to come > with very detailed proof of correctness, convincingly stable wrt > future changes in ceph... OK. What do you have to say about this statement? Because fsnotify_nameremove() is called from d_delete() with negative or unhashed dentry, d_move() is not expected on this dentry, so it is safe to use d_parent/d_name without take_dentry_name_snapshot(). I assume it is not correct, but cannot figure out why. Under what circumstances is d_move() expected to move an unhashed dentry and hash it? If this is not expected then wouldn't we be better off with: @@ -2774,9 +2774,9 @@ static void __d_move(struct dentry *dentry, struct dentry *target, /* unhash both */ - if (!d_unhashed(dentry)) + if (!WARN_ON(d_unhashed(dentry))) ___d_drop(dentry); - if (!d_unhashed(target)) + if (!WARN_ON(d_unhashed(target))) ___d_drop(target); And then we can get rid of take_dentry_name_snapshot() in fsnotify_nameremove() and leave an assertion instead: + /* Proof of stability of d_parent and d_name */ + WARN_ON_ONCE(d_inode(dentry) && !d_unhashed(dentry)); + My other thought is why is fsnotify_nameremove() in d_delete() and not in vfs_unlink()/vfs_rmdir() under parent inode lock like the rest of the fsnotify_create/fsnotify_move hooks? In what case would we need the fsnotify event that is not coming from vfs_unlink()/vfs_rmdir()? Thanks, Amir.
