xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/4] xfs: replace i_pin_wait with a bit waitqueue
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 19 Oct 2011 11:50:36 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111018201405.557330194@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111018201304.279051318@xxxxxxxxxxxxxxxxxxxxxx> <20111018201405.557330194@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxx>
> 
> 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@xxxxxxxxxxxxx

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