Re: [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size
Date: Mon, 14 Oct 2013 15:50:07 +1100	[thread overview]
Message-ID: <20131014045007.GF4446@dastard> (raw)
In-Reply-To: <20131012075639.940898263@bombadil.infradead.org>

On Sat, Oct 12, 2013 at 12:55:04AM -0700, Christoph Hellwig wrote:
> There is no reason to conditionally take the iolock inside xfs_setattr_size
> when we can let the caller handle it unconditionally, which just incrases
> the lock hold time for the case where it was previously taken internally
> by a few instructions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> ---
>  fs/xfs/xfs_bmap_util.c |    3 +--
>  fs/xfs/xfs_file.c      |    2 +-
>  fs/xfs/xfs_iops.c      |   29 ++++++++++++++++-------------
>  fs/xfs/xfs_iops.h      |    2 +-
>  4 files changed, 19 insertions(+), 17 deletions(-)
> 
> Index: xfs/fs/xfs/xfs_file.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_file.c	2013-10-01 21:20:47.564230097 +0200
> +++ xfs/fs/xfs/xfs_file.c	2013-10-01 21:20:54.312230257 +0200
> @@ -852,7 +852,7 @@ xfs_file_fallocate(
>  
>  		iattr.ia_valid = ATTR_SIZE;
>  		iattr.ia_size = new_size;
> -		error = -xfs_setattr_size(ip, &iattr, XFS_ATTR_NOLOCK);
> +		error = -xfs_setattr_size(ip, &iattr);
>  	}
>  
>  out_unlock:
> Index: xfs/fs/xfs/xfs_iops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iops.c	2013-10-01 21:20:47.564230097 +0200
> +++ xfs/fs/xfs/xfs_iops.c	2013-10-01 21:20:54.316230257 +0200
> @@ -709,8 +709,7 @@ out_dqrele:
>  int
>  xfs_setattr_size(
>  	struct xfs_inode	*ip,
> -	struct iattr		*iattr,
> -	int			flags)
> +	struct iattr		*iattr)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> @@ -733,15 +732,11 @@ xfs_setattr_size(
>  	if (error)
>  		return XFS_ERROR(error);
>  
> +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
>  	ASSERT(S_ISREG(ip->i_d.di_mode));
>  	ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
>  			ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>  
> -	if (!(flags & XFS_ATTR_NOLOCK)) {
> -		lock_flags |= XFS_IOLOCK_EXCL;
> -		xfs_ilock(ip, lock_flags);
> -	}
> -

I think there should be a bit more cleanup of this lock_flags
variable. There's quite a few error paths that now "goto out_unlock"
without actually holding a lock. i.e. they could just become a
straight "return error", and lock_flags can probably go away
completely...

Otherwise it look s good to me.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs