Revert "kernfs: remove kernfs_addrm_cxt"
This reverts commit 99177a34110889a8f2c36420c34e3bcc9bfd8a70.
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>
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 770d687..f878e4f 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -399,7 +399,28 @@
}
/**
+ * kernfs_addrm_start - prepare for kernfs_node add/remove
+ * @acxt: pointer to kernfs_addrm_cxt to be used
+ *
+ * This function is called when the caller is about to add or remove
+ * kernfs_node. This function acquires kernfs_mutex. @acxt is used
+ * to keep and pass context to other addrm functions.
+ *
+ * LOCKING:
+ * Kernel thread context (may sleep). kernfs_mutex is locked on
+ * return.
+ */
+void kernfs_addrm_start(struct kernfs_addrm_cxt *acxt)
+ __acquires(kernfs_mutex)
+{
+ memset(acxt, 0, sizeof(*acxt));
+
+ mutex_lock(&kernfs_mutex);
+}
+
+/**
* kernfs_add_one - add kernfs_node to parent without warning
+ * @acxt: addrm context to use
* @kn: kernfs_node to be added
* @parent: the parent kernfs_node to add @kn to
*
@@ -407,29 +428,34 @@
* parent inode if @kn is a directory and link into the children list
* of the parent.
*
+ * 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().
+ *
* RETURNS:
* 0 on success, -EEXIST if entry with the given name already
* exists.
*/
-int kernfs_add_one(struct kernfs_node *kn, struct kernfs_node *parent)
+int kernfs_add_one(struct kernfs_addrm_cxt *acxt, struct kernfs_node *kn,
+ struct kernfs_node *parent)
{
+ bool has_ns = kernfs_ns_enabled(parent);
struct kernfs_iattrs *ps_iattr;
- bool has_ns;
int ret;
- if (!kernfs_get_active(parent))
- return -ENOENT;
+ WARN_ON_ONCE(atomic_read(&parent->active) < 0);
- mutex_lock(&kernfs_mutex);
-
- ret = -EINVAL;
- has_ns = kernfs_ns_enabled(parent);
- if (WARN(has_ns != (bool)kn->ns, KERN_WARNING "kernfs: ns %s in '%s' for '%s'\n",
- has_ns ? "required" : "invalid", parent->name, kn->name))
- goto out_unlock;
+ if (has_ns != (bool)kn->ns) {
+ WARN(1, KERN_WARNING "kernfs: ns %s in '%s' for '%s'\n",
+ has_ns ? "required" : "invalid", parent->name, kn->name);
+ return -EINVAL;
+ }
if (kernfs_type(parent) != KERNFS_DIR)
- goto out_unlock;
+ return -EINVAL;
kn->hash = kernfs_name_hash(kn->name, kn->ns);
kn->parent = parent;
@@ -437,7 +463,7 @@
ret = kernfs_link_sibling(kn);
if (ret)
- goto out_unlock;
+ return ret;
/* Update timestamps on the parent */
ps_iattr = parent->iattr;
@@ -448,11 +474,34 @@
/* Mark the entry added into directory tree */
atomic_sub(KN_DEACTIVATED_BIAS, &kn->active);
- ret = 0;
-out_unlock:
+ return 0;
+}
+
+/**
+ * kernfs_addrm_finish - finish up kernfs_node add/remove
+ * @acxt: addrm context to finish up
+ *
+ * Finish up kernfs_node add/remove. Resources acquired by
+ * kernfs_addrm_start() are released and removed kernfs_nodes are
+ * cleaned up.
+ *
+ * LOCKING:
+ * kernfs_mutex is released.
+ */
+void kernfs_addrm_finish(struct kernfs_addrm_cxt *acxt)
+ __releases(kernfs_mutex)
+{
+ /* release resources acquired by kernfs_addrm_start() */
mutex_unlock(&kernfs_mutex);
- kernfs_put_active(parent);
- return ret;
+
+ /* kill removed kernfs_nodes */
+ while (acxt->removed) {
+ struct kernfs_node *kn = acxt->removed;
+
+ acxt->removed = kn->u.removed_list;
+
+ kernfs_put(kn);
+ }
}
/**
@@ -584,6 +633,7 @@
const char *name, umode_t mode,
void *priv, const void *ns)
{
+ struct kernfs_addrm_cxt acxt;
struct kernfs_node *kn;
int rc;
@@ -598,7 +648,14 @@
kn->priv = priv;
/* link in */
- rc = kernfs_add_one(kn, parent);
+ rc = -ENOENT;
+ if (kernfs_get_active(parent)) {
+ kernfs_addrm_start(&acxt);
+ rc = kernfs_add_one(&acxt, kn, parent);
+ kernfs_addrm_finish(&acxt);
+ kernfs_put_active(parent);
+ }
+
if (!rc)
return kn;
@@ -784,7 +841,8 @@
}
}
-static void __kernfs_remove(struct kernfs_node *kn)
+static void __kernfs_remove(struct kernfs_addrm_cxt *acxt,
+ struct kernfs_node *kn)
{
struct kernfs_node *pos;
@@ -832,7 +890,8 @@
ps_iattr->ia_iattr.ia_mtime = CURRENT_TIME;
}
- kernfs_put(pos);
+ pos->u.removed_list = acxt->removed;
+ acxt->removed = pos;
}
kernfs_put(pos);
@@ -847,9 +906,11 @@
*/
void kernfs_remove(struct kernfs_node *kn)
{
- mutex_lock(&kernfs_mutex);
- __kernfs_remove(kn);
- mutex_unlock(&kernfs_mutex);
+ struct kernfs_addrm_cxt acxt;
+
+ kernfs_addrm_start(&acxt);
+ __kernfs_remove(&acxt, kn);
+ kernfs_addrm_finish(&acxt);
}
/**
@@ -864,6 +925,7 @@
int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
const void *ns)
{
+ struct kernfs_addrm_cxt acxt;
struct kernfs_node *kn;
if (!parent) {
@@ -872,13 +934,13 @@
return -ENOENT;
}
- mutex_lock(&kernfs_mutex);
+ kernfs_addrm_start(&acxt);
kn = kernfs_find_ns(parent, name, ns);
if (kn)
- __kernfs_remove(kn);
+ __kernfs_remove(&acxt, kn);
- mutex_unlock(&kernfs_mutex);
+ kernfs_addrm_finish(&acxt);
if (kn)
return 0;
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index ffe1beb..404ffd2 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -817,6 +817,7 @@
bool name_is_static,
struct lock_class_key *key)
{
+ struct kernfs_addrm_cxt acxt;
struct kernfs_node *kn;
unsigned flags;
int rc;
@@ -852,7 +853,14 @@
if (ops->mmap)
kn->flags |= KERNFS_HAS_MMAP;
- rc = kernfs_add_one(kn, parent);
+ rc = -ENOENT;
+ if (kernfs_get_active(parent)) {
+ kernfs_addrm_start(&acxt);
+ rc = kernfs_add_one(&acxt, kn, parent);
+ kernfs_addrm_finish(&acxt);
+ kernfs_put_active(parent);
+ }
+
if (rc) {
kernfs_put(kn);
return ERR_PTR(rc);
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 4bc5784..e9ec38c 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -46,6 +46,13 @@
}
/*
+ * Context structure to be used while adding/removing nodes.
+ */
+struct kernfs_addrm_cxt {
+ struct kernfs_node *removed;
+};
+
+/*
* mount.c
*/
struct kernfs_super_info {
@@ -94,7 +101,10 @@
struct kernfs_node *kernfs_get_active(struct kernfs_node *kn);
void kernfs_put_active(struct kernfs_node *kn);
-int kernfs_add_one(struct kernfs_node *kn, struct kernfs_node *parent);
+void kernfs_addrm_start(struct kernfs_addrm_cxt *acxt);
+int kernfs_add_one(struct kernfs_addrm_cxt *acxt, struct kernfs_node *kn,
+ struct kernfs_node *parent);
+void kernfs_addrm_finish(struct kernfs_addrm_cxt *acxt);
struct kernfs_node *kernfs_new_node(struct kernfs_root *root, const char *name,
umode_t mode, unsigned flags);
diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
index 3a939c263..b2c106c 100644
--- a/fs/kernfs/symlink.c
+++ b/fs/kernfs/symlink.c
@@ -27,6 +27,7 @@
struct kernfs_node *target)
{
struct kernfs_node *kn;
+ struct kernfs_addrm_cxt acxt;
int error;
kn = kernfs_new_node(kernfs_root(parent), name, S_IFLNK|S_IRWXUGO,
@@ -39,7 +40,14 @@
kn->symlink.target_kn = target;
kernfs_get(target); /* ref owned by symlink */
- error = kernfs_add_one(kn, parent);
+ error = -ENOENT;
+ if (kernfs_get_active(parent)) {
+ kernfs_addrm_start(&acxt);
+ error = kernfs_add_one(&acxt, kn, parent);
+ kernfs_addrm_finish(&acxt);
+ kernfs_put_active(parent);
+ }
+
if (!error)
return kn;
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 9b5a4bb..61bd7ae 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -89,6 +89,10 @@
struct rb_node rb;
+ union {
+ struct kernfs_node *removed_list;
+ } u;
+
const void *ns; /* namespace tag */
unsigned int hash; /* ns + name hash */
union {