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,
ASSERT(is_power_of_2(flags))).
> + spin_unlock(&ip->i_flags_lock);
> + return ret;
> +}
> +
. . .
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
