xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: Re: [PATCH 06/12] repair: use recursive buffer locking
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 13 Jan 2012 14:10:46 -0600
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20111218225445.GB20578@xxxxxxxxxxxxx>
References: <20111202174619.179530033@xxxxxxxxxxxxxxxxxxxxxx> <20111202174742.106589485@xxxxxxxxxxxxxxxxxxxxxx> <20111213022208.GZ14273@dastard> <20111218225445.GB20578@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.24) Gecko/20111206 Thunderbird/3.1.16
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@xxxxxxx>



<Prev in Thread] Current Thread [Next in Thread>
  • Re: Re: [PATCH 06/12] repair: use recursive buffer locking, Mark Tinguely <=