repair: avoid ABBA deadlocks on prefetched buffers

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.

Switch the prefetch code to do a trylock and skip buffers that have
already been locked to avoid this deadlock.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>

diff --git a/include/libxfs.h b/include/libxfs.h
index f92ef6fbea222214d8ecc33ba584e247da9a27a2..893173279837027a1f42c3fcaef9f1612281b8d0 100644 (file)
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -279,27 +279,41 @@ enum xfs_buf_flags_t {    /* b_flags bits */
 extern struct cache    *libxfs_bcache;
 extern struct cache_operations libxfs_bcache_operations;
 
+#define LIBXFS_GETBUF_TRYLOCK  (1 << 0)
+
 #ifdef XFS_BUF_TRACING
 
 #define libxfs_readbuf(dev, daddr, len, flags) \
-               libxfs_trace_readbuf(__FUNCTION__, __FILE__, __LINE__, (dev), (daddr), (len), (flags))
+       libxfs_trace_readbuf(__FUNCTION__, __FILE__, __LINE__, \
+                           (dev), (daddr), (len), (flags))
 #define libxfs_writebuf(buf, flags) \
-               libxfs_trace_writebuf(__FUNCTION__, __FILE__, __LINE__, (buf), (flags))
+       libxfs_trace_writebuf(__FUNCTION__, __FILE__, __LINE__, \
+                             (buf), (flags))
 #define libxfs_getbuf(dev, daddr, len) \
-               libxfs_trace_getbuf(__FUNCTION__, __FILE__, __LINE__, (dev), (daddr), (len))
+       libxfs_trace_getbuf(__FUNCTION__, __FILE__, __LINE__, \
+                           (dev), (daddr), (len))
+#define libxfs_getbuf_flags(dev, daddr, len, flags) \
+       libxfs_trace_getbuf(__FUNCTION__, __FILE__, __LINE__, \
+                           (dev), (daddr), (len), (flags))
 #define libxfs_putbuf(buf) \
-               libxfs_trace_putbuf(__FUNCTION__, __FILE__, __LINE__, (buf))
+       libxfs_trace_putbuf(__FUNCTION__, __FILE__, __LINE__, (buf))
 
-extern xfs_buf_t *libxfs_trace_readbuf(const char *, const char *, int, dev_t, xfs_daddr_t, int, int);
-extern int     libxfs_trace_writebuf(const char *, const char *, int, xfs_buf_t *, int);
+extern xfs_buf_t *libxfs_trace_readbuf(const char *, const char *, int,
+                       dev_t, xfs_daddr_t, int, int);
+extern int     libxfs_trace_writebuf(const char *, const char *, int,
+                       xfs_buf_t *, int);
 extern xfs_buf_t *libxfs_trace_getbuf(const char *, const char *, int, dev_t, xfs_daddr_t, int);
-extern void    libxfs_trace_putbuf (const char *, const char *, int, xfs_buf_t *);
+extern xfs_buf_t *libxfs_trace_getbuf_flags(const char *, const char *, int,
+                       dev_t, xfs_daddr_t, int, unsigned int);
+extern void    libxfs_trace_putbuf (const char *, const char *, int,
+                       xfs_buf_t *);
 
 #else
 
 extern xfs_buf_t *libxfs_readbuf(dev_t, xfs_daddr_t, int, int);
 extern int     libxfs_writebuf(xfs_buf_t *, int);
 extern xfs_buf_t *libxfs_getbuf(dev_t, xfs_daddr_t, int);
+extern xfs_buf_t *libxfs_getbuf_flags(dev_t, xfs_daddr_t, int, unsigned int);
 extern void    libxfs_putbuf (xfs_buf_t *);
 
 #endif

diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index c3edb89e1fb0a94f1c8abac2481d197b0b8bf0fe..dbd94c50f8e59fc111dd22950f6f1ccfec470231 100644 (file)
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -195,6 +195,7 @@ libxfs_log_header(
 #undef libxfs_readbuf
 #undef libxfs_writebuf
 #undef libxfs_getbuf
+#undef libxfs_getbuf_flags
 #undef libxfs_putbuf
 
 xfs_buf_t      *libxfs_readbuf(dev_t, xfs_daddr_t, int, int);
@@ -238,6 +239,19 @@ libxfs_trace_getbuf(const char *func, const char *file, int line, dev_t device,
        return bp;
 }
 
+xfs_buf_t *
+libxfs_trace_getbuf_flags(const char *func, const char *file, int line,
+               dev_t device, xfs_daddr_t blkno, int len, unsigned long flags)
+{
+       xfs_buf_t       *bp = libxfs_getbuf(device, blkno, len, flags);
+
+       bp->b_func = func;
+       bp->b_file = file;
+       bp->b_line = line;
+
+       return bp;
+}
+
 void
 libxfs_trace_putbuf(const char *func, const char *file, int line, xfs_buf_t *bp)
 {
@@ -380,8 +394,8 @@ int                 lock_buf_count = 0;
 
 extern int     use_xfs_buf_lock;
 
-xfs_buf_t *
-libxfs_getbuf(dev_t device, xfs_daddr_t blkno, int len)
+struct xfs_buf *
+libxfs_getbuf_flags(dev_t device, xfs_daddr_t blkno, int len, unsigned int flags)
 {
        xfs_buf_t       *bp;
        xfs_bufkey_t    key;
@@ -392,28 +406,48 @@ libxfs_getbuf(dev_t device, xfs_daddr_t blkno, int len)
        key.bblen = len;
 
        miss = cache_node_get(libxfs_bcache, &key, (struct cache_node **)&bp);
-       if (bp) {
-               if (use_xfs_buf_lock)
+       if (!bp)
+               return NULL;
+
+       if (use_xfs_buf_lock) {
+               if (flags & LIBXFS_GETBUF_TRYLOCK) {
+                       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;
+                       }
+               } else {
                        pthread_mutex_lock(&bp->b_lock);
-               cache_node_set_priority(libxfs_bcache, (struct cache_node *)bp,
-                       cache_node_get_priority((struct cache_node *)bp) -
+               }
+       }
+
+       cache_node_set_priority(libxfs_bcache, (struct cache_node *)bp,
+               cache_node_get_priority((struct cache_node *)bp) -
                                                CACHE_PREFETCH_PRIORITY);
 #ifdef XFS_BUF_TRACING
-               pthread_mutex_lock(&libxfs_bcache->c_mutex);
-               lock_buf_count++;
-               list_add(&bp->b_lock_list, &lock_buf_list);
-               pthread_mutex_unlock(&libxfs_bcache->c_mutex);
+       pthread_mutex_lock(&libxfs_bcache->c_mutex);
+       lock_buf_count++;
+       list_add(&bp->b_lock_list, &lock_buf_list);
+       pthread_mutex_unlock(&libxfs_bcache->c_mutex);
 #endif
 #ifdef IO_DEBUG
-               printf("%lx %s: %s buffer %p for bno = %llu\n",
-                       pthread_self(), __FUNCTION__, miss ? "miss" : "hit",
-                       bp, (long long)LIBXFS_BBTOOFF64(blkno));
+       printf("%lx %s: %s buffer %p for bno = %llu\n",
+               pthread_self(), __FUNCTION__, miss ? "miss" : "hit",
+               bp, (long long)LIBXFS_BBTOOFF64(blkno));
 #endif
-       }
 
        return bp;
 }
 
+struct xfs_buf *
+libxfs_getbuf(dev_t device, xfs_daddr_t blkno, int len)
+{
+       return libxfs_getbuf_flags(device, blkno, len, 0);
+}
+
 void
 libxfs_putbuf(xfs_buf_t *bp)
 {

diff --git a/repair/prefetch.c b/repair/prefetch.c
index da074a89cac085aa7e78df5d705163dbad082263..0e1167ec63dbeec9dbd03e62e4007223507a41f9 100644 (file)
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -112,8 +112,17 @@ pf_queue_io(
 {
        xfs_buf_t               *bp;
 
-       bp = libxfs_getbuf(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
-                       XFS_FSB_TO_BB(mp, blen));
+       /*
+        * Never block on a buffer lock here, given that the actual repair
+        * code might lock buffers in a different order from us.  Given that
+        * the lock holder is either reading it from disk himself or
+        * completely overwriting it this behaviour is perfectly fine.
+        */
+       bp = libxfs_getbuf_flags(mp->m_dev, XFS_FSB_TO_DADDR(mp, fsbno),
+                       XFS_FSB_TO_BB(mp, blen), LIBXFS_GETBUF_TRYLOCK);
+       if (!bp)
+               return;
+
        if (bp->b_flags & LIBXFS_B_UPTODATE) {
                if (B_IS_INODE(flag))
                        pf_read_inode_dirs(args, bp);