[Top] [All Lists]

Re: [PATCH 2/4] xfs: replace i_flock with a sleeping bitlock

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: replace i_flock with a sleeping bitlock
From: Alex Elder <aelder@xxxxxxx>
Date: Wed, 26 Oct 2011 16:07:21 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20111019182420.881974453@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111019182343.762985925@xxxxxxxxxxxxxxxxxxxxxx> <20111019182420.881974453@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-10-19 at 14:23 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-kill-i_flush)
> We almost never block on i_flock, the exception is synchronous inode
> flushing.  Instead of bloating the inode with a 16/24-byte completion
> that we abuse as a semaphore just implement it as a bitlock that uses
> a bit waitqueue for the rare sleeping path.  This primarily is a
> tradeoff between a much smaller inode and a faster non-blocking
> path vs a faster faster wakeups, and we are much better off with
       vs faster wakeups
> the former.
> A small downside is that we will lose lockdep checking for i_flock, but
> given that it's always taken inside the ilock that should be acceptable.
> Note that for example the inode writeback locking is implemented in a
> very similar way.

Substitute "beeing" -> "being" throughout.  There's
also one thing I'd like you to check and likely fix,
below.  Otherwise looks good.

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Reviewed-by: Alex Elder <aelder@xxxxxxx>

. . .

> @@ -331,6 +330,19 @@ xfs_iflags_test_and_clear(xfs_inode_t *i
>       return ret;
>  }
> +static inline int
> +xfs_iflags_test_and_set(xfs_inode_t *ip, unsigned short flags)

i_flags is now an unsigned long (so make the
flags argument here match that type).

> +{
> +     int ret;
> +
> +     spin_lock(&ip->i_flags_lock);
> +     ret = ip->i_flags & flags;
> +     if (!ret)
> +             ip->i_flags |= flags;

Although you are now only passing in a single
flag bit, the interface doesn't preclude you
passing in multiple bits.

Therefore I think the correct logic would be:

        ret = (ip->i_flags & flags) != flags;
        if (ret)
                ip->flags |= flags;

Either that, or change the name of the "flags"
argument to better reflect that we really want
a single lock bit provided (and perhaps,

> +     spin_unlock(&ip->i_flags_lock);
> +     return ret;
> +}
> +

. . .

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