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

[Mark Tinguely <tinguely@xxxxxxx>]

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>