[patch 5/15] reiserfs locking fix

[patch 5/15] reiserfs locking fix

[Andrew Morton]

reiserfs is using b_inode_buffers and fsync_buffers_list() for
attaching dependent buffers to its journal.  For writeout prior to
commit.

This worked OK when a global lock was used everywhere, but the locking
is currently incorrect - try_to_free_buffers() is taking a different
lock when detaching buffers from their "foreign" inode.  So list_head
corruption could occur on SMP.

The patch implements a reiserfs_releasepage() which holds the
journal-wide buffer lock while it runs try_to_free_buffers(), so all
those list_heads are protected.  The lock is held across the
try_to_free_buffers() call as well, so nobody will attach one of this
page's buffers to a list while try_to_free_buffers() is running.



=====================================

--- 2.5.16/fs/reiserfs/inode.c~reiserfs_releasepage	Sun May 19 11:49:47 2002
+++ 2.5.16-akpm/fs/reiserfs/inode.c	Sun May 19 11:49:47 2002
@@ -2096,9 +2096,47 @@ static int reiserfs_commit_write(struct 
     return ret ;
 }
 
+/*
+ * Returns 1 if the page's buffers were dropped.  The page is locked.
+ *
+ * Takes j_dirty_buffers_lock to protect the b_assoc_buffers list_heads
+ * in the buffers at page_buffers(page).
+ *
+ * FIXME: Chris says the buffer list is not used with `mount -o notail',
+ * so in that case the fs can avoid the extra locking.  Create a second
+ * address_space_operations with a NULL ->releasepage and install that
+ * into new address_spaces.
+ */
+static int reiserfs_releasepage(struct page *page, int unused_gfp_flags)
+{
+    struct inode *inode = page->mapping->host ;
+    struct reiserfs_journal *j = SB_JOURNAL(inode->i_sb) ;
+    struct buffer_head *head ;
+    struct buffer_head *bh ;
+    int ret = 1 ;
+
+    spin_lock(&j->j_dirty_buffers_lock) ;
+    head = page_buffers(page) ;
+    bh = head ;
+    do {
+	if (!buffer_dirty(bh) && !buffer_locked(bh)) {
+		list_del_init(&bh->b_assoc_buffers) ;
+	} else {
+		ret = 0 ;
+		break ;
+	}
+	bh = bh->b_this_page ;
+    } while (bh != head) ;
+    if (ret)
+	ret = try_to_free_buffers(page) ;
+    spin_unlock(&j->j_dirty_buffers_lock) ;
+    return ret ;
+}
+
 struct address_space_operations reiserfs_address_space_operations = {
     writepage: reiserfs_writepage,
     readpage: reiserfs_readpage, 
+    releasepage: reiserfs_releasepage,
     sync_page: block_sync_page,
     prepare_write: reiserfs_prepare_write,
     commit_write: reiserfs_commit_write,
--- 2.5.16/fs/reiserfs/file.c~reiserfs_releasepage	Sun May 19 11:49:47 2002
+++ 2.5.16-akpm/fs/reiserfs/file.c	Sun May 19 11:49:47 2002
@@ -72,6 +72,12 @@ static void reiserfs_vfs_truncate_file(s
 }
 
 /* Sync a reiserfs file. */
+
+/*
+ * FIXME: sync_mapping_buffers() never has anything to sync.  Can
+ * be removed...
+ */
+
 static int reiserfs_sync_file(
 			      struct file   * p_s_filp,
 			      struct dentry * p_s_dentry,


-

Re: [reiserfs-dev] [patch 5/15] reiserfs locking fix

[Chris Mason]

On Sun, 2002-05-19 at 15:39, Andrew Morton wrote:
> 
> 
> reiserfs is using b_inode_buffers and fsync_buffers_list() for
> attaching dependent buffers to its journal.  For writeout prior to
> commit.
> 
> This worked OK when a global lock was used everywhere, but the locking
> is currently incorrect - try_to_free_buffers() is taking a different
> lock when detaching buffers from their "foreign" inode.  So list_head
> corruption could occur on SMP.

Thanks Andrew, this is working for me here.

-chris