[Cluster-devel] [PATCH] GFS2: dirty inode correctly in gfs2_write_end

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Tue, 2013-09-03 at 16:59 -0500, Benjamin Marzinski wrote:
> GFS2 was only setting I_DIRTY_DATASYNC on files that it wrote to, when
> it actually increased the file size.  If gfs2_fsync was called without
> I_DIRTY_DATASYNC set, it didn't flush the incore data to the log before
> returning, so any metadata or journaled data changes were not getting
> fsynced. This meant that writes to the middle of files were not always
> getting fsynced properly.
> 
> This patch makes gfs2 set I_DIRTY_DATASYNC whenever metadata has been
> updated during a write. It also make gfs2_sync flush the incore log
> if I_DIRTY_PAGES is set, and the file is using data journalling. This
> will make sure that all incore logged data gets written to disk before
> returning from a fsync.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  fs/gfs2/aops.c |    9 +++++++--
>  fs/gfs2/file.c |    4 +++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> Index: gfs2-3.0-nmw-130830/fs/gfs2/aops.c
> ===================================================================
> --- gfs2-3.0-nmw-130830.orig/fs/gfs2/aops.c
> +++ gfs2-3.0-nmw-130830/fs/gfs2/aops.c
> @@ -815,6 +815,8 @@ static int gfs2_write_end(struct file *f
>  	unsigned int from = pos & (PAGE_CACHE_SIZE - 1);
>  	unsigned int to = from + len;
>  	int ret;
> +	struct gfs2_trans *tr = current->journal_info;
> +	BUG_ON(!tr);
>  
>  	BUG_ON(gfs2_glock_is_locked_by_me(ip->i_gl) == NULL);
>  
> @@ -825,8 +827,6 @@ static int gfs2_write_end(struct file *f
>  		goto failed;
>  	}
>  
> -	gfs2_trans_add_meta(ip->i_gl, dibh);
> -
>  	if (gfs2_is_stuffed(ip))
>  		return gfs2_stuffed_write_end(inode, dibh, pos, len, copied, page);
>  
> @@ -834,6 +834,11 @@ static int gfs2_write_end(struct file *f
>  		gfs2_page_add_databufs(ip, page, from, to);
>  
>  	ret = generic_write_end(file, mapping, pos, len, copied, page, fsdata);
> +	if (tr->tr_num_buf_new)
> +		__mark_inode_dirty(inode, I_DIRTY_DATASYNC);
> +	else
> +		gfs2_trans_add_meta(ip->i_gl, dibh);
> +
I'm not sure I follow this bit... generic_write_end() will call
mark_inode_dirty() if the file size has changed. So that case should be
covered. That leaves only timestamps which I think should be
I_DIRTY_SYNC since we don't need those for fdatasync. But why is there a
conditional here on tr_num_buf_new ? Does the "else" actually occur in
practice?

Otherwise I think the patch looks good,

Steve.

>  
>  	if (inode == sdp->sd_rindex) {
>  		adjust_fs_space(inode);
> Index: gfs2-3.0-nmw-130830/fs/gfs2/file.c
> ===================================================================
> --- gfs2-3.0-nmw-130830.orig/fs/gfs2/file.c
> +++ gfs2-3.0-nmw-130830/fs/gfs2/file.c
> @@ -650,7 +650,7 @@ static int gfs2_fsync(struct file *file,
>  {
>  	struct address_space *mapping = file->f_mapping;
>  	struct inode *inode = mapping->host;
> -	int sync_state = inode->i_state & (I_DIRTY_SYNC|I_DIRTY_DATASYNC);
> +	int sync_state = inode->i_state & I_DIRTY;
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	int ret = 0, ret1 = 0;
>  
> @@ -660,6 +660,8 @@ static int gfs2_fsync(struct file *file,
>  			return ret1;
>  	}
>  
> +	if (!gfs2_is_jdata(ip))
> +		sync_state &= ~I_DIRTY_PAGES;
>  	if (datasync)
>  		sync_state &= ~I_DIRTY_SYNC;
>  
>