Re: [PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[Amir Goldstein <amir73il@gmail.com>]

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