On Mon, 2009-09-14 at 22:24 +0530, Aneesh Kumar K.V wrote:
> 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
Hmm. Yeah, you're right. I was thinking that the inode could be
dirtied without calling do_update_inode() but that's apparently not the
case.
Another version of the patch will be forthcoming shortly.
--
Frank Mayhar <fmayhar@xxxxxxxxxx>
Google, Inc.
--
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
