Re: [patch 7/7] fs: fix or note I_DIRTY handling bugs in filesystems

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Wed, 2010-11-24 at 01:06 +1100, npiggin@kernel.dk wrote:
> plain text document attachment (fs-fix-dirty-flags.patch)
> Comments?
> 
> Index: linux-2.6/drivers/staging/pohmelfs/inode.c
> ===================================================================
> --- linux-2.6.orig/drivers/staging/pohmelfs/inode.c	2010-11-23 22:57:45.000000000 +1100
> +++ linux-2.6/drivers/staging/pohmelfs/inode.c	2010-11-23 22:59:47.000000000 +1100
> @@ -373,6 +373,7 @@ static int pohmelfs_write_inode_create_c
>  		dprintk("%s: parent: %llu, ino: %llu, inode: %p.\n",
>  				__func__, parent->ino, n->ino, inode);
>  
> +		/* XXX: is this race WRT writeback? */
>  		if (inode && (inode->i_state & I_DIRTY)) {
>  			struct pohmelfs_inode *pi = POHMELFS_I(inode);
>  			pohmelfs_write_create_inode(pi);
> Index: linux-2.6/fs/gfs2/file.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/file.c	2010-11-23 22:54:47.000000000 +1100
> +++ linux-2.6/fs/gfs2/file.c	2010-11-24 00:58:42.000000000 +1100
> @@ -557,23 +557,43 @@ static int gfs2_close(struct inode *inod
>  static int gfs2_fsync(struct file *file, int datasync)
>  {
>  	struct inode *inode = file->f_mapping->host;
> -	int sync_state = inode->i_state & (I_DIRTY_SYNC|I_DIRTY_DATASYNC);
> +	unsigned dirty, mask;
>  	int ret = 0;
>  
> -	/* XXX: testing i_state is broken without proper synchronization */
> -
>  	if (gfs2_is_jdata(GFS2_I(inode))) {
>  		gfs2_log_flush(GFS2_SB(inode), GFS2_I(inode)->i_gl);
>  		return 0;
>  	}
>  
> -	if (sync_state != 0) {
> -		if (!datasync)
> -			ret = write_inode_now(inode, 0);
> +	spin_lock(&inode_lock);
> +	inode_writeback_begin(inode, 1);
> +
> +	if (datasync)
> +		mask = I_DIRTY_DATASYNC;
> +	else
> +		mask = I_DIRTY_SYNC | I_DIRTY_DATASYNC;
> +	dirty = inode->i_state & mask;
> +	inode->i_state &= ~mask;
> +	if (dirty) {
> +		spin_unlock(&inode_lock);
> +
> +		if (!datasync) {
> +			struct writeback_control wbc = {
> +				.sync_mode = WB_SYNC_ALL,
> +			};
> +			ret = inode->i_sb->s_op->write_inode(inode, &wbc);
> +		} else {
> +			if (gfs2_is_stuffed(GFS2_I(inode)))
> +				gfs2_log_flush(GFS2_SB(inode),
> +						GFS2_I(inode)->i_gl);
> +		}
>  
> -		if (gfs2_is_stuffed(GFS2_I(inode)))
> -			gfs2_log_flush(GFS2_SB(inode), GFS2_I(inode)->i_gl);
> +		spin_lock(&inode_lock);
>  	}
> +	if (ret)
> +		inode->i_state |= dirty;
> +	inode_writeback_end(inode);
> +	spin_unlock(&inode_lock);
>  
>  	return ret;
>  }

The GFS2 changes seem to make sense to me, so:
Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.