Re: Re: [PATCH 06/12] repair: use recursive buffer locking

Mark Tinguely
2012-01-13T20:10:46Z

On 01/-10/63 13:59, Christoph Hellwig wrote:
> On Tue, Dec 13, 2011 at 01:22:08PM +1100, Dave Chinner wrote:
>>> if (use_xfs_buf_lock) {
>>> - if (flags& LIBXFS_GETBUF_TRYLOCK) {
>>> - int ret;
>>> + int ret;
>>>
>>> - ret = pthread_mutex_trylock(&bp->b_lock);
>>> - if (ret) {
>>> - ASSERT(ret == EAGAIN);
>>> - cache_node_put(libxfs_bcache, (struct cache_node *)bp);
>>> - return NULL;
>>> + ret = pthread_mutex_trylock(&bp->b_lock);
>>> + if (ret) {
>>> + ASSERT(ret == EAGAIN);
>>> + if (flags& LIBXFS_GETBUF_TRYLOCK)
>>> + goto out_put;
>>> +
>>> + if (pthread_equal(bp->b_holder, pthread_self())) {
>>> + fprintf(stderr,
>>> + _("recursive buffer locking detected\n"));
>>
>> "Warning: recursive buffer locking @ bno %lld detected"
>>
>> might be more informative, especially to do with the severity of the
>> issue.
>
> Ok, I'll make it print the block number.
>
>>
>>> + bp->b_recur++;
>>> + } else {
>>> + pthread_mutex_lock(&bp->b_lock);
>>> }
>>> - } else {
>>> - pthread_mutex_lock(&bp->b_lock);
>>> }
>>> +
>>> + bp->b_holder = pthread_self();
>>
>> That should probably only be written in the branch where the lock is
>> taken not every time through here.
>
> We actually should return the buffer just after incrementing the
> recursion count, else we might add it to the global list of buffers
> twice.
>
>

I liked the patch the way it was.

I thought the placement of the setting of b_holder is correct because it
covers the pthread_mutex_trylock and the pthread_mutex_lock attempts.

I did not see anything in cache_node_get_priority() nor
cache_node_set_priority() that would add this to a list.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>