Re: ext4: Can we talk about bforget() and metadata blocks

[Theodore Tso <tytso@mit.edu>]

On Thu, Sep 10, 2009 at 09:54:35PM +0530, Aneesh Kumar K.V wrote:
> 
> But how would it work for fsync ? I mean 
> 
> I would expect for no journal mode ext4_sync_file  should be doing
> simple_fsync(). That should be forcing the metadata buffer_heads
> via sync_mapping_buffers. And if we reuse these meta buffers we
> drop them the inode->mapping->private_list using bforget.
> 
> But I don't see any of the above in code

Aneesh, you're addressing a different problem than the one that Curt
were trying to deal with this patch.  The problem we are worry about
is one where an inode's extent tree or indirect blocks are modified
right before the inode is deleted, and then one or more of those
metadata blocks get reallocated and written right away (most likely
this will happen via an O_DIRECT write), and then, because we didn't
use bforget(), the dirty metadata block in the buffer cache would get
written out, overwriting the O_DIRECT block.

What you're worrying about, is a different issue.  You're concerned
about the fact that since we are not associating an inode's extent
tree or indirect blocks with the inode, those blocks won't get forced
out to disk on an fsync() in ext4 no-journal mode.  This may not be a
big deal for applications which expect to recover from an unclean
using mke2fs (and thus probably don't use fsync in any case), but
here's a patch to deal with the problem you've raised.

       	       	       	    		       - Ted

commit 417cf58253fbf3e36df7b3aca11c120e8367f5e6
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Sep 10 14:58:02 2009 -0400

    ext4: Assure that metadata blocks are written during fsync in no journal mode
    
    When there is no journal present, we must attach buffer heads
    associated with extent tree and indirect blocks to the inode's
    mapping->private_list so that fsync() will write out the inode's
    metadata blocks.  This is done via mark_buffer_dirty_inode().
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index ecb9ca4..6a94099 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -89,7 +89,10 @@ int __ext4_handle_dirty_metadata(const char *where, handle_t *handle,
 			ext4_journal_abort_handle(where, __func__, bh,
 						  handle, err);
 	} else {
-		mark_buffer_dirty(bh);
+		if (inode && bh)
+			mark_buffer_dirty_inode(bh, inode);
+		else
+			mark_buffer_dirty(bh);
 		if (inode && inode_needs_sync(inode)) {
 			sync_dirty_buffer(bh);
 			if (buffer_req(bh) && !buffer_uptodate(bh)) {