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@xxxxxx>
>
> ---
> 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
