open-scale-2.6.2-A0

open-scale-2.6.2-A0

[Ingo Molnar]

[-- Attachment #1: Type: text/plain, Size: 176 bytes --]


i've attached an obvious scalability improvement for write()s. We in
essence used a system-global lock for every open(WRITE) - argh!

Compiles & boots fine on x86 SMP.

	Ingo

[-- Attachment #2: inode-scale-2.6.2-A0 --]
[-- Type: text/plain, Size: 1489 bytes --]

--- linux/fs/namei.c.orig2	
+++ linux/fs/namei.c	
@@ -238,30 +238,34 @@ int permission(struct inode * inode,int 
  * except for the cases where we don't hold i_writecount yet. Then we need to
  * use {get,deny}_write_access() - these functions check the sign and refuse
  * to do the change if sign is wrong. Exclusion between them is provided by
- * spinlock (arbitration_lock) and I'll rip the second arsehole to the first
- * who will try to move it in struct inode - just leave it here.
+ * the inode->i_lock spinlock.
  */
-static spinlock_t arbitration_lock = SPIN_LOCK_UNLOCKED;
+
 int get_write_access(struct inode * inode)
 {
-	spin_lock(&arbitration_lock);
+	spin_lock(&inode->i_lock);
 	if (atomic_read(&inode->i_writecount) < 0) {
-		spin_unlock(&arbitration_lock);
+		spin_unlock(&inode->i_lock);
 		return -ETXTBSY;
 	}
 	atomic_inc(&inode->i_writecount);
-	spin_unlock(&arbitration_lock);
+	spin_unlock(&inode->i_lock);
+
 	return 0;
 }
+
 int deny_write_access(struct file * file)
 {
-	spin_lock(&arbitration_lock);
-	if (atomic_read(&file->f_dentry->d_inode->i_writecount) > 0) {
-		spin_unlock(&arbitration_lock);
+	struct inode *inode = file->f_dentry->d_inode;
+
+	spin_lock(&inode->i_lock);
+	if (atomic_read(&inode->i_writecount) > 0) {
+		spin_unlock(&inode->i_lock);
 		return -ETXTBSY;
 	}
-	atomic_dec(&file->f_dentry->d_inode->i_writecount);
-	spin_unlock(&arbitration_lock);
+	atomic_dec(&inode->i_writecount);
+	spin_unlock(&inode->i_lock);
+
 	return 0;
 }
 

Re: open-scale-2.6.2-A0

[Jamie Lokier]

Ingo Molnar wrote:
> i've attached an obvious scalability improvement for write()s. We in
> essence used a system-global lock for every open(WRITE) - argh!

I wonder if the "rip the second arsehole" is there for a reason.

Does this scalability improvement make any measured difference in any
conceivable application, or is it just making struct inode larger?

-- Jamie

Re: open-scale-2.6.2-A0

[Ingo Molnar]

* Jamie Lokier <jamie@shareable.org> wrote:

> Ingo Molnar wrote:
> > i've attached an obvious scalability improvement for write()s. We in
> > essence used a system-global lock for every open(WRITE) - argh!
> 
> I wonder if the "rip the second arsehole" is there for a reason.

these days i dont think the comment is justified.

> Does this scalability improvement make any measured difference in any
> conceivable application, or is it just making struct inode larger?

i've not added any new lock, i'm merely reusing the existing ->i_lock. 
So there's no data or code bloat whatsoever.

one doesnt need any measurements to tell that a global cacheline
dirtying for every (most of the time completely unrelated) open(WRITE)
will hurt workloads that do that, especially on NUMA systems. I'd not be
surprised if it showed up in dbench/netbench.

	Ingo

Re: open-scale-2.6.2-A0

[Jamie Lokier]

Ingo Molnar wrote:
> > Does this scalability improvement make any measured difference in any
> > conceivable application, or is it just making struct inode larger?
> 
> i've not added any new lock, i'm merely reusing the existing ->i_lock. 
> So there's no data or code bloat whatsoever.

Oh of course (duh!).

-- Jamie

Re: open-scale-2.6.2-A0

[Andrew Morton]

Ingo Molnar <mingo@elte.hu> wrote:
>
> 
> * Jamie Lokier <jamie@shareable.org> wrote:
> 
> > Ingo Molnar wrote:
> > > i've attached an obvious scalability improvement for write()s. We in
> > > essence used a system-global lock for every open(WRITE) - argh!
> > 
> > I wonder if the "rip the second arsehole" is there for a reason.
> 
> these days i dont think the comment is justified.

It was kinda funny though.

> > Does this scalability improvement make any measured difference in any
> > conceivable application, or is it just making struct inode larger?
> 
> i've not added any new lock, i'm merely reusing the existing ->i_lock. 
> So there's no data or code bloat whatsoever.

yes, that's why I called it i_lock and not i_blocks_lock.  i_lock's mandate
is "an innermost lock for protecting stuff in the inode".  This is an
appropriate use of it.