[Cluster-devel] [PATCH v2] GFS: write vfs inode attributes to disk

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

Looks good to me. Thanks for fixing this,

Steve.

On 30/05/14 06:19, Benjamin Marzinski wrote:
> GFS wasn't ever updating mtime during mmaps to a file.  This patch makes
> gfs_write_inode write the vfs inode attributes out to disk, so that they
> get updated when the mmap data is written back to disk. It also makes
> sure that the updated vfs timestamps don't get overwritten by older gfs
> ones before they can be written out.
>
> This patch is similar to my earlier version, but includes more checks in
> gfs_write_inode to make sure that it can start a transaction, and changes
> to inode_attr_in to keep updated vfs timestamps from getting overwritten.
> These changes were made to resolve issues that caused bz #1066181 to fail
> QA. I have verified that it is still possible to manually reset the inode
> mtime to an earlier time using the touch command.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>   gfs-kernel/src/gfs/inode.c     | 15 +++++++++------
>   gfs-kernel/src/gfs/ops_super.c | 43 ++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/gfs-kernel/src/gfs/inode.c b/gfs-kernel/src/gfs/inode.c
> index 454366a..52b0819 100644
> --- a/gfs-kernel/src/gfs/inode.c
> +++ b/gfs-kernel/src/gfs/inode.c
> @@ -46,7 +46,7 @@
>    */
>   
>   static void
> -inode_attr_in(struct gfs_inode *ip, struct inode *inode)
> +inode_attr_in(struct gfs_inode *ip, struct inode *inode, int force)
>   {
>   	unsigned int mode;
>   
> @@ -93,9 +93,12 @@ inode_attr_in(struct gfs_inode *ip, struct inode *inode)
>   	inode->i_uid = ip->i_di.di_uid;
>   	inode->i_gid = ip->i_di.di_gid;
>   	i_size_write(inode, ip->i_di.di_size);
> -	inode->i_atime.tv_sec = ip->i_di.di_atime;
> -	inode->i_mtime.tv_sec = ip->i_di.di_mtime;
> -	inode->i_ctime.tv_sec = ip->i_di.di_ctime;
> +	if (force || ip->i_di.di_atime > inode->i_atime.tv_sec)
> +		inode->i_atime.tv_sec = ip->i_di.di_atime;
> +	if (force || ip->i_di.di_mtime > inode->i_mtime.tv_sec)
> +		inode->i_mtime.tv_sec = ip->i_di.di_mtime;
> +	if (force || ip->i_di.di_ctime > inode->i_ctime.tv_sec)
> +		inode->i_ctime.tv_sec = ip->i_di.di_ctime;
>   	inode->i_atime.tv_nsec = inode->i_mtime.tv_nsec = inode->i_ctime.tv_nsec = 0;
>   	inode->i_blocks = ip->i_di.di_blocks <<
>   		(ip->i_sbd->sd_sb.sb_bsize_shift - GFS_BASIC_BLOCK_SHIFT);
> @@ -125,7 +128,7 @@ gfs_inode_attr_in(struct gfs_inode *ip)
>   
>   	inode = gfs_iget(ip, NO_CREATE);
>   	if (inode) {
> -		inode_attr_in(ip, inode);
> +		inode_attr_in(ip, inode, 0);
>   		iput(inode);
>   	}
>   
> @@ -185,7 +188,7 @@ gfs_iget(struct gfs_inode *ip, int create)
>   	if (!tmp)
>   		return NULL;
>   
> -	inode_attr_in(ip, tmp);
> +	inode_attr_in(ip, tmp, 1);
>   
>   	/* Attach GFS-specific ops vectors */
>   	if (ip->i_di.di_type == GFS_FILE_REG) {
> diff --git a/gfs-kernel/src/gfs/ops_super.c b/gfs-kernel/src/gfs/ops_super.c
> index c111a2e..10dbff7 100644
> --- a/gfs-kernel/src/gfs/ops_super.c
> +++ b/gfs-kernel/src/gfs/ops_super.c
> @@ -39,6 +39,7 @@
>   #include "super.h"
>   #include "sys.h"
>   #include "mount.h"
> +#include "trans.h"
>   
>   /**
>    * gfs_write_inode - Make sure the inode is stable on the disk
> @@ -51,14 +52,52 @@
>   static int
>   gfs_write_inode(struct inode *inode, int sync)
>   {
> +	int ret = 0;
>   	struct gfs_inode *ip = get_v2ip(inode);
> +	struct buffer_head *dibh;
> +	struct gfs_holder i_gh;
> +	int need_unlock = 0;
> +
> +	if (!ip)
> +		return 0;
>   
>   	atomic_inc(&ip->i_sbd->sd_ops_super);
>   
> -	if (ip && sync)
> +	if (current->flags & PF_MEMALLOC || get_transaction)
> +		goto do_flush;
> +
> +	if (!gfs_glock_is_locked_by_me(ip->i_gl)) {
> +		ret = gfs_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &i_gh);
> +		if (ret)
> +			goto do_flush;
> +		need_unlock = 1;
> +	}
> +	else if (!gfs_glock_is_held_excl(ip->i_gl))
> +		goto do_flush;
> +	/* Trans may require:
> +	   one dinode block. */
> +	ret = gfs_trans_begin(ip->i_sbd, 1, 0);
> +	if (ret)
> +		goto do_unlock;
> +
> +	ret = gfs_get_inode_buffer(ip, &dibh);
> +	if (ret == 0) {
> +		gfs_inode_attr_out(ip);
> +		gfs_trans_add_bh(ip->i_gl, dibh);
> +		gfs_dinode_out(&ip->i_di, dibh->b_data);
> +		brelse(dibh);
> +	}
> +
> +	gfs_trans_end(ip->i_sbd);
> +
> +do_unlock:
> +	if (need_unlock)
> +		gfs_glock_dq_uninit(&i_gh);
> +do_flush:
> +	if (sync)
>   		gfs_log_flush_glock(ip->i_gl, 0);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   /**