mbox series

[v3,00/10] Sort out fsnotify_nameremove() mess

Message ID 20190526143411.11244-1-amir73il@gmail.com
Headers show
Series Sort out fsnotify_nameremove() mess | expand

Message

Amir Goldstein May 26, 2019, 2:34 p.m. UTC
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(-)

Comments

Greg Kroah-Hartman May 27, 2019, 8:24 a.m. UTC | #1
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
Amir Goldstein May 27, 2019, 9:49 a.m. UTC | #2
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.
Greg Kroah-Hartman May 27, 2019, 11:41 a.m. UTC | #3
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
Jan Kara May 27, 2019, 11:59 a.m. UTC | #4
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
>