From ff09297ec9964b3fe4bade77c92c75fde34fa8e9 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 24 Jul 2019 23:01:55 -0400 Subject: [PATCH 1/3] autofs: simplify get_next_positive_...(), get rid of trylocks * new helper: positive_after(parent, child); parent->d_lock is held by caller, grabs and returns the first thing after child in the list of children that has simple_positive() true. NULL if nothing's found; NULL child == search the entire list. * get_next_positive_subdir() loses the redundant check for d_count and switches to use of that helper. BTW, dput(NULL) is a no-op for a good reason... * get_next_positive_dentry() switched to the same helper. Logics: look for positive child in prev; if not found, look for the positive child of prev's parent following prev, etc. That way we are guaranteed that we are only moving rootwards through the ancestors of prev, which is pinned and thus not going anywhere. Since ->d_parent on autofs never changes, the same goes for the entire chain of ancestors and we don't need overlapping ->d_lock on them. Which avoids the trylock loops, in addition to simplifying the logics in there... Signed-off-by: Al Viro --- fs/autofs/expire.c | 103 ++++++++++++++------------------------------- 1 file changed, 32 insertions(+), 71 deletions(-) diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c index cdff0567aacb..2866fabf497f 100644 --- a/fs/autofs/expire.c +++ b/fs/autofs/expire.c @@ -70,6 +70,27 @@ done: return status; } +/* p->d_lock held */ +static struct dentry *positive_after(struct dentry *p, struct dentry *child) +{ + if (child) + child = list_next_entry(child, d_child); + else + child = list_first_entry(&p->d_subdirs, struct dentry, d_child); + + list_for_each_entry_from(child, &p->d_subdirs, d_child) { + spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED); + if (simple_positive(child)) { + dget_dlock(child); + spin_unlock(&child->d_lock); + return child; + } + spin_unlock(&child->d_lock); + } + + return NULL; +} + /* * Calculate and dget next entry in the subdirs list under root. */ @@ -77,43 +98,14 @@ static struct dentry *get_next_positive_subdir(struct dentry *prev, struct dentry *root) { struct autofs_sb_info *sbi = autofs_sbi(root->d_sb); - struct list_head *next; struct dentry *q; spin_lock(&sbi->lookup_lock); spin_lock(&root->d_lock); - - if (prev) - next = prev->d_child.next; - else { - prev = dget_dlock(root); - next = prev->d_subdirs.next; - } - -cont: - if (next == &root->d_subdirs) { - spin_unlock(&root->d_lock); - spin_unlock(&sbi->lookup_lock); - dput(prev); - return NULL; - } - - q = list_entry(next, struct dentry, d_child); - - spin_lock_nested(&q->d_lock, DENTRY_D_LOCK_NESTED); - /* Already gone or negative dentry (under construction) - try next */ - if (!d_count(q) || !simple_positive(q)) { - spin_unlock(&q->d_lock); - next = q->d_child.next; - goto cont; - } - dget_dlock(q); - spin_unlock(&q->d_lock); + q = positive_after(root, prev); spin_unlock(&root->d_lock); spin_unlock(&sbi->lookup_lock); - dput(prev); - return q; } @@ -124,59 +116,28 @@ static struct dentry *get_next_positive_dentry(struct dentry *prev, struct dentry *root) { struct autofs_sb_info *sbi = autofs_sbi(root->d_sb); - struct list_head *next; - struct dentry *p, *ret; + struct dentry *p = prev, *ret = NULL, *d = NULL; if (prev == NULL) return dget(root); spin_lock(&sbi->lookup_lock); -relock: - p = prev; spin_lock(&p->d_lock); -again: - next = p->d_subdirs.next; - if (next == &p->d_subdirs) { - while (1) { - struct dentry *parent; + while (1) { + struct dentry *parent; - if (p == root) { - spin_unlock(&p->d_lock); - spin_unlock(&sbi->lookup_lock); - dput(prev); - return NULL; - } - - parent = p->d_parent; - if (!spin_trylock(&parent->d_lock)) { - spin_unlock(&p->d_lock); - cpu_relax(); - goto relock; - } - spin_unlock(&p->d_lock); - next = p->d_child.next; - p = parent; - if (next != &parent->d_subdirs) - break; - } - } - ret = list_entry(next, struct dentry, d_child); - - spin_lock_nested(&ret->d_lock, DENTRY_D_LOCK_NESTED); - /* Negative dentry - try next */ - if (!simple_positive(ret)) { + ret = positive_after(p, d); + if (ret || p == root) + break; + parent = p->d_parent; spin_unlock(&p->d_lock); - lock_set_subclass(&ret->d_lock.dep_map, 0, _RET_IP_); - p = ret; - goto again; + spin_lock(&parent->d_lock); + d = p; + p = parent; } - dget_dlock(ret); - spin_unlock(&ret->d_lock); spin_unlock(&p->d_lock); spin_unlock(&sbi->lookup_lock); - dput(prev); - return ret; } From c4931db9b08c18005fb21ab201e7137ba0547df5 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sat, 27 Jul 2019 10:00:33 -0400 Subject: [PATCH 2/3] get rid of autofs_info->active_count autofs_add_active() is always called only once (and on a dentry with freshly allocated ino, at that). autofs_del_active() is never called more than once. Signed-off-by: Al Viro --- fs/autofs/autofs_i.h | 1 - fs/autofs/root.c | 33 ++++++--------------------------- 2 files changed, 6 insertions(+), 28 deletions(-) diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h index 8c0c11181fad..8bcec8dcabb6 100644 --- a/fs/autofs/autofs_i.h +++ b/fs/autofs/autofs_i.h @@ -58,7 +58,6 @@ struct autofs_info { struct completion expire_complete; struct list_head active; - int active_count; struct list_head expiring; diff --git a/fs/autofs/root.c b/fs/autofs/root.c index e646569c75ed..64f974c61068 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -60,38 +60,15 @@ const struct dentry_operations autofs_dentry_operations = { .d_release = autofs_dentry_release, }; -static void autofs_add_active(struct dentry *dentry) -{ - struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb); - struct autofs_info *ino; - - ino = autofs_dentry_ino(dentry); - if (ino) { - spin_lock(&sbi->lookup_lock); - if (!ino->active_count) { - if (list_empty(&ino->active)) - list_add(&ino->active, &sbi->active_list); - } - ino->active_count++; - spin_unlock(&sbi->lookup_lock); - } -} - static void autofs_del_active(struct dentry *dentry) { struct autofs_sb_info *sbi = autofs_sbi(dentry->d_sb); struct autofs_info *ino; ino = autofs_dentry_ino(dentry); - if (ino) { - spin_lock(&sbi->lookup_lock); - ino->active_count--; - if (!ino->active_count) { - if (!list_empty(&ino->active)) - list_del_init(&ino->active); - } - spin_unlock(&sbi->lookup_lock); - } + spin_lock(&sbi->lookup_lock); + list_del_init(&ino->active); + spin_unlock(&sbi->lookup_lock); } static int autofs_dir_open(struct inode *inode, struct file *file) @@ -539,7 +516,9 @@ static struct dentry *autofs_lookup(struct inode *dir, dentry->d_fsdata = ino; ino->dentry = dentry; - autofs_add_active(dentry); + spin_lock(&sbi->lookup_lock); + list_add(&ino->active, &sbi->active_list); + spin_unlock(&sbi->lookup_lock); } return NULL; } From 5f68056ca50fdd3954a93ae66fea7452abddb66f Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sat, 27 Jul 2019 10:03:14 -0400 Subject: [PATCH 3/3] autofs_lookup(): hold ->d_lock over playing with ->d_flags ... as well as setting ->d_fsdata, etc. Make all of that atomic. Signed-off-by: Al Viro --- fs/autofs/root.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 64f974c61068..29abafc0ce31 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -504,21 +504,22 @@ static struct dentry *autofs_lookup(struct inode *dir, if (!autofs_oz_mode(sbi) && !IS_ROOT(dentry->d_parent)) return ERR_PTR(-ENOENT); - /* Mark entries in the root as mount triggers */ - if (IS_ROOT(dentry->d_parent) && - autofs_type_indirect(sbi->type)) - __managed_dentry_set_managed(dentry); - ino = autofs_new_ino(sbi); if (!ino) return ERR_PTR(-ENOMEM); + spin_lock(&sbi->lookup_lock); + spin_lock(&dentry->d_lock); + /* Mark entries in the root as mount triggers */ + if (IS_ROOT(dentry->d_parent) && + autofs_type_indirect(sbi->type)) + __managed_dentry_set_managed(dentry); dentry->d_fsdata = ino; ino->dentry = dentry; - spin_lock(&sbi->lookup_lock); list_add(&ino->active, &sbi->active_list); spin_unlock(&sbi->lookup_lock); + spin_unlock(&dentry->d_lock); } return NULL; }