| Message ID | 20220820000550.367085-7-tj@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] kernfs: Simply by replacing kernfs_deref_open_node() with of_on() | expand |
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);
Hello, On Tue, Aug 23, 2022 at 01:49:07PM +0800, Chengming Zhou wrote: > > - 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? Yeah, ACTIVATED means taht created but never activated while kernfs_active() means currently active. I tried to substitute all ACTIVATED tests with kernfs_active() and remove the former but I wasn't sure about changing kernfs_add_one() behavior. I think it's too confusing to combine the initial activated state with user-requested show/hide state and causes other problems like kernfs_activate() used to activate newly created files unhiding files explicitly deactivated. Lemme separate out show/hide state into something separate so that the distinction is clear. Thanks.
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)) 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);
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(-)