Re: [PATCH 3/4] xfs: replace i_pin_wait with a bit waitqueue

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 3/4] xfs: replace i_pin_wait with a bit waitqueue
Date: Wed, 19 Oct 2011 11:50:36 +1100	[thread overview]
Message-ID: <20111019005036.GC21338@dastard> (raw)
In-Reply-To: <20111018201405.557330194@bombadil.infradead.org>

On Tue, Oct 18, 2011 at 04:13:07PM -0400, Christoph Hellwig wrote:
> Replace i_pin_wait, which is only used during synchronous inode flushing
> with a bit waitqueue.  This trades off a much smaller inode against
> slightly slower wakeup performance, and saves 12 (32-bit) or 20 (64-bit)
> bytes in the XFS inode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_inode.c	2011-10-18 20:58:40.141854067 +0200
> +++ xfs/fs/xfs/xfs_inode.c	2011-10-18 21:01:48.296353322 +0200
> @@ -2151,7 +2151,7 @@ xfs_idestroy_fork(
>   * once someone is waiting for it to be unpinned.
>   */
>  static void
> -xfs_iunpin_nowait(
> +xfs_iunpin(
>  	struct xfs_inode	*ip)
>  {
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> @@ -2163,14 +2163,29 @@ xfs_iunpin_nowait(
>  
>  }
>  
> +static void
> +__xfs_iunpin_wait(
> +	struct xfs_inode	*ip)
> +{
> +	wait_queue_head_t *wq = bit_waitqueue(&ip->i_flags, __XFS_IPINNED);
> +	DEFINE_WAIT_BIT(q, &ip->i_flags, __XFS_IPINNED);
> +
> +	xfs_iunpin(ip);
> +
> +	do {
> +		prepare_to_wait(wq, &q.wait, TASK_UNINTERRUPTIBLE);
> +		if (xfs_ipincount(ip))
> +			schedule();
> +	} while (xfs_ipincount(ip));
> +	finish_wait(wq, &q.wait);
> +}

Same comment about io_schedule() here - it's an IO we're waiting to
complete here. And it's not an exclusive wait because we can have
multiple callers waiting on the inode being unpinned and we want
them all woken in one go?

> @@ -374,6 +373,7 @@ xfs_set_projid(struct xfs_inode *ip,
>  #define XFS_IDIRTY_RELEASE	0x0040	/* dirty release already seen */
>  #define __XFS_IFLOCK		8	/* inode is beeing flushed right now */
>  #define XFS_IFLOCK		(1 << __XFS_IFLOCK)
> +#define __XFS_IPINNED		9	/* wakeup key for zero pin count */

Should you also define XFS_IPINNED for consistency, even though it
is not used?

Otherwise looks OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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