On Mon, May 6, 2019 at 5:26 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > On Mon, May 06, 2019 at 03:43:24PM +0300, Amir Goldstein wrote: > > 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? > > For starters, d_splice_alias() picking an exising alias for given directory > inode. Ok. But seeing that we are already in d_delete() said directory is already IS_DEADDIR, so that can be added to the assertion that proves the stability of d_name. Are there any other cases? weird filesystems? > > > 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()? > > *snort* > > You can thank those who whine about notifications on sysfs/devpts/whatnot. > Go talk to them if you wish, but don't ask me to translate what you'll get > into something coherent - I'd never been able to. I see. So all of those fs that are interested in notifications already have fsnotify_create()/fsnotify_move() calls in them. There are only 5 of them: binderfs, debugfs, devpts, tracefs, sunrpc. It would be easier and less convoluted to also add the fsnotify_nameremove() explicit calls in those fs. With those fs either d_name is inherently stable or (debugfs) locks on parent are taken properly. But d_delete() effectively provides something else. If provides "remote server change notifications" for clustered/networking filesystems when server invalidates the local dentry. This is not a feature that is guarantied by fsnotify. notifications will be delivered randomly based on existence of local entry in dcache. Only networking fs that provide proper remote notifications (cifs is interested in doing that) should really be calling the fsnofity hooks. Of course, we remove the random remote notifications from clustered/network fs, there is bound to be someone unhappy. Urgh! out of those fs, only ocfs2 calls fsnotify_create(). If it is deemed important, can add fsnotify_nameremove() to ocfs2 as well. Dare we make this change and see if people shout? It's easy to add the random remote fsnotify_nameremove() calls per request for a filesystems that want it. Thanks, Amir.
