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

[Alex Elder <aelder@sgi.com>]

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@lst.de>

Reviewed-by: Alex Elder <aelder@sgi.com>

. . .

> @@ -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,
ASSERT(is_power_of_2(flags))).

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

. . .


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