Re: [PATCH 6/7] kernfs: Allow kernfs nodes to be deactivated and re-activated

[Chengming Zhou <zhouchengming@bytedance.com>]

On 2022/8/20 08:05, Tejun Heo wrote:
> Currently, kernfs nodes can be created deactivated and activated later to
> allow creation of multiple nodes to succeed or fail as a group. Extend this
> capability so that kernfs nodes can be deactivated and re-activated anytime
> and however many times. This can be used to toggle interface files for
> features which can be dynamically turned on and off.
> 
> kernfs_activate()'s skip conditions are updated so that it doesn't ignore
> re-activations and suppress re-activations of files which are being removed.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chengming Zhou <zhouchengming@bytedance.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  fs/kernfs/dir.c        | 89 ++++++++++++++++++++++++++++++------------
>  include/linux/kernfs.h |  2 +
>  2 files changed, 65 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index f857731598cd..6db031362585 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -483,7 +483,7 @@ static bool kernfs_drain(struct kernfs_node *kn)
>  	 * worrying about draining.
>  	 */
>  	if (atomic_read(&kn->active) == KN_DEACTIVATED_BIAS &&
> -	    kernfs_should_drain_open_files(kn))
> +	    !kernfs_should_drain_open_files(kn))
>  		return false;
>  
>  	up_write(&root->kernfs_rwsem);
> @@ -1321,14 +1321,15 @@ static struct kernfs_node *kernfs_next_descendant_post(struct kernfs_node *pos,
>  }
>  
>  /**
> - * kernfs_activate - activate a node which started deactivated
> + * kernfs_activate - activate a node's subtree
>   * @kn: kernfs_node whose subtree is to be activated
>   *
> - * If the root has KERNFS_ROOT_CREATE_DEACTIVATED set, a newly created node
> - * needs to be explicitly activated.  A node which hasn't been activated
> - * isn't visible to userland and deactivation is skipped during its
> - * removal.  This is useful to construct atomic init sequences where
> - * creation of multiple nodes should either succeed or fail atomically.
> + * If newly created on a root w/ %KERNFS_ROOT_CREATE_DEACTIVATED or after a
> + * kernfs_deactivate() call, @kn is deactivated and invisible to userland. This
> + * function activates all nodes in @kn's inclusive subtree making them visible.
> + *
> + * %KERNFS_ROOT_CREATE_DEACTIVATED is useful when constructing init sequences
> + * where creation of multiple nodes should either succeed or fail atomically.
>   *
>   * The caller is responsible for ensuring that this function is not called
>   * after kernfs_remove*() is invoked on @kn.
> @@ -1342,7 +1343,7 @@ void kernfs_activate(struct kernfs_node *kn)
>  
>  	pos = NULL;
>  	while ((pos = kernfs_next_descendant_post(pos, kn))) {
> -		if (pos->flags & KERNFS_ACTIVATED)
> +		if (kernfs_active(pos) || (kn->flags & KERNFS_REMOVING))

May I ask a question, what's the difference between kernfs_active() and KERNFS_ACTIVATED?

KERNFS_ACTIVATED is always set when kernfs_activate() and never clear, so I think it means:

1. !KERNFS_ACTIVATED : allocated but not activated
2. KERNFS_ACTIVATED && !kernfs_active() : make deactivated by kernfs_deactivate_locked()

I see most code check kernfs_active(), but two places check KERNFS_ACTIVATED, I'm not sure where
should check KERNFS_ACTIVATED, or is there any chance we can remove KERNFS_ACTIVATED?

Thanks!


>  			continue;
>  
>  		WARN_ON_ONCE(pos->parent && RB_EMPTY_NODE(&pos->rb));
> @@ -1355,6 +1356,58 @@ void kernfs_activate(struct kernfs_node *kn)
>  	up_write(&root->kernfs_rwsem);
>  }
>  
> +static void kernfs_deactivate_locked(struct kernfs_node *kn, bool removing)
> +{
> +	struct kernfs_root *root = kernfs_root(kn);
> +	struct kernfs_node *pos;
> +
> +	lockdep_assert_held_write(&root->kernfs_rwsem);
> +
> +	/* prevent any new usage under @kn by deactivating all nodes */
> +	pos = NULL;
> +	while ((pos = kernfs_next_descendant_post(pos, kn))) {
> +		if (kernfs_active(pos))
> +			atomic_add(KN_DEACTIVATED_BIAS, &pos->active);
> +		if (removing)
> +			pos->flags |= KERNFS_REMOVING;
> +	}
> +
> +	/*
> +	 * No new active usage can be created. Drain existing ones. As
> +	 * kernfs_drain() may drop kernfs_rwsem temporarily, pin @pos so that it
> +	 * doesn't go away underneath us.
> +	 *
> +	 * If kernfs_rwsem was released, restart from the beginning. Forward
> +	 * progress is guaranteed as a drained node is guaranteed to stay
> +	 * drained. In the unlikely case that the loop restart ever becomes a
> +	 * problem, we should be able to work around by batching up the
> +	 * draining.
> +	 */
> +	pos = NULL;
> +	while ((pos = kernfs_next_descendant_post(pos, kn))) {
> +		kernfs_get(pos);
> +		if (kernfs_drain(pos))
> +			pos = NULL;
> +		kernfs_put(pos);
> +	}
> +}
> +
> +/**
> + * kernfs_deactivate - deactivate a node's subtree
> + * @kn: kernfs_node whose subtree is to be deactivated
> + *
> + * Deactivate @kn's inclusive subtree. On return, the subtree is invisible to
> + * userland and there are no in-flight file operations.
> + */
> +void kernfs_deactivate(struct kernfs_node *kn)
> +{
> +	struct kernfs_root *root = kernfs_root(kn);
> +
> +	down_write(&root->kernfs_rwsem);
> +	kernfs_deactivate_locked(kn, false);
> +	up_write(&root->kernfs_rwsem);
> +}
> +
>  static void __kernfs_remove(struct kernfs_node *kn)
>  {
>  	struct kernfs_node *pos;
> @@ -1374,26 +1427,12 @@ static void __kernfs_remove(struct kernfs_node *kn)
>  
>  	pr_debug("kernfs %s: removing\n", kn->name);
>  
> -	/* prevent any new usage under @kn by deactivating all nodes */
> -	pos = NULL;
> -	while ((pos = kernfs_next_descendant_post(pos, kn)))
> -		if (kernfs_active(pos))
> -			atomic_add(KN_DEACTIVATED_BIAS, &pos->active);
> +	kernfs_deactivate_locked(kn, true);
>  
> -	/* deactivate and unlink the subtree node-by-node */
> +	/* unlink the subtree node-by-node */
>  	do {
>  		pos = kernfs_leftmost_descendant(kn);
>  
> -		/*
> -		 * kernfs_drain() may drop kernfs_rwsem temporarily and @pos's
> -		 * base ref could have been put by someone else by the time
> -		 * the function returns.  Make sure it doesn't go away
> -		 * underneath us.
> -		 */
> -		kernfs_get(pos);
> -
> -		kernfs_drain(pos);
> -
>  		/*
>  		 * kernfs_unlink_sibling() succeeds once per node.  Use it
>  		 * to decide who's responsible for cleanups.
> @@ -1410,8 +1449,6 @@ static void __kernfs_remove(struct kernfs_node *kn)
>  
>  			kernfs_put(pos);
>  		}
> -
> -		kernfs_put(pos);
>  	} while (pos != kn);
>  }
>  
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index 367044d7708c..657eea1395b6 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -112,6 +112,7 @@ enum kernfs_node_flag {
>  	KERNFS_SUICIDED		= 0x0800,
>  	KERNFS_EMPTY_DIR	= 0x1000,
>  	KERNFS_HAS_RELEASE	= 0x2000,
> +	KERNFS_REMOVING		= 0x4000,
>  };
>  
>  /* @flags for kernfs_create_root() */
> @@ -429,6 +430,7 @@ struct kernfs_node *kernfs_create_link(struct kernfs_node *parent,
>  				       const char *name,
>  				       struct kernfs_node *target);
>  void kernfs_activate(struct kernfs_node *kn);
> +void kernfs_deactivate(struct kernfs_node *kn);
>  void kernfs_remove(struct kernfs_node *kn);
>  void kernfs_break_active_protection(struct kernfs_node *kn);
>  void kernfs_unbreak_active_protection(struct kernfs_node *kn);