On Thu, May 16, 2019 at 3:25 PM Jan Kara <jack@xxxxxxx> wrote: > > Hi, > > On Thu 16-05-19 13:26:27, Amir Goldstein wrote: > > What do you think will be the best merge strategy for this patch series? > > > > How about sending first 3 prep patches to Linus for applying at the end > > of v5.2 merge window, so individual maintainers can pick up per fs > > patches for the v5.3 development cycle? > > I don't think we'll make it for rc1. But those three cleanup patches would > be OK for rc2 as well. But overall patches are not very intrusive so I'm > also OK with pushing the patches myself once we get acks from respective > maintainers if Al will be too busy and won't react. > > > The following d_delete() call sites have been audited and will no longer > > generate fsnotify event after this series: > > > > drivers/usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - cleanup? (*) > > Not really but creation events are not generated either. > > > fs/ceph/dir.c:ceph_unlink() - from vfs_unlink() > > fs/ceph/inode.c:ceph_fill_trace() - invalidate (**) > > There's one more 'invalidate' case in fs/ceph/inode.c. > > > fs/configfs/dir.c:detach_groups() - cleanup (*) > > Why is this a cleanup? detach_groups() is used also from > configfs_detach_group() which gets called from configfs_rmdir() which is > real deletion. True, configfs is a special case where both cleanup and real deletion use the same helper. configfs_detach_group() is either called for cleanup or from vfs_rmdir->configfs_rmdir()/configfs_unregister_{group,subsystem}() the latter 3 cases have new fsnotify hooks. > > > fs/configfs/dir.c:configfs_attach_item() - cleanup (*) > > fs/configfs/dir.c:configfs_attach_group() - cleanup (*) > > fs/efivarfs/file.c:efivarfs_file_write() - invalidate (**) > > fs/fuse/dir.c:fuse_reverse_inval_entry() - invalidate (**) > > fs/nfs/dir.c:nfs_dentry_handle_enoent() - invalidate (**) > > fs/nsfs.c:__ns_get_path() - invalidate (**) > > More a cleanup case I'd say... > > > fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate? (**) > > This is really invalidate. > > > fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode > > > > (*) There are 2 "cleanup" use cases: > > - Create;init;delte if init failed > > - Batch delete of files within dir before removing dir > > Both those cases are not interesting for users that wish to observe > > configuration changes on pseudo filesystems. Often, there is already > > an fsnotify event generated on the directory removal which is what > > users should find interesting, for example: > > configfs_unregister_{group,subsystem}(). > > > > (**) The different "invalidate" use cases differ, but they all share > > one thing in common - user is not guarantied to get an event with > > current kernel. For example, when a file is deleted remotely on > > nfs server, nfs client is not guarantied to get an fsnotify delete > > event. On current kernel, nfs client could generate an fsnotify > > delete event if the local entry happens to be in cache and client > > finds out that entry is deleted on server during another user > > operation. Incidentally, this group of use cases is where most of > > the call sites are with "unstable" d_name, which is the reason for > > this patch series to begin with. > > Thanks. Maybe for other reviewers it would be more convincing to take > sanitized output of 'git grep "[^a-z_]d_delete("' and annotate how each > callsite is handled - i.e., "cleanup, invalidate, simple_remove, vfs, > manual - patch X". But I'm now reasonably convinced we didn't forget > anything :). > > Honza > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR