Invalidate_inodes can be very slow

PATCH In review Kirill Korotaev 4 participants 13 messages Oct 13, 2003 VFS

lore

Share & tools

lore

Kirill Korotaev

author

2003-10-13 09:17 UTC #0 lore raw

PATCH 1/1 Invalidate_inodes can be very slow

I'm not sure whom this patch should be send to, so I'm posting it here.

this patch fixes the problem that huge inode cache
can make invalidate_inodes() calls very long. Meanwhile lock_kernel() and inode_lock are being held and system
can freeze for seconds.
I detected this problem on kernel with 4gb split. When inode cache was 2.5Gb (1,000,000 of inodes in cache) it took seconds to umount or turn quota off.
This patch is against 2.4.22 kernel. But as far as I can see the problem still exists in 2.6 kernels.
This patch introduces per-super block inode list which makes possible to scan sb's only inodes when doing umount.

Kirill

fs/super.c renamed from fs/super.c.ii +1 -0

@@ -271,6 +271,7 @@ static struct super_block *alloc_super(v
 		INIT_LIST_HEAD(&s->s_locked_inodes);
 		INIT_LIST_HEAD(&s->s_files);
 		INIT_LIST_HEAD(&s->s_instances);
+		INIT_LIST_HEAD(&s->s_inodes);
 		init_rwsem(&s->s_umount);
 		sema_init(&s->s_lock, 1);
 		down_write(&s->s_umount);

fs/inode.c renamed from fs/inode.c.ii +11 ~~-26~~

@@ -604,7 +604,7 @@ static void dispose_list(struct list_hea
 /*
  * Invalidate all inodes for a device.
  */
-static int invalidate_list(struct list_head *head, struct super_block * sb, struct list_head * dispose)
+static int invalidate_list(struct list_head *head, struct list_head * dispose)
 {
 	struct list_head *next;
 	int busy = 0, count = 0;

@@ -617,12 +617,11 @@ static int invalidate_list(struct list_h
 		next = next->next;
 		if (tmp == head)
 			break;
-		inode = list_entry(tmp, struct inode, i_list);
-		if (inode->i_sb != sb)
-			continue;
+		inode = list_entry(tmp, struct inode, i_sb_list);
 		invalidate_inode_buffers(inode);
 		if (!atomic_read(&inode->i_count)) {
 			list_del_init(&inode->i_hash);
+			list_del(&inode->i_sb_list);
 			list_del(&inode->i_list);
 			list_add(&inode->i_list, dispose);
 			inode->i_state |= I_FREEING;

@@ -659,10 +658,7 @@ int invalidate_inodes(struct super_block
 	LIST_HEAD(throw_away);
 
 	spin_lock(&inode_lock);
-	busy = invalidate_list(&inode_in_use, sb, &throw_away);
-	busy |= invalidate_list(&inode_unused, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_dirty, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_locked_inodes, sb, &throw_away);
+	busy = invalidate_list(&sb->s_inodes, &throw_away);
 	spin_unlock(&inode_lock);
 
 	dispose_list(&throw_away);

@@ -738,6 +734,7 @@ void prune_icache(int goal)
 		list_del(tmp);
 		list_del(&inode->i_hash);
 		INIT_LIST_HEAD(&inode->i_hash);
+		list_del(&inode->i_sb_list);
 		list_add(tmp, freeable);
 		inode->i_state |= I_FREEING;
 		count++;

@@ -827,6 +824,7 @@ struct inode * new_inode(struct super_bl
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
+		list_add(&inode->i_sb_list, &sb->s_inodes);
 		inode->i_ino = ++last_ino;
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);

@@ -854,6 +852,7 @@ static struct inode * get_new_inode(stru
 		if (!old) {
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
+			list_add(&inode->i_sb_list, &sb->s_inodes);
 			list_add(&inode->i_hash, head);
 			inode->i_ino = ino;
 			inode->i_state = I_LOCK;

@@ -1046,6 +1045,7 @@ void iput(struct inode *inode)
 			INIT_LIST_HEAD(&inode->i_hash);
 			list_del(&inode->i_list);
 			INIT_LIST_HEAD(&inode->i_list);
+			list_del(&inode->i_sb_list);
 			inode->i_state|=I_FREEING;
 			inodes_stat.nr_inodes--;
 			spin_unlock(&inode_lock);

@@ -1079,6 +1079,7 @@ void iput(struct inode *inode)
 				list_del_init(&inode->i_hash);
 			}
 			list_del_init(&inode->i_list);
+			list_del(&inode->i_sb_list);
 			inode->i_state|=I_FREEING;
 			inodes_stat.nr_inodes--;
 			spin_unlock(&inode_lock);

@@ -1239,26 +1240,10 @@ void remove_dquot_ref(struct super_block
 	lock_kernel();		/* This lock is for quota code */
 	spin_lock(&inode_lock);	/* This lock is for inodes code */
  
-	list_for_each(act_head, &inode_in_use) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &inode_unused) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_dirty) {
-		inode = list_entry(act_head, struct inode, i_list);
+	list_for_each_entry(inode, act_head, &sb->s_inodes)
 		if (IS_QUOTAINIT(inode))
 			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_locked_inodes) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
+
 	spin_unlock(&inode_lock);
 	unlock_kernel();

include/linux/fs.h renamed from include/linux/fs.h.ii +2 -0

@@ -438,6 +438,7 @@ struct block_device {
 struct inode {
 	struct list_head	i_hash;
 	struct list_head	i_list;
+	struct list_head	i_sb_list;
 	struct list_head	i_dentry;
 	
 	struct list_head	i_dirty_buffers;

@@ -756,6 +757,7 @@ struct super_block {
 	int			s_count;
 	atomic_t		s_active;
 
+	struct list_head	s_inodes;	/* all inodes */
 	struct list_head	s_dirty;	/* dirty inodes */
 	struct list_head	s_locked_inodes;/* inodes being synced */
 	struct list_head	s_files;

William Lee Irwin III

2003-10-13 09:51 UTC #1 lore raw

Quoted 4 lines · Kirill Korotaev

I'm not sure whom this patch should be send to, so I'm posting it here. this patch fixes the problem that huge inode cache
can make invalidate_inodes() calls very long. Meanwhile lock_kernel() and inode_lock are being held and system

Quoted 8 lines

can freeze for seconds.
I detected this problem on kernel with 4gb split. When inode cache was 2.5Gb (1,000,000 of inodes in cache) it took seconds to umount or turn quota off.
This patch is against 2.4.22 kernel. But as far as I can see the problem still exists in 2.6 kernels.
This patch introduces per-super block inode list which makes possible to scan sb's only inodes when doing umount.

Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the locking is correct or if list movement is done in all needed places.

fs/inode.c +12 ~~-28~~

diff -prauN linux-2.6.0-test7-bk4/fs/inode.c inode-2.6.0-test7-bk4-1/fs/inode.c
@@ -285,7 +285,7 @@ static void dispose_list(struct list_hea
 /*
  * Invalidate all inodes for a device.
  */
-static int invalidate_list(struct list_head *head, struct super_block * sb, struct list_head * dispose)
+static int invalidate_list(struct list_head *head, struct list_head *dispose)
 {
 	struct list_head *next;
 	int busy = 0, count = 0;

@@ -298,13 +298,11 @@ static int invalidate_list(struct list_h
 		next = next->next;
 		if (tmp == head)
 			break;
-		inode = list_entry(tmp, struct inode, i_list);
-		if (inode->i_sb != sb)
-			continue;
+		inode = list_entry(tmp, struct inode, i_sb_list);
 		invalidate_inode_buffers(inode);
 		if (!atomic_read(&inode->i_count)) {
 			hlist_del_init(&inode->i_hash);
-			list_del(&inode->i_list);
+			list_del(&inode->i_sb_list);
 			list_add(&inode->i_list, dispose);
 			inode->i_state |= I_FREEING;
 			count++;

@@ -340,10 +338,7 @@ int invalidate_inodes(struct super_block
 
 	down(&iprune_sem);
 	spin_lock(&inode_lock);
-	busy = invalidate_list(&inode_in_use, sb, &throw_away);
-	busy |= invalidate_list(&inode_unused, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_dirty, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_io, sb, &throw_away);
+	busy = invalidate_list(&sb->s_inodes, &throw_away);
 	spin_unlock(&inode_lock);
 
 	dispose_list(&throw_away);

@@ -443,6 +438,7 @@ static void prune_icache(int nr_to_scan)
 				continue;
 		}
 		hlist_del_init(&inode->i_hash);
+		list_del_init(&inode->i_sb_list);
 		list_move(&inode->i_list, &freeable);
 		inode->i_state |= I_FREEING;
 		nr_pruned++;

@@ -553,6 +549,7 @@ struct inode *new_inode(struct super_blo
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
+		list_add(&inode->i_sb_list, &sb->s_inodes);
 		inode->i_ino = ++last_ino;
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);

@@ -601,6 +598,7 @@ static struct inode * get_new_inode(stru
 
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
+			list_add(&inode->i_sb_list, &sb->s_inodes);
 			hlist_add_head(&inode->i_hash, head);
 			inode->i_state = I_LOCK|I_NEW;
 			spin_unlock(&inode_lock);

@@ -984,6 +982,7 @@ void generic_delete_inode(struct inode *
 	struct super_operations *op = inode->i_sb->s_op;
 
 	list_del_init(&inode->i_list);
+	list_del_init(&inode->i_sb_list);
 	inode->i_state|=I_FREEING;
 	inodes_stat.nr_inodes--;
 	spin_unlock(&inode_lock);

@@ -1031,6 +1030,7 @@ static void generic_forget_inode(struct 
 		hlist_del_init(&inode->i_hash);
 	}
 	list_del_init(&inode->i_list);
+	list_del_init(&inode->i_sb_list);
 	inode->i_state|=I_FREEING;
 	inodes_stat.nr_inodes--;
 	spin_unlock(&inode_lock);

@@ -1228,27 +1228,11 @@ void remove_dquot_ref(struct super_block
 		return;	/* nothing to do */
 	spin_lock(&inode_lock);	/* This lock is for inodes code */
 	/* We don't have to lock against quota code - test IS_QUOTAINIT is just for speedup... */
- 
-	list_for_each(act_head, &inode_in_use) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &inode_unused) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_dirty) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_io) {
-		inode = list_entry(act_head, struct inode, i_list);
+
+	list_for_each_entry(inode, &sb->s_inodes)
 		if (IS_QUOTAINIT(inode))
 			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
+ 
 	spin_unlock(&inode_lock);
 
 	put_dquot_list(&tofree_head);

fs/super.c +1 -0

diff -prauN linux-2.6.0-test7-bk4/fs/super.c inode-2.6.0-test7-bk4-1/fs/super.c
@@ -66,6 +66,7 @@ static struct super_block *alloc_super(v
 		INIT_LIST_HEAD(&s->s_files);
 		INIT_LIST_HEAD(&s->s_instances);
 		INIT_HLIST_HEAD(&s->s_anon);
+		INIT_LIST_HEAD(&s->s_inodes);
 		init_rwsem(&s->s_umount);
 		sema_init(&s->s_lock, 1);
 		down_write(&s->s_umount);

include/linux/fs.h +2 -0

diff -prauN linux-2.6.0-test7-bk4/include/linux/fs.h inode-2.6.0-test7-bk4-1/include/linux/fs.h
@@ -369,6 +369,7 @@ struct block_device {
 struct inode {
 	struct hlist_node	i_hash;
 	struct list_head	i_list;
+	struct list_head	i_sb_list;
 	struct list_head	i_dentry;
 	unsigned long		i_ino;
 	atomic_t		i_count;

@@ -687,6 +688,7 @@ struct super_block {
 	atomic_t		s_active;
 	void                    *s_security;
 
+	struct list_head	s_inodes;	/* all inodes */
 	struct list_head	s_dirty;	/* dirty inodes */
 	struct list_head	s_io;		/* parked for writeback */
 	struct hlist_head	s_anon;		/* anonymous dentries for (nfs) exporting */

Kirill Korotaev

author

2003-10-13 10:45 UTC #2 lore raw

Quoted 2 lines

Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the locking is correct or if list movement is done in all needed places.

First of all, thanks for forward port. I think this patch is quite usefull for both 2.4 and 2.6 kernels, but I can't check it on all kernels myself.

comments:
1. why have you replaced list_del with list_del_init in my patch? It's not required.
2. locks are ok, as for i_list, i_hash lists.
3. Missed
+list_add(&inode->i_sb_list, &sb->s_inodes);
in get_new_inode_fast() function. Please add it.

I looked through, everything else looks ok.

[your patch]

Kirill

William Lee Irwin III

2003-10-13 11:04 UTC #3 lore raw

Quoted 2 lines · At some point in the past, I wrote:

Quoted 2 lines
Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the locking is correct or if list movement is done in all needed places.

Quoted 11 lines · Kirill Korotaev

First of all, thanks for forward port. I think this patch is quite usefull for both 2.4 and 2.6 kernels, but I can't check it on all kernels myself. comments:
1. why have you replaced list_del with list_del_init in my patch? It's not required.
2. locks are ok, as for i_list, i_hash lists.
3. Missed
+list_add(&inode->i_sb_list, &sb->s_inodes);
in get_new_inode_fast() function. Please add it.
I looked through, everything else looks ok.
[your patch]

list_del_init() was for consistency with the surrounding ->i_list deletions, hence erring on the side of caution.
Amended patch below, addressing point 3.

fs/inode.c +13 ~~-28~~

diff -prauN linux-2.6.0-test7-bk4/fs/inode.c inode-2.6.0-test7-bk4-1/fs/inode.c
@@ -285,7 +285,7 @@ static void dispose_list(struct list_hea
 /*
  * Invalidate all inodes for a device.
  */
-static int invalidate_list(struct list_head *head, struct super_block * sb, struct list_head * dispose)
+static int invalidate_list(struct list_head *head, struct list_head *dispose)
 {
 	struct list_head *next;
 	int busy = 0, count = 0;

@@ -298,13 +298,11 @@ static int invalidate_list(struct list_h
 		next = next->next;
 		if (tmp == head)
 			break;
-		inode = list_entry(tmp, struct inode, i_list);
-		if (inode->i_sb != sb)
-			continue;
+		inode = list_entry(tmp, struct inode, i_sb_list);
 		invalidate_inode_buffers(inode);
 		if (!atomic_read(&inode->i_count)) {
 			hlist_del_init(&inode->i_hash);
-			list_del(&inode->i_list);
+			list_del(&inode->i_sb_list);
 			list_add(&inode->i_list, dispose);
 			inode->i_state |= I_FREEING;
 			count++;

@@ -340,10 +338,7 @@ int invalidate_inodes(struct super_block
 
 	down(&iprune_sem);
 	spin_lock(&inode_lock);
-	busy = invalidate_list(&inode_in_use, sb, &throw_away);
-	busy |= invalidate_list(&inode_unused, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_dirty, sb, &throw_away);
-	busy |= invalidate_list(&sb->s_io, sb, &throw_away);
+	busy = invalidate_list(&sb->s_inodes, &throw_away);
 	spin_unlock(&inode_lock);
 
 	dispose_list(&throw_away);

@@ -443,6 +438,7 @@ static void prune_icache(int nr_to_scan)
 				continue;
 		}
 		hlist_del_init(&inode->i_hash);
+		list_del_init(&inode->i_sb_list);
 		list_move(&inode->i_list, &freeable);
 		inode->i_state |= I_FREEING;
 		nr_pruned++;

@@ -553,6 +549,7 @@ struct inode *new_inode(struct super_blo
 		spin_lock(&inode_lock);
 		inodes_stat.nr_inodes++;
 		list_add(&inode->i_list, &inode_in_use);
+		list_add(&inode->i_sb_list, &sb->s_inodes);
 		inode->i_ino = ++last_ino;
 		inode->i_state = 0;
 		spin_unlock(&inode_lock);

@@ -601,6 +598,7 @@ static struct inode * get_new_inode(stru
 
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
+			list_add(&inode->i_sb_list, &sb->s_inodes);
 			hlist_add_head(&inode->i_hash, head);
 			inode->i_state = I_LOCK|I_NEW;
 			spin_unlock(&inode_lock);

@@ -649,6 +647,7 @@ static struct inode * get_new_inode_fast
 			inode->i_ino = ino;
 			inodes_stat.nr_inodes++;
 			list_add(&inode->i_list, &inode_in_use);
+			list_add(&inode->i_sb_list, &sb->s_inodes);
 			hlist_add_head(&inode->i_hash, head);
 			inode->i_state = I_LOCK|I_NEW;
 			spin_unlock(&inode_lock);

@@ -984,6 +983,7 @@ void generic_delete_inode(struct inode *
 	struct super_operations *op = inode->i_sb->s_op;
 
 	list_del_init(&inode->i_list);
+	list_del_init(&inode->i_sb_list);
 	inode->i_state|=I_FREEING;
 	inodes_stat.nr_inodes--;
 	spin_unlock(&inode_lock);

@@ -1031,6 +1031,7 @@ static void generic_forget_inode(struct 
 		hlist_del_init(&inode->i_hash);
 	}
 	list_del_init(&inode->i_list);
+	list_del_init(&inode->i_sb_list);
 	inode->i_state|=I_FREEING;
 	inodes_stat.nr_inodes--;
 	spin_unlock(&inode_lock);

@@ -1228,27 +1229,11 @@ void remove_dquot_ref(struct super_block
 		return;	/* nothing to do */
 	spin_lock(&inode_lock);	/* This lock is for inodes code */
 	/* We don't have to lock against quota code - test IS_QUOTAINIT is just for speedup... */
- 
-	list_for_each(act_head, &inode_in_use) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &inode_unused) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (inode->i_sb == sb && IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_dirty) {
-		inode = list_entry(act_head, struct inode, i_list);
-		if (IS_QUOTAINIT(inode))
-			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
-	list_for_each(act_head, &sb->s_io) {
-		inode = list_entry(act_head, struct inode, i_list);
+
+	list_for_each_entry(inode, &sb->s_inodes)
 		if (IS_QUOTAINIT(inode))
 			remove_inode_dquot_ref(inode, type, &tofree_head);
-	}
+ 
 	spin_unlock(&inode_lock);
 
 	put_dquot_list(&tofree_head);

fs/super.c +1 -0

diff -prauN linux-2.6.0-test7-bk4/fs/super.c inode-2.6.0-test7-bk4-1/fs/super.c
@@ -66,6 +66,7 @@ static struct super_block *alloc_super(v
 		INIT_LIST_HEAD(&s->s_files);
 		INIT_LIST_HEAD(&s->s_instances);
 		INIT_HLIST_HEAD(&s->s_anon);
+		INIT_LIST_HEAD(&s->s_inodes);
 		init_rwsem(&s->s_umount);
 		sema_init(&s->s_lock, 1);
 		down_write(&s->s_umount);

include/linux/fs.h +2 -0

diff -prauN linux-2.6.0-test7-bk4/include/linux/fs.h inode-2.6.0-test7-bk4-1/include/linux/fs.h
@@ -369,6 +369,7 @@ struct block_device {
 struct inode {
 	struct hlist_node	i_hash;
 	struct list_head	i_list;
+	struct list_head	i_sb_list;
 	struct list_head	i_dentry;
 	unsigned long		i_ino;
 	atomic_t		i_count;

@@ -687,6 +688,7 @@ struct super_block {
 	atomic_t		s_active;
 	void                    *s_security;
 
+	struct list_head	s_inodes;	/* all inodes */
 	struct list_head	s_dirty;	/* dirty inodes */
 	struct list_head	s_io;		/* parked for writeback */
 	struct hlist_head	s_anon;		/* anonymous dentries for (nfs) exporting */

Andrew Morton

2003-10-13 11:05 UTC #4 lore raw

In reply to William Lee Irwin III

Quoted 3 lines · William Lee Irwin III

Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the
locking is correct or if list movement is done in all needed places.

My preferred approach to this would be to move all the global inode lists into the superblock so both they and inode_lock become per-sb.

It is a big change though. And, amazingly, nobody has yet hit sufficient inode_lock contention to warrant it.

Yes, I bet that this search will hurt like hell on a really big box which has thousands of auto-expiring NFS mounts. Please test your patch and I'll queue it up while we think about it some more.

William Lee Irwin III

2003-10-13 11:16 UTC #5 lore raw

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines
Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the
locking is correct or if list movement is done in all needed places.

Quoted 7 lines · Andrew Morton

My preferred approach to this would be to move all the global inode lists into the superblock so both they and inode_lock become per-sb. It is a big change though. And, amazingly, nobody has yet hit sufficient inode_lock contention to warrant it.
Yes, I bet that this search will hurt like hell on a really big box which has thousands of auto-expiring NFS mounts. Please test your patch and I'll queue it up while we think about it some more.

Generally dcache_lock stands in front of inode_lock, even with the current hashtable RCU code. inode_lock has been seen before in unusual situations I don't remember offhand, though generally it's not #1. The workloads used for, say, benchmark testing don't adequately model situations like what you just mentioned (or a number of other real-life usage cases), so per-sb inode_lock may be worth considering on a priori grounds, though it would probably be better to actually set something up to test that scenario.

-- wli

Kirill Korotaev

author

2003-10-13 11:44 UTC #6 lore raw

Quoted 13 lines

Quoted 2 lines · William Lee Irwin III
Quoted 2 lines
Untested brute-force forward port to 2.6.0-test7-bk4. No idea if the
locking is correct or if list movement is done in all needed places.
Quoted 7 lines · Andrew Morton
My preferred approach to this would be to move all the global inode lists into the superblock so both they and inode_lock become per-sb. It is a big change though. And, amazingly, nobody has yet hit sufficient inode_lock contention to warrant it.
Yes, I bet that this search will hurt like hell on a really big box which has thousands of auto-expiring NFS mounts. Please test your patch and I'll queue it up while we think about it some more.

Quoted 8 lines

Generally dcache_lock stands in front of inode_lock, even with the current hashtable RCU code. inode_lock has been seen before in unusual situations I don't remember offhand, though generally it's not #1. The workloads used for, say, benchmark testing don't adequately model situations like what you just mentioned (or a number of other real-life usage cases), so per-sb inode_lock may be worth considering on a priori grounds, though it would probably be better to actually set something up to test that scenario.

This patch can be tested quite easily. Main changes are in invalidate_list. This path can be triggered by umount/quota off.
So I tested it the following way:
1. mounting/working/umounting partition (and turning quota on/off)
2. running memeater to call prune_icache when partition was mounted to test that lists are ok.

All other places should be ok, since simple list_add/list_del is inserted.

Kirill

William Lee Irwin III

2003-10-13 11:51 UTC #7 lore raw

Quoted 8 lines · William Lee Irwin III

Quoted 8 lines
Generally dcache_lock stands in front of inode_lock, even with the current hashtable RCU code. inode_lock has been seen before in unusual situations I don't remember offhand, though generally it's not #1. The workloads used for, say, benchmark testing don't adequately model situations like what you just mentioned (or a number of other real-life usage cases), so per-sb inode_lock may be worth considering on a priori grounds, though it would probably be better to actually set something up to test that scenario.

Quoted 7 lines · Kirill Korotaev

This patch can be tested quite easily. Main changes are in invalidate_list. This path can be triggered by umount/quota off.
So I tested it the following way:
1. mounting/working/umounting partition (and turning quota on/off)
2. running memeater to call prune_icache when partition was mounted to test that lists are ok.
All other places should be ok, since simple list_add/list_del is inserted.

Sorry if I was unclear, I had in mind SMP performance testing of mount and unmount -heavy workloads, like uni setups with many automounted fs's, not stability testing per se.

-- wli

Kirill Korotaev

author

2003-10-13 12:01 UTC #8 lore raw

Quoted 9 lines

Quoted 8 lines
This patch can be tested quite easily. Main changes are in invalidate_list. This path can be triggered by umount/quota off. So I tested it the following way:
1. mounting/working/umounting partition (and turning quota on/off)
2. running memeater to call prune_icache when partition was mounted to test that lists are ok.
All other places should be ok, since simple list_add/list_del is inserted.

Quoted 3 lines

Sorry if I was unclear, I had in mind SMP performance testing of mount and unmount -heavy workloads, like uni setups with many automounted fs's, not stability testing per se.

Oh, sorry for misunderstanding.
In our internal testcase on 8-CPU 8Gb RAM machine with 4gb split kernel w/o this patch mount/umount test longs in many-many (>10) times longer. Moreover, during the test machine is very slow (due to lock_kernel) and typing simple commands takes up to 30 seconds or so.
I think such a long hangs are due to number of umounts executed subsequently. But ofcourse it's not numbers, just for you to know where the patch comes from :)

Kirill

William Lee Irwin III

2003-10-13 12:08 UTC #9 lore raw

Quoted 3 lines · At some point in the past, I wrote:

Quoted 3 lines
Sorry if I was unclear, I had in mind SMP performance testing of mount and unmount -heavy workloads, like uni setups with many automounted fs's, not stability testing per se.

Quoted 8 lines · Kirill Korotaev

Oh, sorry for misunderstanding.
In our internal testcase on 8-CPU 8Gb RAM machine with 4gb split kernel w/o this patch mount/umount test longs in many-many (>10) times longer. Moreover, during the test machine is very slow (due to lock_kernel) and typing simple commands takes up to 30 seconds or so.
I think such a long hangs are due to number of umounts executed subsequently. But ofcourse it's not numbers, just for you to know where the patch comes from :)

Is this testcase available and/or trivial? Actually, even if it's trivial it might just save us the pain of writing the scripts ourselves.

-- wli

Kirill Korotaev

author

2003-10-13 12:20 UTC #10 lore raw

Quoted 15 lines

Quoted 3 lines · At some point in the past, I wrote:
Quoted 3 lines
Sorry if I was unclear, I had in mind SMP performance testing of mount and unmount -heavy workloads, like uni setups with many automounted fs's, not stability testing per se.
Quoted 8 lines · Kirill Korotaev
Oh, sorry for misunderstanding.
In our internal testcase on 8-CPU 8Gb RAM machine with 4gb split kernel w/o this patch mount/umount test longs in many-many (>10) times longer. Moreover, during the test machine is very slow (due to lock_kernel) and typing simple commands takes up to 30 seconds or so.
I think such a long hangs are due to number of umounts executed subsequently. But ofcourse it's not numbers, just for you to know where the patch comes from :)

Quoted 2 lines

Is this testcase available and/or trivial? Actually, even if it's trivial it might just save us the pain of writing the scripts ourselves.

no, testcase is not available :( And it uses functionality not available in mainstream kernel. But the problem can be hit with very simple script instead:

1. mount N filesystems.
2. work on them, so that inode cache grows to its maximum possible size (it was 1,000,000 of inodes in our case).
3. umount these filesystems.

During operation #3 node is very slow and it is quite noticable on ssh console when typing commands.

Kirill

Nikita Danilov

2003-10-13 12:29 UTC #11 lore raw

Quoted 21 lines · Kirill Korotaev

Quoted 15 lines
Quoted 3 lines · At some point in the past, I wrote:
Quoted 3 lines
Sorry if I was unclear, I had in mind SMP performance testing of mount and unmount -heavy workloads, like uni setups with many automounted fs's, not stability testing per se.
Quoted 8 lines · Kirill Korotaev
Oh, sorry for misunderstanding.
In our internal testcase on 8-CPU 8Gb RAM machine with 4gb split kernel w/o this patch mount/umount test longs in many-many (>10) times longer. Moreover, during the test machine is very slow (due to lock_kernel) and typing simple commands takes up to 30 seconds or so.
I think such a long hangs are due to number of umounts executed subsequently. But ofcourse it's not numbers, just for you to know where the patch comes from :)
Quoted 2 lines
Is this testcase available and/or trivial? Actually, even if it's trivial it might just save us the pain of writing the scripts ourselves.
no, testcase is not available :( And it uses functionality not available in mainstream kernel. But the problem can be hit with very simple script instead:

Quoted 7 lines

1. mount N filesystems.
2. work on them, so that inode cache grows to its maximum possible size (it was 1,000,000 of inodes in our case).
3. umount these filesystems.
During operation #3 node is very slow and it is quite noticable on ssh console when typing commands.

This can be due to a number of reasons (worst case behavior of shrink_dcache_parent() for example). What /proc/profile shows?

Quoted 2 lines

Kirill

Nikita.

Quoted 1 line

Kirill Korotaev

author

2003-10-13 13:07 UTC #12 lore raw

Quoted 9 lines

Quoted 9 lines
no, testcase is not available :( And it uses functionality not available in mainstream kernel. But the problem can be hit with very simple script instead:
1. mount N filesystems.
2. work on them, so that inode cache grows to its maximum possible size (it was 1,000,000 of inodes in our case).
3. umount these filesystems.

Quoted 5 lines

Quoted 2 lines
During operation #3 node is very slow and it is quite noticable on ssh console when typing commands.
This can be due to a number of reasons (worst case behavior of shrink_dcache_parent() for example). What /proc/profile shows?

I used cycles measuring patch. It showed that > 50% CPU during the test was spent in invalidate_inodes().

Kirill