xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 14 Oct 2013 15:50:07 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131012075639.940898263@xxxxxxxxxxxxxxxxxxxxxx>
References: <20131012075503.370510641@xxxxxxxxxxxxxxxxxxxxxx> <20131012075639.940898263@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

<Prev in Thread] Current Thread [Next in Thread>