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

[Mark Tinguely]

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 at sgi.com>