diff mbox series

[3/7] proc: Mov rcu_read_(lock|unlock) in proc_prune_siblings_dcache

Message ID 87h7zl9e7u.fsf_-_@x220.int.ebiederm.org (mailing list archive)
State New, archived
Headers show
Series [1/7] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes | expand

Commit Message

Eric W. Biederman Feb. 20, 2020, 8:49 p.m. UTC 
Don't make it look like rcu_read_lock is held over the entire loop
instead just take the rcu_read_lock over the part of the loop that
matters.  This makes the intent of the code a little clearer.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/proc/inode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Linus Torvalds Feb. 20, 2020, 10:33 p.m. UTC | #1 
On Thu, Feb 20, 2020 at 12:51 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Don't make it look like rcu_read_lock is held over the entire loop
> instead just take the rcu_read_lock over the part of the loop that
> matters.  This makes the intent of the code a little clearer.

No, this is horrid.

Maybe it makes the intent clearer, but it also causes that "continue"
case to unlock and relock immediately.

And maybe that case never triggers, and that's ok. But then it needs a
big comment about it.

              Linus
diff mbox series

Patch

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 74ce4a8d05eb..38a7baa41aba 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -110,11 +110,13 @@  void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock)
 	struct hlist_node *node;
 	struct super_block *sb;
 
-	rcu_read_lock();
 	for (;;) {
+		rcu_read_lock();
 		node = hlist_first_rcu(inodes);
-		if (!node)
+		if (!node) {
+			rcu_read_unlock();
 			break;
+		}
 		ei = hlist_entry(node, struct proc_inode, sibling_inodes);
 		spin_lock(lock);
 		hlist_del_init_rcu(&ei->sibling_inodes);
@@ -122,23 +124,21 @@  void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock)
 
 		inode = &ei->vfs_inode;
 		sb = inode->i_sb;
-		if (!atomic_inc_not_zero(&sb->s_active))
+		if (!atomic_inc_not_zero(&sb->s_active)) {
+			rcu_read_unlock();
 			continue;
+		}
 		inode = igrab(inode);
 		rcu_read_unlock();
 		if (unlikely(!inode)) {
 			deactivate_super(sb);
-			rcu_read_lock();
 			continue;
 		}
 
 		d_prune_aliases(inode);
 		iput(inode);
 		deactivate_super(sb);
-
-		rcu_read_lock();
 	}
-	rcu_read_unlock();
 }
 
 static int proc_show_options(struct seq_file *seq, struct dentry *root)