On Wed, Sep 09, 2009 at 10:34:24AM -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 _and_ we're
> not using a journal, ext4_do_update_inode() calls sync_dirty_buffer()
> instead of ext4_handle_dirty_metadata().
>
> This problem was found in power-fail testing, checking the amount of
> loss of files and blocks after a power failure when using fsync() and
> when not using fsync(). It turned out that using fsync() was actually
> worse than not doing so, possibly because it increased the likelihood
> that the inodes would remain unflushed and would therefore be lost at
> the power failure.
>
> Signed-off-by: Frank Mayhar <fmayhar@xxxxxxxxxx>
>
> fs/ext4/inode.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d87f6a0..ef2e780 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4741,7 +4741,8 @@ static int ext4_inode_blocks_set(handle_t *handle,
> */
> static int ext4_do_update_inode(handle_t *handle,
> struct inode *inode,
> - struct ext4_iloc *iloc)
> + struct ext4_iloc *iloc,
> + int do_sync)
> {
> struct ext4_inode *raw_inode = ext4_raw_inode(iloc);
> struct ext4_inode_info *ei = EXT4_I(inode);
> @@ -4843,10 +4844,22 @@ static int ext4_do_update_inode(handle_t *handle,
> raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> }
>
> - BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
> - rc = ext4_handle_dirty_metadata(handle, inode, bh);
> - if (!err)
> - err = rc;
> + /*
> + * If we're not using a journal and we were called from
> + * ext4_write_inode() to sync the inode (making do_sync true),
> + * we can just use sync_dirty_buffer() directly to do our dirty
> + * work. Testing s_journal here is a bit redundant but it's
> + * worth it to avoid potential future trouble.
> + */
> + if (EXT4_SB(inode->i_sb)->s_journal == NULL && do_sync) {
> + BUFFER_TRACE(bh, "call sync_dirty_buffer");
> + sync_dirty_buffer(bh);
> + } else {
> + BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
> + rc = ext4_handle_dirty_metadata(handle, inode, bh);
> + if (!err)
> + err = rc;
> + }
> ei->i_state &= ~EXT4_STATE_NEW;
>
> out_brelse:
> @@ -4892,19 +4905,32 @@ out_brelse:
> */
> int ext4_write_inode(struct inode *inode, int wait)
> {
> + int err;
> +
> if (current->flags & PF_MEMALLOC)
> return 0;
>
> - if (ext4_journal_current_handle()) {
> - jbd_debug(1, "called recursively, non-PF_MEMALLOC!\n");
> - dump_stack();
> - return -EIO;
> - }
> + if (EXT4_SB(inode->i_sb)->s_journal) {
> + if (ext4_journal_current_handle()) {
> + jbd_debug(1, "called recursively, non-PF_MEMALLOC!\n");
> + dump_stack();
> + return -EIO;
> + }
>
> - if (!wait)
> - return 0;
> + if (!wait)
> + return 0;
> +
> + err = ext4_force_commit(inode->i_sb);
> + } else {
> + struct ext4_iloc iloc;
>
> - return ext4_force_commit(inode->i_sb);
> + err = ext4_get_inode_loc(inode, &iloc);
> + if (err)
> + return err;
> + err = ext4_do_update_inode(EXT4_NOJOURNAL_HANDLE,
> + inode, &iloc, wait);
> + }
> + return err;
> }
Why not just do
err = ext4_get_inode_loc(inode, &iloc);
if (err)
return err;
if (wait)
sync_dirty_buffer(iloc.bh);
because when we updated inode we would have called ext4_mark_inode_dirty which
internally does ext4_mark_iloc_dirty -> ext4_do_update_inode
May be we also want to check the error from sync_dirty_buffer
sync_dirty_buffer(bh);
if (buffer_req(bh) && !buffer_uptodate(bh)) {
ext4_error(inode->i_sb, __func__,
"IO error syncing inode, "
"inode=%lu, block=%llu",
inode->i_ino,
(unsigned long long) bh->b_blocknr);
err = -EIO;
}
>
> /*
> @@ -5198,7 +5224,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
> get_bh(iloc->bh);
>
> /* ext4_do_update_inode() does jbd2_journal_dirty_metadata */
> - err = ext4_do_update_inode(handle, inode, iloc);
> + err = ext4_do_update_inode(handle, inode, iloc, 0);
> put_bh(iloc->bh);
> return err;
> }
>
-aneesh
--
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
