Re: [PATCH v3 2/3] kernfs: send IN_DELETE_SELF and IN_IGNORED on file deletion

[Jan Kara]

On Tue 17-02-26 19:22:31, T.J. Mercier wrote:
> Currently some kernfs files (e.g. cgroup.events, memory.events) support
> inotify watches for IN_MODIFY, but unlike with regular filesystems, they
> do not receive IN_DELETE_SELF or IN_IGNORED events when they are
> removed.

Please see my email:
https://lore.kernel.org/all/lc2jgt3yrvuvtdj2kk7q3rloie2c5mzyhfdy4zvxylx732voet@ol3kl4ackrpb

I think this is actually a bug in kernfs...

								Honza

> 
> This creates a problem for processes monitoring cgroups. For example, a
> service monitoring memory.events for memory.high breaches needs to know
> when a cgroup is removed to clean up its state. Where it's known that a
> cgroup is removed when all processes die, without IN_DELETE_SELF the
> service must resort to inefficient workarounds such as:
> 1.  Periodically scanning procfs to detect process death (wastes CPU and
>     is susceptible to PID reuse).
> 2.  Placing an additional IN_DELETE watch on the parent directory
>     (wastes resources managing double the watches).
> 3.  Holding a pidfd for every monitored cgroup (can exhaust file
>     descriptors).
> 
> This patch enables kernfs to send IN_DELETE_SELF and IN_IGNORED events.
> This allows applications to rely on a single existing watch on the file
> of interest (e.g. memory.events) to receive notifications for both
> modifications and the eventual removal of the file, as well as automatic
> watch descriptor cleanup, simplifying userspace logic and improving
> resource efficiency.
> 
> Implementation details:
> The kernfs notification worker is updated to handle file deletion.
> The optimized single call for MODIFY events to both the parent and the
> file is retained, however because CREATE (parent) events remain
> unsupported for kernfs files, support for DELETE (parent) events is not
> added here to retain symmetry. Only support for DELETE_SELF events is
> added.
> 
> Signed-off-by: T.J. Mercier <tjmercier@xxxxxxxxxx>
> Acked-by: Tejun Heo <tj@xxxxxxxxxx>
> ---
>  fs/kernfs/dir.c             | 21 +++++++++++++++++
>  fs/kernfs/file.c            | 45 ++++++++++++++++++++-----------------
>  fs/kernfs/kernfs-internal.h |  3 +++
>  3 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 29baeeb97871..e5bda829fcb8 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/sched.h>
>  #include <linux/fs.h>
> +#include <linux/fsnotify_backend.h>
>  #include <linux/namei.h>
>  #include <linux/idr.h>
>  #include <linux/slab.h>
> @@ -1471,6 +1472,23 @@ void kernfs_show(struct kernfs_node *kn, bool show)
>  	up_write(&root->kernfs_rwsem);
>  }
>  
> +static void kernfs_notify_file_deleted(struct kernfs_node *kn)
> +{
> +	static DECLARE_WORK(kernfs_notify_deleted_work,
> +			    kernfs_notify_workfn);
> +
> +	guard(spinlock_irqsave)(&kernfs_notify_lock);
> +	/* may overwite already pending FS_MODIFY events */
> +	kn->attr.notify_event = FS_DELETE;
> +
> +	if (!kn->attr.notify_next) {
> +		kernfs_get(kn);
> +		kn->attr.notify_next = kernfs_notify_list;
> +		kernfs_notify_list = kn;
> +		schedule_work(&kernfs_notify_deleted_work);
> +	}
> +}
> +
>  static void __kernfs_remove(struct kernfs_node *kn)
>  {
>  	struct kernfs_node *pos, *parent;
> @@ -1520,6 +1538,9 @@ static void __kernfs_remove(struct kernfs_node *kn)
>  			struct kernfs_iattrs *ps_iattr =
>  				parent ? parent->iattr : NULL;
>  
> +			if (kernfs_type(pos) == KERNFS_FILE)
> +				kernfs_notify_file_deleted(pos);
> +
>  			/* update timestamps on the parent */
>  			down_write(&kernfs_root(kn)->kernfs_iattr_rwsem);
>  
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index e978284ff983..4be9bbe29378 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -37,8 +37,8 @@ struct kernfs_open_node {
>   */
>  #define KERNFS_NOTIFY_EOL			((void *)&kernfs_notify_list)
>  
> -static DEFINE_SPINLOCK(kernfs_notify_lock);
> -static struct kernfs_node *kernfs_notify_list = KERNFS_NOTIFY_EOL;
> +DEFINE_SPINLOCK(kernfs_notify_lock);
> +struct kernfs_node *kernfs_notify_list = KERNFS_NOTIFY_EOL;
>  
>  static inline struct mutex *kernfs_open_file_mutex_ptr(struct kernfs_node *kn)
>  {
> @@ -909,7 +909,7 @@ static loff_t kernfs_fop_llseek(struct file *file, loff_t offset, int whence)
>  	return ret;
>  }
>  
> -static void kernfs_notify_workfn(struct work_struct *work)
> +void kernfs_notify_workfn(struct work_struct *work)
>  {
>  	struct kernfs_node *kn;
>  	struct kernfs_super_info *info;
> @@ -935,11 +935,7 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  	down_read(&root->kernfs_supers_rwsem);
>  	down_read(&root->kernfs_rwsem);
>  	list_for_each_entry(info, &kernfs_root(kn)->supers, node) {
> -		struct kernfs_node *parent;
> -		struct inode *p_inode = NULL;
> -		const char *kn_name;
>  		struct inode *inode;
> -		struct qstr name;
>  
>  		/*
>  		 * We want fsnotify_modify() on @kn but as the
> @@ -951,24 +947,31 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  		if (!inode)
>  			continue;
>  
> -		kn_name = kernfs_rcu_name(kn);
> -		name = QSTR(kn_name);
> -		parent = kernfs_get_parent(kn);
> -		if (parent) {
> -			p_inode = ilookup(info->sb, kernfs_ino(parent));
> -			if (p_inode) {
> -				fsnotify(notify_event | FS_EVENT_ON_CHILD,
> -					 inode, FSNOTIFY_EVENT_INODE,
> -					 p_inode, &name, inode, 0);
> -				iput(p_inode);
> +		if (notify_event == FS_DELETE) {
> +			fsnotify_inoderemove(inode);
> +		} else {
> +			struct kernfs_node *parent = kernfs_get_parent(kn);
> +			struct inode *p_inode = NULL;
> +
> +			if (parent) {
> +				p_inode = ilookup(info->sb, kernfs_ino(parent));
> +				if (p_inode) {
> +					const char *kn_name = kernfs_rcu_name(kn);
> +					struct qstr name = QSTR(kn_name);
> +
> +					fsnotify(notify_event | FS_EVENT_ON_CHILD,
> +						 inode, FSNOTIFY_EVENT_INODE,
> +						 p_inode, &name, inode, 0);
> +					iput(p_inode);
> +				}
> +
> +				kernfs_put(parent);
>  			}
>  
> -			kernfs_put(parent);
> +			if (!p_inode)
> +				fsnotify_inode(inode, notify_event);
>  		}
>  
> -		if (!p_inode)
> -			fsnotify_inode(inode, notify_event);
> -
>  		iput(inode);
>  	}
>  
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index 6061b6f70d2a..cf4b21f4f3b6 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -199,6 +199,8 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
>   * file.c
>   */
>  extern const struct file_operations kernfs_file_fops;
> +extern struct kernfs_node *kernfs_notify_list;
> +extern void kernfs_notify_workfn(struct work_struct *work);
>  
>  bool kernfs_should_drain_open_files(struct kernfs_node *kn);
>  void kernfs_drain_open_files(struct kernfs_node *kn);
> @@ -212,4 +214,5 @@ extern const struct inode_operations kernfs_symlink_iops;
>   * kernfs locks
>   */
>  extern struct kernfs_global_locks *kernfs_locks;
> +extern spinlock_t kernfs_notify_lock;
>  #endif	/* __KERNFS_INTERNAL_H */
> -- 
> 2.53.0.310.g728cabbaf7-goog
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR