On Wed 08-05-13 15:37:34, Zheng Liu wrote:
> On Mon, Apr 08, 2013 at 11:32:30PM +0200, Jan Kara wrote:
> > Just use the generic function instead of duplicating it. We only need
> > to reshuffle the read-only check a bit (which is there to prevent
> > writing to a filesystem which has been remounted read-only after error
> > I assume).
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
>
> A minor nit below. Otherwise the patch looks good to me.
> Reviewed-by: Zheng Liu <wenqing.lz@xxxxxxxxxx>
>
> > ---
> > fs/ext4/fsync.c | 45 ++++++++++-----------------------------------
> > 1 files changed, 10 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> > index e02ba1b..1c08780 100644
> > --- a/fs/ext4/fsync.c
> > +++ b/fs/ext4/fsync.c
> > @@ -73,32 +73,6 @@ static int ext4_sync_parent(struct inode *inode)
> > return ret;
> > }
> >
> > -/**
> > - * __sync_file - generic_file_fsync without the locking and filemap_write
> > - * @inode: inode to sync
> > - * @datasync: only sync essential metadata if true
> > - *
> > - * This is just generic_file_fsync without the locking. This is needed for
> > - * nojournal mode to make sure this inodes data/metadata makes it to disk
> > - * properly. The i_mutex should be held already.
> > - */
> > -static int __sync_inode(struct inode *inode, int datasync)
> > -{
> > - int err;
> > - int ret;
> > -
> > - ret = sync_mapping_buffers(inode->i_mapping);
> > - if (!(inode->i_state & I_DIRTY))
> > - return ret;
> > - if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
> > - return ret;
> > -
> > - err = sync_inode_metadata(inode, 1);
> > - if (ret == 0)
> > - ret = err;
> > - return ret;
> > -}
> > -
> > /*
> > * akpm: A new design for ext4_sync_file().
> > *
> > @@ -116,7 +90,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > struct inode *inode = file->f_mapping->host;
> > struct ext4_inode_info *ei = EXT4_I(inode);
> > journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
> > - int ret, err;
> > + int ret = 0, err;
> > tid_t commit_tid;
> > bool needs_barrier = false;
> >
> > @@ -124,21 +98,21 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> >
> > trace_ext4_sync_file_enter(file, datasync);
> >
> > - ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > - if (ret)
> > - return ret;
> > - mutex_lock(&inode->i_mutex);
> > -
> > if (inode->i_sb->s_flags & MS_RDONLY)
> > - goto out;
> > + goto out_trace;
> >
> > if (!journal) {
> > - ret = __sync_inode(inode, datasync);
> > + ret = generic_file_fsync(file, start, end, datasync);
> > if (!ret && !hlist_empty(&inode->i_dentry))
> > ret = ext4_sync_parent(inode);
> > goto out;
> ^^^^
> goto out_trace;
> Otherwise we will unlock i_mutex that we haven't take it.
Ha, good spotting! The following patch will fix this but nevertheless
it's good to fix the breakage here. Thanks for review.
Honza
> > }
> >
> > + ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > + if (ret)
> > + return ret;
> > + mutex_lock(&inode->i_mutex);
> > +
> > /*
> > * data=writeback,ordered:
> > * The caller's filemap_fdatawrite()/wait will sync the data.
> > @@ -169,8 +143,9 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > if (!ret)
> > ret = err;
> > }
> > - out:
> > +out:
> > mutex_unlock(&inode->i_mutex);
> > +out_trace:
> > trace_ext4_sync_file_exit(inode, ret);
> > return ret;
> > }
> > --
> > 1.7.1
> >
> > --
> > 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
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
