Date:	Tue, 8 Sep 2009 21:00:50 -0700
From:	Curt Wohlgemuth <curtw@...gle.com>
To:	Theodore Tso <tytso@....edu>
Cc:	Valerie Aurora <vaurora@...hat.com>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: Odd "leak" of extent info into data blocks?

Hi Ted:

On Tue, Sep 8, 2009 at 4:36 PM, Theodore Tso<tytso@....edu> wrote:
> On Tue, Sep 08, 2009 at 02:18:35PM -0700, Curt Wohlgemuth wrote:
>>
>> All bforget() does is clear the buffer's dirty bit.  Meanwhile, the
>> page is still marked dirty, and can be in the middle of writeback;
>> it's true that __block_write_full_page() will check the dirty bit for
>> each buffer in the page, but there doesn't seem to be any
>> synchronization to ensure that the write won't take place at some
>> point in time after bforget() is called.  Which means it can be called
>> after the bitmap is changed.
>
> Let me make sure I got this right.  The problem that you're worried
> about is a block that had previously contained an extent tree node for
> an inode that gets deleted, and then that blocks gets reallocated for
> use as a data block.

Correct.

> In ext3 and ext4, metadata blocks (such as
> extent tree blocks), aren't stored in the page cache.

Hmm.  You're saying that in the absence of a journal, all metadata
writes go direct to disk?  Where should I look for this in the code?

Looking at ext4_ext_new_meta_block() and code that uses it, I don't
see anything that prevents the use of the page cache.  And if this
were the case, wouldn't the call to mark_buffer_dirty() in
__ext4_handle_dirty_metadata() (when there's no journal) do nothing?

I also put in code in submit_bio() to scan all pages for the extent
header pattern that I was seeing ("leaking" into the data pages).
When I saw it, the stack trace was always from pdflush() (from
wb_kupdate()).  I.e., these are from the page cache.

> So I'm not sure why you're worried about the page being marked dirty.
> What's the scenario you are concerned about?

If you're right that metadata writes are not through the page cache,
then there is no scenario I'm worried about :-) .

The problem is that I've seen this in real life.  And the patch below
seems to fix it.  (Unfortunately, I haven't been able to recreate this
in a simple example, after several days work.  I've only seen this in
a *very* small number of cases on heavily loaded machines.)

> If it's the case where a data block for a deleted inode getting
> rewritten after the inode is deleted, when the inode is deleted,
> truncate_inode_apges() end up dropping the pages from the page cache
> *before* the block allocation bitmap is dropped.

It's quite possible that there's an interaction with older code that
we have in our 2.6.26-based kernels -- our ext4/jbd2 code is pretty
up-to-date, but the rest of the code base is not.  But really, in my
case -- and you'll have to trust me -- I've seen this pattern:

1. file A (~8MB) is written out and closed, with a final mod time of 12:08 p.m.
2. My submit_bio() scan sees the "bad extent header" written out to
physical block B at 12:15 p.m.
3. Looking at file A later, its logical block 2048 corresponds to
physical block B -- and contains the "bad extent header" pattern.

truncate_inode_pages() only deals with data blocks, right?  So it
should have no effect on metadata...

>> This is why I opted to wait for the buffer to be written out before
>> continuing on to ext4_free_blocks().
>
> Just to be clear, which buffer are you talking about here?

The leaf extent blocks buffer_head.  Here's the patch, as applied to a
2.6.30.3 version of extents.c:

diff -Naur orig/fs/ext4/extents.c new/fs/ext4/extents.c
--- orig/fs/ext4/extents.c	2009-09-08 20:28:46.000000000 -0700
+++ new/fs/ext4/extents.c	2009-09-08 20:31:42.000000000 -0700
@@ -1958,6 +1958,15 @@
 		return err;
 	ext_debug("index is empty, remove it, free block %llu\n", leaf);
 	bh = sb_find_get_block(inode->i_sb, leaf);
+
+	/*
+	 * If we don't have a journal, then we've dirtied the BH for the leaf
+	 * block, but we're freeing the block now.  We need to wait here for
+	 * the page to be written out before we proceed.
+	 */
+	if (!ext4_handle_valid(handle) && bh)
+		sync_dirty_buffer(bh);
+
 	ext4_forget(handle, 1, inode, bh, leaf);
 	ext4_free_blocks(handle, inode, leaf, 1, 1);
 	return err;


Thanks,
Curt
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists