Revert "kernfs: restructure removal path to fix possible premature return"

This reverts commit 45a140e587f3d32d8d424ed940dffb61e1739047.

Tejun writes:
        I'm sorry but can you please revert the whole series?
        get_active() waiting while a node is deactivated has potential
        to lead to deadlock and that deactivate/reactivate interface is
        something fundamentally flawed and that cgroup will have to work
        with the remove_self() like everybody else.  IOW, I think the
        first posting was correct.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

1 files changed, 53 insertions, 86 deletions

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index e565ec096ae9..7f8afc1d08f1 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -181,38 +181,14 @@ void kernfs_put_active(struct kernfs_node *kn)
  * kernfs_drain - drain kernfs_node
  * @kn: kernfs_node to drain
  *
- * Drain existing usages of @kn.  Mutiple removers may invoke this function
- * concurrently on @kn and all will return after draining is complete.
- * Returns %true if drain is performed and kernfs_mutex was temporarily
- * released.  %false if @kn was already drained and no operation was
- * necessary.
- *
- * The caller is responsible for ensuring @kn stays pinned while this
- * function is in progress even if it gets removed by someone else.
+ * Drain existing usages.
  */
-static bool kernfs_drain(struct kernfs_node *kn)
-        __releases(&kernfs_mutex) __acquires(&kernfs_mutex)
+static void kernfs_drain(struct kernfs_node *kn)
 {
         struct kernfs_root *root = kernfs_root(kn);
 
-        lockdep_assert_held(&kernfs_mutex);
         WARN_ON_ONCE(atomic_read(&kn->active) >= 0);
 
-        /*
-         * We want to go through the active ref lockdep annotation at least
-         * once for all node removals, but the lockdep annotation can't be
-         * nested inside kernfs_mutex and deactivation can't make forward
-         * progress if we keep dropping the mutex.  Use JUST_ACTIVATED to
-         * force the slow path once for each deactivation if lockdep is
-         * enabled.
-         */
-        if ((!kernfs_lockdep(kn) || !(kn->flags & KERNFS_JUST_DEACTIVATED)) &&
-            atomic_read(&kn->active) == KN_DEACTIVATED_BIAS)
-                return false;
-
-        kn->flags &= ~KERNFS_JUST_DEACTIVATED;
-        mutex_unlock(&kernfs_mutex);
-
         if (kernfs_lockdep(kn)) {
                 rwsem_acquire(&kn->dep_map, 0, 0, _RET_IP_);
                 if (atomic_read(&kn->active) != KN_DEACTIVATED_BIAS)
@@ -226,9 +202,6 @@ static bool kernfs_drain(struct kernfs_node *kn)
                 lock_acquired(&kn->dep_map, _RET_IP_);
                 rwsem_release(&kn->dep_map, 1, _RET_IP_);
         }
-
-        mutex_lock(&kernfs_mutex);
-        return true;
 }
 
 /**
@@ -474,6 +447,49 @@ int kernfs_add_one(struct kernfs_addrm_cxt *acxt, struct kernfs_node *kn,
 }
 
 /**
+ *      kernfs_remove_one - remove kernfs_node from parent
+ *      @acxt: addrm context to use
+ *      @kn: kernfs_node to be removed
+ *
+ *      Mark @kn removed and drop nlink of parent inode if @kn is a
+ *      directory.  @kn is unlinked from the children list.
+ *
+ *      This function should be called between calls to
+ *      kernfs_addrm_start() and kernfs_addrm_finish() and should be
+ *      passed the same @acxt as passed to kernfs_addrm_start().
+ *
+ *      LOCKING:
+ *      Determined by kernfs_addrm_start().
+ */
+static void kernfs_remove_one(struct kernfs_addrm_cxt *acxt,
+                              struct kernfs_node *kn)
+{
+        struct kernfs_iattrs *ps_iattr;
+
+        /*
+         * Removal can be called multiple times on the same node.  Only the
+         * first invocation is effective and puts the base ref.
+         */
+        if (atomic_read(&kn->active) < 0)
+                return;
+
+        if (kn->parent) {
+                kernfs_unlink_sibling(kn);
+
+                /* Update timestamps on the parent */
+                ps_iattr = kn->parent->iattr;
+                if (ps_iattr) {
+                        ps_iattr->ia_iattr.ia_ctime = CURRENT_TIME;
+                        ps_iattr->ia_iattr.ia_mtime = CURRENT_TIME;
+                }
+        }
+
+        atomic_add(KN_DEACTIVATED_BIAS, &kn->active);
+        kn->u.removed_list = acxt->removed;
+        acxt->removed = kn;
+}
+
+/**
  *      kernfs_addrm_finish - finish up kernfs_node add/remove
  *      @acxt: addrm context to finish up
  *
@@ -496,6 +512,7 @@ void kernfs_addrm_finish(struct kernfs_addrm_cxt *acxt)
 
                 acxt->removed = kn->u.removed_list;
 
+                kernfs_drain(kn);
                 kernfs_unmap_bin_file(kn);
                 kernfs_put(kn);
         }
@@ -805,73 +822,23 @@ static struct kernfs_node *kernfs_next_descendant_post(struct kernfs_node *pos,
         return pos->parent;
 }
 
-static void __kernfs_deactivate(struct kernfs_node *kn)
-{
-        struct kernfs_node *pos;
-
-        lockdep_assert_held(&kernfs_mutex);
-
-        /* prevent any new usage under @kn by deactivating all nodes */
-        pos = NULL;
-        while ((pos = kernfs_next_descendant_post(pos, kn))) {
-                if (atomic_read(&pos->active) >= 0) {
-                        atomic_add(KN_DEACTIVATED_BIAS, &pos->active);
-                        pos->flags |= KERNFS_JUST_DEACTIVATED;
-                }
-        }
-
-        /*
-         * Drain the subtree.  If kernfs_drain() blocked to drain, which is
-         * indicated by %true return, it temporarily released kernfs_mutex
-         * and the rbtree might have been modified inbetween breaking our
-         * future walk.  Restart the walk after each %true return.
-         */
-        pos = NULL;
-        while ((pos = kernfs_next_descendant_post(pos, kn))) {
-                bool drained;
-
-                kernfs_get(pos);
-                drained = kernfs_drain(pos);
-                kernfs_put(pos);
-                if (drained)
-                        pos = NULL;
-        }
-}
-
 static void __kernfs_remove(struct kernfs_addrm_cxt *acxt,
                             struct kernfs_node *kn)
 {
-        struct kernfs_node *pos;
-
-        lockdep_assert_held(&kernfs_mutex);
+        struct kernfs_node *pos, *next;
 
         if (!kn)
                 return;
 
         pr_debug("kernfs %s: removing\n", kn->name);
 
-        __kernfs_deactivate(kn);
-
-        /* unlink the subtree node-by-node */
+        next = NULL;
         do {
-                struct kernfs_iattrs *ps_iattr;
-
-                pos = kernfs_leftmost_descendant(kn);
-
-                if (pos->parent) {
-                        kernfs_unlink_sibling(pos);
-
-                        /* update timestamps on the parent */
-                        ps_iattr = pos->parent->iattr;
-                        if (ps_iattr) {
-                                ps_iattr->ia_iattr.ia_ctime = CURRENT_TIME;
-                                ps_iattr->ia_iattr.ia_mtime = CURRENT_TIME;
-                        }
-                }
-
-                pos->u.removed_list = acxt->removed;
-                acxt->removed = pos;
-        } while (pos != kn);
+                pos = next;
+                next = kernfs_next_descendant_post(pos, kn);
+                if (pos)
+                        kernfs_remove_one(acxt, pos);
+        } while (next);
 }
 
 /**