Re: ext3 0.9.12 for 2.4.10-ac11

[Andrew Morton <akpm zip com au>]

"Stephen C. Tweedie" wrote:
> 
> Hi,
> 
> On Fri, Oct 12, 2001 at 07:15:54PM -0700, Andrew Morton wrote:
> 
> > It seems to work OK.  I can oops it on SMP with one particular
> > workload, but I doubt if anyone else will see that.
> 
> Do you have an oops trace there?
> 

My trick in ext3_writepage() of elevating the buffer counts while
we're using the page works OK for try_to_free_buffers(), but it
doesn't protect the page from truncate.  Truncate will happily
throw the page away while it has attached buffers - it becomes
an anon buffercache page.  But ext3_truncate continues to use
page->mapping, which has become NULL.

So ext3_writepage() needs to be taught to not touch page->mapping()
after the page has become unlocked - ie: after the block_write_full_page()
call.  The bug appears to be present in -ac as well.  We just never hit
it.

So I guess we attach the writepage buffers to the transaction prior
to calling block_write_full_page(), rather than after.  I'll test
the below patch tomorrow.

The locking's a bit dodgy - it'd be better to lock the journal around
the journal_dirty_async_data and block_write_full_page() calls to
bind them together.   I guess it's time for a lock_journal() in
ext3_writepage() - what do you think?


Index: fs/ext3/inode.c
===================================================================
RCS file: /cvsroot/gkernel/ext3/fs/ext3/inode.c,v
retrieving revision 1.64.2.8
diff -u -r1.64.2.8 inode.c
--- fs/ext3/inode.c	2001/10/11 07:36:09	1.64.2.8
+++ fs/ext3/inode.c	2001/10/18 07:36:55
@@ -1053,7 +1053,6 @@
 static int journal_dirty_async_data(handle_t *handle, struct buffer_head *bh)
 {
 	int ret = ext3_journal_dirty_data(handle, bh, 1);
-	__brelse(bh);
 	return ret;
 }
 
@@ -1263,37 +1262,28 @@
 	order_data = ext3_should_order_data(inode) ||
 			ext3_should_journal_data(inode);
 
-	unlock_kernel();
-
 	/* bget() all the buffers */
 	if (order_data) {
 		if (!page->buffers)
 			create_empty_buffers(page,
 				inode->i_dev, inode->i_sb->s_blocksize);
-		walk_page_buffers(handle, page, 0,
-				PAGE_CACHE_SIZE, NULL, bget_one);
+		ret = walk_page_buffers(handle, page,
+			0, PAGE_CACHE_SIZE, NULL, journal_dirty_async_data);
 	}
 
-	ret = block_write_full_page(page, ext3_get_block);
+	unlock_kernel();
+	err = block_write_full_page(page, ext3_get_block);
+	if (ret == 0)
+		ret = err;
 	handle = ext3_journal_current_handle();
 	lock_kernel();
-
-	/* And attach them to the current transaction */
-	if (order_data) {
-		err = walk_page_buffers(handle, page,
-			0, PAGE_CACHE_SIZE, NULL, journal_dirty_async_data);
-		if (!ret)
-			ret = err;
-	}
-
 	err = ext3_journal_stop(handle, inode);
+	unlock_kernel();
 	if (!ret)
 		ret = err;
-	unlock_kernel();
 	return ret;
 
 out_fail:
-	
 	unlock_kernel();
 	SetPageDirty(page);
 	UnlockPage(page);