From 7653fe9d6cddc3fc5e4220608079006d8ac0054c Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Mon, 13 Jan 2014 14:20:56 -0800 Subject: [PATCH] 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 Signed-off-by: Greg Kroah-Hartman --- fs/kernfs/dir.c | 114 ++++++++++++++++++++++++++++-------- fs/kernfs/file.c | 10 +++- fs/kernfs/kernfs-internal.h | 12 +++- fs/kernfs/symlink.c | 10 +++- include/linux/kernfs.h | 4 ++ 5 files changed, 121 insertions(+), 29 deletions(-) diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c index 770d687ee9f3..f878e4f2efe7 100644 --- a/fs/kernfs/dir.c +++ b/fs/kernfs/dir.c @@ -398,8 +398,29 @@ struct kernfs_node *kernfs_new_node(struct kernfs_root *root, const char *name, return NULL; } +/** + * 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 @@ struct kernfs_node *kernfs_new_node(struct kernfs_root *root, const char *name, * 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 @@ int kernfs_add_one(struct kernfs_node *kn, struct kernfs_node *parent) 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 @@ int kernfs_add_one(struct kernfs_node *kn, struct kernfs_node *parent) /* 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 @@ struct kernfs_node *kernfs_create_dir_ns(struct kernfs_node *parent, 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 @@ struct kernfs_node *kernfs_create_dir_ns(struct kernfs_node *parent, 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_deactivate(struct kernfs_node *kn) } } -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 @@ static void __kernfs_remove(struct kernfs_node *kn) 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 @@ static void __kernfs_remove(struct kernfs_node *kn) */ 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 @@ void kernfs_remove(struct kernfs_node *kn) 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 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name, 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 ffe1bebf9197..404ffd2f27bc 100644 --- a/fs/kernfs/file.c +++ b/fs/kernfs/file.c @@ -817,6 +817,7 @@ struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, 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 @@ struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent, 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 4bc57848076c..e9ec38c86074 100644 --- a/fs/kernfs/kernfs-internal.h +++ b/fs/kernfs/kernfs-internal.h @@ -45,6 +45,13 @@ static inline struct kernfs_root *kernfs_root(struct kernfs_node *kn) return kn->dir.root; } +/* + * Context structure to be used while adding/removing nodes. + */ +struct kernfs_addrm_cxt { + struct kernfs_node *removed; +}; + /* * mount.c */ @@ -94,7 +101,10 @@ extern const struct inode_operations kernfs_dir_iops; 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 3a939c263ede..b2c106ca3434 100644 --- a/fs/kernfs/symlink.c +++ b/fs/kernfs/symlink.c @@ -27,6 +27,7 @@ struct kernfs_node *kernfs_create_link(struct kernfs_node *parent, 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 @@ struct kernfs_node *kernfs_create_link(struct kernfs_node *parent, 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 9b5a4bb88c64..61bd7ae6b8e0 100644 --- a/include/linux/kernfs.h +++ b/include/linux/kernfs.h @@ -89,6 +89,10 @@ struct kernfs_node { struct rb_node rb; + union { + struct kernfs_node *removed_list; + } u; + const void *ns; /* namespace tag */ unsigned int hash; /* ns + name hash */ union {