===================================================================
if (ext4_journal_current_handle())
goto no_write;
if (PageChecked(page)) {
/*
* It's mmapped pagecache. Add buffers and journal it. There
* doesn't seem much point in redirtying the page here.
*/
ClearPageChecked(page);
return __ext4_journalled_writepage(page, wbc);
} else {
/*
* It may be a page full of checkpoint-mode buffers. We don't
* really know unless we go poke around in the buffer_heads.
* But block_write_full_page will do the right thing.
*/
return block_write_full_page(page,
ext4_normal_get_block_write,
wbc);
}
no_write:
===================================================================
3. Unfortunately, __ext4_journalled_writepage() in the case of no journal,
will just
- call block_prepare_write()
- calls write_end_fn() on all buffers, which just marks them dirty,
doesn't actually write them out.
And it doesn't seem to me that __ext4_journalled_writepage() will ever be
called if there IS a journal.
Am I missing something here?
4. If PageChecked bit isn't set, it calls block_write_full_page() and works
fine.
Below is a patch that "fixes" ext4_set_aops(): that is, in the case of
delayed allocation but no journal, we'll use ext4_da_aops. It does NOT fix
the problem of nodelalloc and either
- no journal
- data=journal
I'm holding off on fixing this because I'm not sure of the right place for
it. I think it should be one of:
a. Adding an address space ops structure just for nodelalloc/nojournal
b. Getting rid of __ext4_journalled_writepage() as well as
ext4_journalled_set_page_dirty(), since I'm not really sure they do
anything.
Comments please?
Signed-off-by: Curt Wohlgemuth <curtw@google.com>
---
@@ -3442,14 +3442,10 @@ static const struct address_space_operat
void ext4_set_aops(struct inode *inode)
{
- if (ext4_should_order_data(inode) &&
- test_opt(inode->i_sb, DELALLOC))
+ if (test_opt(inode->i_sb, DELALLOC))
inode->i_mapping->a_ops = &ext4_da_aops;
else if (ext4_should_order_data(inode))
inode->i_mapping->a_ops = &ext4_ordered_aops;
- else if (ext4_should_writeback_data(inode) &&
- test_opt(inode->i_sb, DELALLOC))
- inode->i_mapping->a_ops = &ext4_da_aops;
else if (ext4_should_writeback_data(inode))
inode->i_mapping->a_ops = &ext4_writeback_aops;
else
So I finally was able to figure out the data corruption problem with locale-gen on ext4 without a journal. Basically, using mmap(MAP_SHARED, PROT_WRITE) to write to a file through an mmap'ed pointer is broken on ext4 when there is no journal. It seems to be a combination of several problems: a. The choice of what address space ops to use in ext4_set_aops() just seems wrong to me. b. The use of ext4_journalled_writepage() if there is no journal being used is broken if the page was marked dirty from ext4_journalled_set_page_dirty(). I don't understand how __ext4_journalled_writepage() would ever work. ext4_set_aops() chooses among 4 different structures: ext4_da_aops ext4_ordered_aops ext4_writeback_aops ext4_journalled_aops It seems to me that ext4_da_aops should be used whenever delayed allocation is used, and the rest otherwise. But this leaves open the question of what to use when nodelalloc is used, AND there's no journal. Today, this uses ext4_journalled_aops, but this seems odd on its face. Yes, I know that all the routines there are supposed to handle no journal, but it's nevertheless odd. The problem with ext4_journalled_writepage() is this: 1. ext4_journalled_set_page_dirty() sets the PageChecked bit, then dirties the page. 2. ext4_journalled_writepage() will do the following; note the goto if there IS a journal: -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html