Re: [PATCH v4 05/10] eventfs: Implement eventfs file, directory remove function

[Steven Rostedt]

Some more nits:

Subject:

 eventfs: Implement removal of meta data from eventfs


On Thu, 13 Jul 2023 17:03:19 +0530
Ajay Kaher <akaher@xxxxxxxxxx> wrote:

> Adding eventfs_remove(), this function will recursively remove
> dir or file info from eventfs.

 When events are removed from tracefs, the eventfs must be aware of this.
 The eventfs_remove() removes the meta data from eventfs so that it will no
 longer create the files associated with that event.

 When an instance is removed from tracefs, eventfs_remove_events_dir() will
 remove and clean up the entire "events" directory.


> 
> added a recursion check to eventfs_remove_rec() as it is really
> dangerous to have unchecked recursion in the kernel (we do have
> a fixed size stack).

The above doesn't need to be in the change log. It's an update from the
previous version. The eventfs_remove_rec() is added here, so the recursion
check was not.

> 
> have the free use srcu callbacks. After the srcu grace periods
> are done, it adds the eventfs_file onto a llist (lockless link
> list) and wakes up a work queue. Then the work queue does the
> freeing (this needs to be done in task/workqueue context, as
> srcu callbacks are done in softirq context).

If you want to document the above, we can say:

 The helper function eventfs_remove_rec() is used to clean up and free the
 associated data from eventfs for both of the added functions. SRCU is used
 to protect the lists of meta data stored in the eventfs. The eventfs_mutex
 is used to protect the content of the items in the list.

 As lookups may be happening as deletions of events are made, the freeing
 of of dentry/inodes and relative information is done after the SRCU grace
 period has passed. As the callback of SRCU is in a softirq context, a work
 queue is added to perform the cleanups in a task context.

 The struct evenfs_file is given a union of an rcu_head and a llist_node.
 The SRCU callback uses the rcu_head from this structure to insert it into
 the SRCU queue. When the SRCU grace periods are complete, the callback
 will then insert the eventfs_file struct onto a lockless llist using the
 llist_node of the structure. A union is used as this process is just a
 hand off from SRCU to workqueue, and only one field is necessary for this
 to work.

You can also add the above as a comment in the code (and keep it in the
change log as well).

-- Steve


> 
> Signed-off-by: Ajay Kaher <akaher@xxxxxxxxxx>
> Co-developed-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> Tested-by: Ching-lin Yu <chinglinyu@xxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202305030611.Kas747Ev-lkp@xxxxxxxxx/
> ---
>  fs/tracefs/event_inode.c | 110 +++++++++++++++++++++++++++++++++++++++
>  include/linux/tracefs.h  |   4 ++
>  2 files changed, 114 insertions(+)
> 
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index 75dc8953d813..322a77be5a56 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -45,6 +45,7 @@ struct eventfs_file {
>  };
>  
>  static DEFINE_MUTEX(eventfs_mutex);
> +DEFINE_STATIC_SRCU(eventfs_srcu);
>  
>  static const struct file_operations eventfs_file_operations = {
>  };
> @@ -299,3 +300,112 @@ int eventfs_add_file(const char *name, umode_t mode,
>  	mutex_unlock(&eventfs_mutex);
>  	return 0;
>  }
> +
> +static LLIST_HEAD(free_list);
> +
> +static void eventfs_workfn(struct work_struct *work)
> +{
> +	struct eventfs_file *ef, *tmp;
> +	struct llist_node *llnode;
> +
> +	llnode = llist_del_all(&free_list);
> +	llist_for_each_entry_safe(ef, tmp, llnode, llist) {
> +		if (ef->created && ef->dentry)
> +			dput(ef->dentry);
> +		kfree(ef->name);
> +		kfree(ef->ei);
> +		kfree(ef);
> +	}
> +}
> +
> +DECLARE_WORK(eventfs_work, eventfs_workfn);
> +
> +static void free_ef(struct rcu_head *head)
> +{
> +	struct eventfs_file *ef = container_of(head, struct eventfs_file, rcu);
> +
> +	if (!llist_add(&ef->llist, &free_list))
> +		return;
> +
> +	queue_work(system_unbound_wq, &eventfs_work);
> +}
> +
> +
> +
> +/**
> + * eventfs_remove_rec - remove eventfs dir or file from list
> + * @ef: eventfs_file to be removed.
> + *
> + * This function recursively remove eventfs_file which
> + * contains info of file or dir.
> + */
> +static void eventfs_remove_rec(struct eventfs_file *ef, int level)
> +{
> +	struct eventfs_file *ef_child;
> +
> +	if (!ef)
> +		return;
> +	/*
> +	 * Check recursion depth. It should never be greater than 3:
> +	 * 0 - events/
> +	 * 1 - events/group/
> +	 * 2 - events/group/event/
> +	 * 3 - events/group/event/file
> +	 */
> +	if (WARN_ON_ONCE(level > 3))
> +		return;
> +
> +	if (ef->ei) {
> +		/* search for nested folders or files */
> +		list_for_each_entry_srcu(ef_child, &ef->ei->e_top_files, list,
> +					 lockdep_is_held(&eventfs_mutex)) {
> +			eventfs_remove_rec(ef_child, level + 1);
> +		}
> +	}
> +
> +	if (ef->created && ef->dentry)
> +		d_invalidate(ef->dentry);
> +
> +	list_del_rcu(&ef->list);
> +	call_srcu(&eventfs_srcu, &ef->rcu, free_ef);
> +}
> +
> +/**
> + * eventfs_remove - remove eventfs dir or file from list
> + * @ef: eventfs_file to be removed.
> + *
> + * This function acquire the eventfs_mutex lock and call eventfs_remove_rec()
> + */
> +void eventfs_remove(struct eventfs_file *ef)
> +{
> +	if (!ef)
> +		return;
> +
> +	mutex_lock(&eventfs_mutex);
> +	eventfs_remove_rec(ef, 0);
> +	mutex_unlock(&eventfs_mutex);
> +}
> +
> +/**
> + * eventfs_remove_events_dir - remove eventfs dir or file from list
> + * @dentry: events's dentry to be removed.
> + *
> + * This function remove events main directory
> + */
> +void eventfs_remove_events_dir(struct dentry *dentry)
> +{
> +	struct tracefs_inode *ti;
> +	struct eventfs_inode *ei;
> +
> +	if (!dentry || !dentry->d_inode)
> +		return;
> +
> +	ti = get_tracefs(dentry->d_inode);
> +	if (!ti || !(ti->flags & TRACEFS_EVENT_INODE))
> +		return;
> +
> +	ei = ti->private;
> +	d_invalidate(dentry);
> +	dput(dentry);
> +	kfree(ei);
> +}
> diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
> index a51312ff803c..2c08edd4a739 100644
> --- a/include/linux/tracefs.h
> +++ b/include/linux/tracefs.h
> @@ -40,6 +40,10 @@ int eventfs_add_top_file(const char *name, umode_t mode,
>  			 struct dentry *parent, void *data,
>  			 const struct file_operations *fops);
>  
> +void eventfs_remove(struct eventfs_file *ef);
> +
> +void eventfs_remove_events_dir(struct dentry *dentry);
> +
>  struct dentry *tracefs_create_file(const char *name, umode_t mode,
>  				   struct dentry *parent, void *data,
>  				   const struct file_operations *fops);