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

[Jan Kara <jack@suse.cz>]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR