lkml.org 
[lkml]   [2003]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
Subject2.4, 2.5: SMP race: __sync_single_inode vs. __mark_inode_dirty
Hi.

there's a SMP race condition between __sync_single_inode (or __sync_one on
2.4.20) and __mark_inode_dirty. __mark_inode_dirty doesn't take inode
spinlock. As we know -- unless you take a spinlock or use barrier,
processor can change order of instructions.

CPU 1

modify inode
(but modifications are in cpu-local
buffer and do not go to bus)

calls
__mark_inode_dirty
it sees I_DIRTY and exits immediatelly
CPU 2
takes spinlock
calls __sync_single_inode
inode->i_state &= ~I_DIRTY
writes the inode (but does not see
modifications by CPU 1 yet)

CPU 1 flushes its write buffer to the bus
inode is already written, clean, modifications
done by CPU1 are lost

The easiest fix would be to move the test inside spinlock in
__mark_inode_dirty; if you do not want to suffer from performance loss,
use the attached patches that use memory barriers to ensure ordering of
reads and writes.

Mikulas



--- linux/fs/inode.c_ Sat Aug 3 01:39:44 2002
+++ linux/fs/inode.c Sun Feb 2 23:21:40 2003
@@ -147,6 +147,10 @@
sb->s_op->dirty_inode(inode);
}

+ /* make sure that changes are seen by all cpus before we test i_state
+ -- mikulas */
+ smp_mb();
+
/* avoid the locking if we can */
if ((inode->i_state & flags) == flags)
return;
@@ -219,6 +223,11 @@
dirty = inode->i_state & I_DIRTY;
inode->i_state |= I_LOCK;
inode->i_state &= ~I_DIRTY;
+
+ smp_rmb(); /* mark_inode_dirty doesn't take spinlock, make sure
+ that inode is not read speculatively by this cpu
+ before &= ~I_DIRTY -- mikulas */
+
spin_unlock(&inode_lock);

filemap_fdatasync(inode->i_mapping);--- linux/fs/fs-writeback.c_ Fri Jan 17 03:23:00 2003
+++ linux/fs/fs-writeback.c Sun Feb 2 23:22:00 2003
@@ -61,6 +61,10 @@
sb->s_op->dirty_inode(inode);
}

+ /* make sure that changes are seen by all cpus before we test i_state
+ -- mikulas */
+ smp_mb();
+
/* avoid the locking if we can */
if ((inode->i_state & flags) == flags)
return;
@@ -135,6 +139,11 @@
dirty = inode->i_state & I_DIRTY;
inode->i_state |= I_LOCK;
inode->i_state &= ~I_DIRTY;
+
+ /* smp_rmb(); note: if you remove write_lock below, you must add this.
+ mark_inode_dirty doesn't take spinlock, make sure
+ that inode is not read speculatively by this cpu
+ before &= ~I_DIRTY -- mikulas */

write_lock(&mapping->page_lock);
if (wait || !wbc->for_kupdate || list_empty(&mapping->io_pages))
\
 
 \ /
  Last update: 2005-03-22 13:32    [W:0.037 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site