Re: [PATCH] repair: avoid ABBA deadlocks on prefetched buffers

Christoph Hellwig
2011-11-22T22:46:23Z

On Fri, Nov 18, 2011 at 09:44:09AM +0100, Arkadiusz Mi??kiewicz wrote:
> On Tuesday 15 of November 2011, Christoph Hellwig wrote:
> > Both the prefetch threads and actual repair processing threads can have
> > multiple buffers at a time locked, but they do no use a common locker
> > order, which can lead to ABBA deadlocks while trying to lock the buffers.
>
> There is still some issue with deadlocking.
>
> The last printed messages:
> b????dna liczba magiczna 0x41425443 w bloku inobt 2/1438099
> b????dna liczba magiczna 0x41425443 w bloku inobt 2/1438196
> b????dna liczba magiczna 0x41425443 w bloku inobt 2/1438732
> (invalid magic number ... in block inobt ...)

It looks like you have a circular loop in the inobt tree, and repair
deadlocks trying to read the same node again. Below is a patch working
around that by allowing recursive locking for the buffer lock and then
letting the normal two strikes and out policy apply. I'm not overly
proud of the patch, but in the short term I can't think of anything
better.

Index: xfsprogs-dev/include/libxfs.h
===================================================================
--- xfsprogs-dev.orig/include/libxfs.h 2011-11-22 22:28:23.000000000 +0000
+++ xfsprogs-dev/include/libxfs.h 2011-11-22 22:34:27.000000000 +0000
@@ -226,6 +226,8 @@ typedef struct xfs_buf {
unsigned b_bcount;
dev_t b_dev;
pthread_mutex_t b_lock;
+ pthread_t b_holder;
+ unsigned int b_recur;
void *b_fsprivate;
void *b_fsprivate2;
void *b_fsprivate3;
Index: xfsprogs-dev/libxfs/rdwr.c
===================================================================
--- xfsprogs-dev.orig/libxfs/rdwr.c 2011-11-22 22:28:23.000000000 +0000
+++ xfsprogs-dev/libxfs/rdwr.c 2011-11-22 22:40:01.000000000 +0000
@@ -342,6 +342,8 @@ libxfs_initbuf(xfs_buf_t *bp, dev_t devi
list_head_init(&bp->b_lock_list);
#endif
pthread_mutex_init(&bp->b_lock, NULL);
+ bp->b_holder = 0;
+ bp->b_recur = 0;
}

xfs_buf_t *
@@ -410,18 +412,24 @@ libxfs_getbuf_flags(dev_t device, xfs_da
return NULL;

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"));
+ bp->b_recur++;
+ } else {
+ pthread_mutex_lock(&bp->b_lock);
}
- } else {
- pthread_mutex_lock(&bp->b_lock);
}
+
+ bp->b_holder = pthread_self();
}

cache_node_set_priority(libxfs_bcache, (struct cache_node *)bp,
@@ -440,6 +448,9 @@ libxfs_getbuf_flags(dev_t device, xfs_da
#endif

return bp;
+out_put:
+ cache_node_put(libxfs_bcache, (struct cache_node *)bp);
+ return NULL;
}

struct xfs_buf *
@@ -458,8 +469,14 @@ libxfs_putbuf(xfs_buf_t *bp)
list_del_init(&bp->b_lock_list);
pthread_mutex_unlock(&libxfs_bcache->c_mutex);
#endif
- if (use_xfs_buf_lock)
- pthread_mutex_unlock(&bp->b_lock);
+ if (use_xfs_buf_lock) {
+ if (bp->b_recur) {
+ bp->b_recur--;
+ } else {
+ bp->b_holder = 0;
+ pthread_mutex_unlock(&bp->b_lock);
+ }
+ }
cache_node_put(libxfs_bcache, (struct cache_node *)bp);
}