Hi Ted: On Mon, Sep 7, 2009 at 10:06 PM, Theodore Tso<tytso@xxxxxxx> wrote: > On Fri, Sep 04, 2009 at 07:55:00PM -0700, Frank Mayhar wrote: >> Teach ext4_write_inode() and ext4_do_update_inode() about non-journal >> mode: If we're not using a journal, ext4_write_inode() now calls >> ext4_do_update_inode() (after getting the iloc via ext4_get_inode_loc()) >> with a new "do_sync" parameter. If that parameter is nonzero >> ext4_do_update_inode() calls sync_dirty_buffer() instead of >> ext4_handle_dirty_metadata(). > > Hi Frank, > > The problem with this patch is that it's only safe to call > sync_dirty_buffer() if we are not journalling. If we are using the > journal, we must *not* call sync_dirty_buffer(), but instead must use > jbd2_journal_dirty_metadata(). > > The problem is that there are paths where ext4_do_update_inode() can > get called with do_sync==1, even when journalling is enabled. > Specifically, if ext4_write_inode() is called with wait==1, wait is > passed to ext4_do_update_inode() as do_sync, and then when a journal > is present, we will end up calling sync_dirty_buffer(), which means we > will be writing out the modified metadata *before* the transaction has > committed. > > If you try using your patch with journalling enabled, and you try > doing some power fail testing, my code inspection leads me to believe > with 99% certainty that the filesystem will be corrupted as a result. > > I think what you need to do instead is to add an extra parameter > do_sync to ext4_handle_dirty_metadata(), and continue to call > ext4_handle_dirty_metadata. However in code paths where we will later > force a commit to guarantee that the metadata has been written out > (i.e., in the fsync() code path), ext4_handle_dirty_metadata() should > be called with the new do_sync parameter set to 1. > > Does that make sense? I think we can take a look at this, but there are a lot of calls to ext4_handle_dirty_metadata(), and it's not clear on a quick inspection that we'd be able to determine which would need to be called with do_sync = 1... On the other hand, this would take care of a similar problem that I was going to be sending a patch for this week: where removing an extent block without a journal requires a sync_dirty_buffer() in order to avoid writeback of the extent header in the block, *after* the block is marked free in the bitmap. There are probably other cases where, without a journal, an explicit sync_dirty_buffer() is needed for metadata. Handling this in ext4_handle_dirty_metadata() may be the best way to solve this. Thanks, Curt > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
