mbox series

[v2,00/14] Sort out fsnotify_nameremove() mess

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

Message

Amir Goldstein May 16, 2019, 10:26 a.m. UTC
Jan,

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?

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? (*)
fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
fs/configfs/dir.c:detach_groups() - 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() - invalidate (**)
fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - 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,
Amir.

Changes since v1:
- Split up per filesystem patches
- New hook names fsnotify_{unlink,rmdir}()
- Simplify fsnotify code in separate final patch

Amir Goldstein (14):
  ASoC: rename functions that pollute the simple_xxx namespace
  fs: create simple_remove() helper
  fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
  fs: convert hypfs to use simple_remove() helper
  fs: convert qibfs/ipathfs to use simple_remove() helper
  fs: convert debugfs to use simple_remove() helper
  fs: convert tracefs to use simple_remove() helper
  fs: convert rpc_pipefs to use simple_remove() helper
  fs: convert apparmorfs to use simple_remove() helper
  fsnotify: call fsnotify_rmdir() hook from btrfs
  fsnotify: call fsnotify_rmdir() hook from configfs
  fsnotify: call fsnotify_unlink() hook from devpts
  fsnotify: move fsnotify_nameremove() hook out of d_delete()
  fsnotify: get rid of fsnotify_nameremove()

 arch/s390/hypfs/inode.c            |  9 ++-----
 drivers/infiniband/hw/qib/qib_fs.c |  3 +--
 fs/afs/dir_silly.c                 |  5 ----
 fs/btrfs/ioctl.c                   |  4 ++-
 fs/configfs/dir.c                  |  3 +++
 fs/dcache.c                        |  2 --
 fs/debugfs/inode.c                 | 20 +++------------
 fs/devpts/inode.c                  |  1 +
 fs/libfs.c                         | 32 +++++++++++++++++++++++
 fs/namei.c                         |  2 ++
 fs/nfs/unlink.c                    |  6 -----
 fs/notify/fsnotify.c               | 41 ------------------------------
 fs/tracefs/inode.c                 | 23 +++--------------
 include/linux/fs.h                 |  1 +
 include/linux/fsnotify.h           | 26 +++++++++++++++++++
 include/linux/fsnotify_backend.h   |  4 ---
 net/sunrpc/rpc_pipe.c              | 16 ++----------
 security/apparmor/apparmorfs.c     |  6 +----
 sound/soc/generic/simple-card.c    |  8 +++---
 19 files changed, 86 insertions(+), 126 deletions(-)

Comments

Amir Goldstein May 16, 2019, 10:40 a.m. UTC | #1
On Thu, May 16, 2019 at 1:26 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Jan,
>
> 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?

Doh! I was going to CC Al on the entire series and forgot.

Al,

We could definitely use your input on this series in general and about
merge strategy in particular.
If you would agree to take the entire series through your tree, that could
make things considerably easier.
After all it, this patch set is more a vfs work than it is an fsnotify work.

Thanks,
Amir.

>
> 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? (*)
> fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
> fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
> fs/configfs/dir.c:detach_groups() - 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() - invalidate (**)
> fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - 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,
> Amir.
>
> Changes since v1:
> - Split up per filesystem patches
> - New hook names fsnotify_{unlink,rmdir}()
> - Simplify fsnotify code in separate final patch
>
> Amir Goldstein (14):
>   ASoC: rename functions that pollute the simple_xxx namespace
>   fs: create simple_remove() helper
>   fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
>   fs: convert hypfs to use simple_remove() helper
>   fs: convert qibfs/ipathfs to use simple_remove() helper
>   fs: convert debugfs to use simple_remove() helper
>   fs: convert tracefs to use simple_remove() helper
>   fs: convert rpc_pipefs to use simple_remove() helper
>   fs: convert apparmorfs to use simple_remove() helper
>   fsnotify: call fsnotify_rmdir() hook from btrfs
>   fsnotify: call fsnotify_rmdir() hook from configfs
>   fsnotify: call fsnotify_unlink() hook from devpts
>   fsnotify: move fsnotify_nameremove() hook out of d_delete()
>   fsnotify: get rid of fsnotify_nameremove()
>
>  arch/s390/hypfs/inode.c            |  9 ++-----
>  drivers/infiniband/hw/qib/qib_fs.c |  3 +--
>  fs/afs/dir_silly.c                 |  5 ----
>  fs/btrfs/ioctl.c                   |  4 ++-
>  fs/configfs/dir.c                  |  3 +++
>  fs/dcache.c                        |  2 --
>  fs/debugfs/inode.c                 | 20 +++------------
>  fs/devpts/inode.c                  |  1 +
>  fs/libfs.c                         | 32 +++++++++++++++++++++++
>  fs/namei.c                         |  2 ++
>  fs/nfs/unlink.c                    |  6 -----
>  fs/notify/fsnotify.c               | 41 ------------------------------
>  fs/tracefs/inode.c                 | 23 +++--------------
>  include/linux/fs.h                 |  1 +
>  include/linux/fsnotify.h           | 26 +++++++++++++++++++
>  include/linux/fsnotify_backend.h   |  4 ---
>  net/sunrpc/rpc_pipe.c              | 16 ++----------
>  security/apparmor/apparmorfs.c     |  6 +----
>  sound/soc/generic/simple-card.c    |  8 +++---
>  19 files changed, 86 insertions(+), 126 deletions(-)
>
> --
> 2.17.1
>
Jan Kara May 16, 2019, 12:25 p.m. UTC | #2
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.

> 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
Amir Goldstein May 16, 2019, 1:56 p.m. UTC | #3
On Thu, May 16, 2019 at 3:25 PM Jan Kara <jack@suse.cz> 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@suse.com>
> SUSE Labs, CR
Al Viro May 16, 2019, 4:17 p.m. UTC | #4
On Thu, May 16, 2019 at 04:56:20PM +0300, Amir Goldstein wrote:

> > 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.

FWIW, I've an old series on configfs, from the "deal with kernel-side rm -rf
properly" pile.

I'll try to resurrect and post it.  A _lot_ of locking crap in there is
due to the bad idea of having the subtree being built reachable from
root as we are putting it together; massaging it to the form when we
build a subtree and move it in place only when we are past the last
failure makes for much simpler logics, for obvious reasons.  The massage
is not trivial, though.

In general, the problem with kernel-side recursive removals is that
a bunch of places are trying to cobble local solutions for the problem
that is actually a missing primitive.  And fucking it up in all kinds
of ways.  We definitely want a unified primitive for that; the question
is what kind of API would be kludge-free for callers.

I've a pile of notes and half-assed patch series in that direction;
I'll dig it out and try to get something coherent out of it.