decrement of inodes_stat.nr_inodes in inode.c not SMP safe?

decrement of inodes_stat.nr_inodes in inode.c not SMP safe?

[Rebecca.Callan]

The value for nr_inodes in /proc/sys/fs/inode-state appears to be wrong.

I think this is probably a bug in all 2.4 smp kernels. I've seen it in
2.4.8-26mdksmp and 2.4.18-3smp.

I first noticed the bug when sar -v reported inode-sz to be 4294966776. 
I check the sar source code and the documentation and found the value 
of nr_free_inodes (second value in the /proc/sys/fs/inode-state file) is
larger than nr_inodes (the number of allocated inodes - first value in
file). 

I think I have tracked the bug down to an unsafe decrement of 
inodes_stat.nr_inodes in fs/inode.c. This variable is changed in a 
number of places in inode.c and it is locked everywhere except in 
dispose_list(). The comment above dispose_list says:
	 
 * Dispose-list gets a local list with local inodes in it, so it doesn't
 * need to worry about list corruption and SMP locks. 

But inodes_stat.nr_inodes is not local and I think it requires locking.

I am not a kernel developer and don't know exactly how to fix this problem.
I suppose there is a reason why the dispose_list function was designed to 
not use locking so I guess it's not a good idea to put a lock in there. The
only other option I can think of is to use atomic decrement, but I don't
know whether it is safe to atomicaly decrement something that is decremented

elsewhere using locking not an atomic decrement. Is it a good idea to change

all accesses  of this variable to atomic? Would this add unnecessary
overhead? 

Thanks,
Rebecca

Re: decrement of inodes_stat.nr_inodes in inode.c not SMP safe?

[Andrew Morton]

Rebecca.Callan@ir.com wrote:
> 
> The value for nr_inodes in /proc/sys/fs/inode-state appears to be wrong.
> 
> I think this is probably a bug in all 2.4 smp kernels. I've seen it in
> 2.4.8-26mdksmp and 2.4.18-3smp.
> 

Is true.  The 2.5 change needs to be backported.


--- linux-akpm/fs/inode.c~inodes_stat-race	Tue Nov 19 21:42:08 2002
+++ linux-akpm-akpm/fs/inode.c	Tue Nov 19 21:42:23 2002
@@ -532,22 +532,25 @@ void clear_inode(struct inode *inode)
  * Dispose-list gets a local list with local inodes in it, so it doesn't
  * need to worry about list corruption and SMP locks.
  */
-static void dispose_list(struct list_head * head)
+static void dispose_list(struct list_head *head)
 {
-	struct list_head * inode_entry;
-	struct inode * inode;
+	int nr_disposed = 0;
 
-	while ((inode_entry = head->next) != head)
-	{
-		list_del(inode_entry);
+	while (!list_empty(head)) {
+		struct inode *inode;
+
+		inode = list_entry(head->next, struct inode, i_list);
+		list_del(&inode->i_list);
 
-		inode = list_entry(inode_entry, struct inode, i_list);
 		if (inode->i_data.nrpages)
 			truncate_inode_pages(&inode->i_data, 0);
 		clear_inode(inode);
 		destroy_inode(inode);
-		inodes_stat.nr_inodes--;
+		nr_disposed++;
 	}
+	spin_lock(&inode_lock);
+	inodes_stat.nr_inodes -= nr_disposed;
+	spin_unlock(&inode_lock);
 }
 
 /*

_