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: Alex Elder <aelder@xxxxxxx>
Date: Wed, 26 Oct 2011 16:07:33 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20111019182421.048260722@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111019182343.762985925@xxxxxxxxxxxxxxxxxxxxxx> <20111019182421.048260722@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-10-19 at 14:23 -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>

One minor suggestion, plus some discussion from
inside my head below.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

. . .

> @@ -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_BIT);
> +     DEFINE_WAIT_BIT(q, &ip->i_flags, __XFS_IPINNED_BIT);

In your last patch, you used "wait" as the name
rather than "q".  Minor consistency nit...

> +     xfs_iunpin(ip);
> +

This initially struck me as unsafe or something,
assuming the inode was pinned.  But I was thinking
of it more like an unlock request, which it is not.
It's more like unplugging something so the inode
will eventually get unpinned.  (Just thinking aloud
here, nevermind me...)

> +     do {
> +             prepare_to_wait(wq, &q.wait, TASK_UNINTERRUPTIBLE);
> +             if (xfs_ipincount(ip))
> +                     io_schedule();
> +     } while (xfs_ipincount(ip));
> +     finish_wait(wq, &q.wait);
> +}
> +
. . .

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