public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
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

  reply	other threads:[~2013-10-14  4:50 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-12  7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
2013-10-12  7:55 ` [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
2013-10-14  4:50   ` Dave Chinner [this message]
2013-10-14  7:29     ` Christoph Hellwig
2013-10-14 14:09   ` [PATCH 1/5 v3] " Christoph Hellwig
2013-10-14 20:28     ` Dave Chinner
2013-10-12  7:55 ` [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
2013-10-14  4:51   ` Dave Chinner
2013-10-17 20:01     ` Ben Myers
2013-10-17 20:03       ` Christoph Hellwig
2013-10-17 21:17         ` Ben Myers
2013-10-12  7:55 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
2013-10-14  4:55   ` Dave Chinner
2013-10-12  7:55 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
2013-10-14  5:04   ` Dave Chinner
2013-10-14  7:30     ` Christoph Hellwig
2013-10-14 20:03       ` Dave Chinner
2013-10-12  7:55 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
2013-10-14  5:08   ` Dave Chinner
2013-10-15 15:31     ` Christoph Hellwig
2013-10-15 21:47       ` Dave Chinner
2013-10-16  7:03         ` Christoph Hellwig
2013-10-21 21:58 ` [PATCH 0/5] refactor the preallocation and hole punching code Ben Myers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131014045007.GF4446@dastard \
    --to=david@fromorbit.com \
    --cc=hch@infradead.org \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox