| Message ID | 20190526143411.11244-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Sort out fsnotify_nameremove() mess | expand |
On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote: > Jan, > > For v3 I went with a straight forward approach. > Filesystems that have fsnotify_{create,mkdir} hooks also get > explicit fsnotify_{unlink,rmdir} hooks. > > Hopefully, this approach is orthogonal to whatever changes Al is > planning for recursive tree remove code, because in many of the > cases, the hooks are added at the entry point for the recursive > tree remove. > > After looking closer at all the filesystems that were converted to > simple_remove in v2, I decided to exempt another 3 filesystems from > the fsnotify delete hooks: hypfs,qibfs and aafs. > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user > configuration change, but they do not generate create events, so it > is less likely that users depend on the delete events. > > That leaves configfs the only filesystem that gets the new delete hooks > even though it does not have create hooks. why doesn't configfs have create hooks? That's what userspace does in configfs, shouldn't it be notified about it? Keeping it "unequal" seems odd to me. thanks, greg k-h
On Mon, May 27, 2019 at 11:25 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote: > > Jan, > > > > For v3 I went with a straight forward approach. > > Filesystems that have fsnotify_{create,mkdir} hooks also get > > explicit fsnotify_{unlink,rmdir} hooks. > > > > Hopefully, this approach is orthogonal to whatever changes Al is > > planning for recursive tree remove code, because in many of the > > cases, the hooks are added at the entry point for the recursive > > tree remove. > > > > After looking closer at all the filesystems that were converted to > > simple_remove in v2, I decided to exempt another 3 filesystems from > > the fsnotify delete hooks: hypfs,qibfs and aafs. > > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user > > configuration change, but they do not generate create events, so it > > is less likely that users depend on the delete events. > > > > That leaves configfs the only filesystem that gets the new delete hooks > > even though it does not have create hooks. > > why doesn't configfs have create hooks? That's what userspace does in > configfs, shouldn't it be notified about it? Keeping it "unequal" seems > odd to me. > So it's not exactly that configfs has no create hooks at all. For "normal" filesystems mkdir (for example) is only possible by mkdir(2) syscall and there is create hook in vfs_mkdir(). The configfs (as well as debugfs/tracefs/etc), there are other code paths that create directories, namely: configfs_register_grup/subsystem(). Those code paths have explicit fsnotify_mkdir() hook in debugfs/tracefs/etc, but not in configfs. Why? because nobody put the hooks and no user complained. Should we add fsnotify_mkdir() hooks in configfs - probably yes. I can do it as followup, but this is not the purpose of this patch set. The purpose of this patch set (achieved in the last patch) is to simplify the implementation of the fsnotify delete hook. Today it is overly complicated by the fact that the hooks was placed in d_delete() and d_delete() is called from some code paths that have no business with fsnotify notifications at all. Once this patch set is done sprinkling fsnotify_rmdir/unlink() hooks in "proper" places, it removes the current fsnotify hook from d_delete(). d_delete() is called from configfs_unregister_group/subsystem(), so if we do not add fsnotify delete hooks to configfs we will regress the existing "unequal" behavior. Maybe there are no users depending on fsnotify delete notifications from configfs - I do not know. If we believe that is the case, we can drop the configfs patch. Maybe there *are* user depending on fsnotify delete notifications from aafs (apparmorfs), which I decided to exclude from v3 patch set. If we believe that is the case, or if we find out later that in case - no problem it is simple to add the missing fsnotify hooks in apparmorfs. Thought? About inclusion of configfs? About exclusion of apparmorfs? Again this is only exclusion/inclusion of hooks from code path that does NOT come from vfs syscalls (e.g. aa_remove_profiles()). Thanks, Amir.
On Mon, May 27, 2019 at 12:49:23PM +0300, Amir Goldstein wrote: > On Mon, May 27, 2019 at 11:25 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote: > > > Jan, > > > > > > For v3 I went with a straight forward approach. > > > Filesystems that have fsnotify_{create,mkdir} hooks also get > > > explicit fsnotify_{unlink,rmdir} hooks. > > > > > > Hopefully, this approach is orthogonal to whatever changes Al is > > > planning for recursive tree remove code, because in many of the > > > cases, the hooks are added at the entry point for the recursive > > > tree remove. > > > > > > After looking closer at all the filesystems that were converted to > > > simple_remove in v2, I decided to exempt another 3 filesystems from > > > the fsnotify delete hooks: hypfs,qibfs and aafs. > > > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user > > > configuration change, but they do not generate create events, so it > > > is less likely that users depend on the delete events. > > > > > > That leaves configfs the only filesystem that gets the new delete hooks > > > even though it does not have create hooks. > > > > why doesn't configfs have create hooks? That's what userspace does in > > configfs, shouldn't it be notified about it? Keeping it "unequal" seems > > odd to me. > > > > So it's not exactly that configfs has no create hooks at all. > For "normal" filesystems mkdir (for example) is only possible > by mkdir(2) syscall and there is create hook in vfs_mkdir(). Ah, ok, missed that. thanks, greg k-h
Hi Amir! On Sun 26-05-19 17:34:01, Amir Goldstein wrote: > For v3 I went with a straight forward approach. > Filesystems that have fsnotify_{create,mkdir} hooks also get > explicit fsnotify_{unlink,rmdir} hooks. > > Hopefully, this approach is orthogonal to whatever changes Al is > planning for recursive tree remove code, because in many of the > cases, the hooks are added at the entry point for the recursive > tree remove. > > After looking closer at all the filesystems that were converted to > simple_remove in v2, I decided to exempt another 3 filesystems from > the fsnotify delete hooks: hypfs,qibfs and aafs. > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user > configuration change, but they do not generate create events, so it > is less likely that users depend on the delete events. > > That leaves configfs the only filesystem that gets the new delete hooks > even though it does not have create hooks. OK, I went through the patches and they look OK to me. So once we get acks from respective maintainers I think we can merge them. Honza > > The following d_delete() call sites have been audited and will no longer > generate fsnotify event after this series: > > arch/s390/hypfs/inode.c:hypfs_remove() - cleanup (*) > .../usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - no create events > .../infiniband/hw/qib/qib_fs.c:remove_device_files() - no create events > fs/ceph/dir.c:ceph_unlink() - from vfs_unlink() > fs/ceph/inode.c:ceph_fill_trace() - invalidate (**) > fs/ceph/inode.c:ceph_readdir_prepopulate() - invalidate (**) > fs/configfs/dir.c:detach_groups() - hooks added, from vfs or cleanup (*) > 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() - cleanup (*) > fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate (**) > fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode > security/apparmor/apparmorfs.c:aafs_remove() - no create events > > (*) There are 2 "cleanup" use cases: > - Create;init;delete 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, > Amir. > > Changes since v2: > - Drop simple_rename() conversions (add explicit hooks instead) > - Drop hooks from hypfs/qibfs/aafs > - Split out debugfs re-factoring patch > > Changes since v1: > - Split up per filesystem patches > - New hook names fsnotify_{unlink,rmdir}() > - Simplify fsnotify code in separate final patch > > Amir Goldstein (10): > fsnotify: add empty fsnotify_{unlink,rmdir}() hooks > btrfs: call fsnotify_rmdir() hook > rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks > tracefs: call fsnotify_{unlink,rmdir}() hooks > devpts: call fsnotify_unlink() hook > debugfs: simplify __debugfs_remove_file() > debugfs: call fsnotify_{unlink,rmdir}() hooks > configfs: call fsnotify_rmdir() hook > fsnotify: move fsnotify_nameremove() hook out of d_delete() > fsnotify: get rid of fsnotify_nameremove() > > fs/afs/dir_silly.c | 5 ---- > fs/btrfs/ioctl.c | 4 +++- > fs/configfs/dir.c | 3 +++ > fs/dcache.c | 2 -- > fs/debugfs/inode.c | 21 ++++++++-------- > fs/devpts/inode.c | 1 + > fs/namei.c | 2 ++ > fs/nfs/unlink.c | 6 ----- > fs/notify/fsnotify.c | 41 -------------------------------- > fs/tracefs/inode.c | 3 +++ > include/linux/fsnotify.h | 26 ++++++++++++++++++++ > include/linux/fsnotify_backend.h | 4 ---- > net/sunrpc/rpc_pipe.c | 4 ++++ > 13 files changed, 52 insertions(+), 70 deletions(-) > > -- > 2.17.1 >
Jan, For v3 I went with a straight forward approach. Filesystems that have fsnotify_{create,mkdir} hooks also get explicit fsnotify_{unlink,rmdir} hooks. Hopefully, this approach is orthogonal to whatever changes Al is planning for recursive tree remove code, because in many of the cases, the hooks are added at the entry point for the recursive tree remove. After looking closer at all the filesystems that were converted to simple_remove in v2, I decided to exempt another 3 filesystems from the fsnotify delete hooks: hypfs,qibfs and aafs. hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user configuration change, but they do not generate create events, so it is less likely that users depend on the delete events. That leaves configfs the only filesystem that gets the new delete hooks even though it does not have create hooks. The following d_delete() call sites have been audited and will no longer generate fsnotify event after this series: arch/s390/hypfs/inode.c:hypfs_remove() - cleanup (*) .../usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - no create events .../infiniband/hw/qib/qib_fs.c:remove_device_files() - no create events fs/ceph/dir.c:ceph_unlink() - from vfs_unlink() fs/ceph/inode.c:ceph_fill_trace() - invalidate (**) fs/ceph/inode.c:ceph_readdir_prepopulate() - invalidate (**) fs/configfs/dir.c:detach_groups() - hooks added, from vfs or cleanup (*) 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() - cleanup (*) fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate (**) fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode security/apparmor/apparmorfs.c:aafs_remove() - no create events (*) There are 2 "cleanup" use cases: - Create;init;delete 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, Amir. Changes since v2: - Drop simple_rename() conversions (add explicit hooks instead) - Drop hooks from hypfs/qibfs/aafs - Split out debugfs re-factoring patch Changes since v1: - Split up per filesystem patches - New hook names fsnotify_{unlink,rmdir}() - Simplify fsnotify code in separate final patch Amir Goldstein (10): fsnotify: add empty fsnotify_{unlink,rmdir}() hooks btrfs: call fsnotify_rmdir() hook rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks tracefs: call fsnotify_{unlink,rmdir}() hooks devpts: call fsnotify_unlink() hook debugfs: simplify __debugfs_remove_file() debugfs: call fsnotify_{unlink,rmdir}() hooks configfs: call fsnotify_rmdir() hook fsnotify: move fsnotify_nameremove() hook out of d_delete() fsnotify: get rid of fsnotify_nameremove() fs/afs/dir_silly.c | 5 ---- fs/btrfs/ioctl.c | 4 +++- fs/configfs/dir.c | 3 +++ fs/dcache.c | 2 -- fs/debugfs/inode.c | 21 ++++++++-------- fs/devpts/inode.c | 1 + fs/namei.c | 2 ++ fs/nfs/unlink.c | 6 ----- fs/notify/fsnotify.c | 41 -------------------------------- fs/tracefs/inode.c | 3 +++ include/linux/fsnotify.h | 26 ++++++++++++++++++++ include/linux/fsnotify_backend.h | 4 ---- net/sunrpc/rpc_pipe.c | 4 ++++ 13 files changed, 52 insertions(+), 70 deletions(-)