Fix dnode_hold_impl() soft lockup

Soft lockups could happen when multiple threads trying
to get zrl on the same dnode handle in order to allocate
and initialize the dnode marked as DN_SLOT_ALLOCATED.

Don't loop from beginning when we can't get zrl, otherwise
we would increase the zrl refcount and nobody can actually
lock it.

Reviewed by: Tom Caputi <tcaputi@datto.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Li Dongyang <dongyangli@ddn.com>
Closes #8433

diff --git a/include/sys/zfs_context.h b/include/sys/zfs_context.h
index 11a32bb3117a314e1053832ce88b826d5bc9c034..260b8a458d73e92279ce694ad4737ad48827a7a8 100644 (file)
--- a/include/sys/zfs_context.h
+++ b/include/sys/zfs_context.h
@@ -236,6 +236,7 @@ extern kthread_t *zk_thread_create(void (*func)(void *), void *arg,
 
 #define        kpreempt_disable()      ((void)0)
 #define        kpreempt_enable()       ((void)0)
+#define        cond_resched()          sched_yield()
 
 /*
  * Mutexes

diff --git a/module/zfs/dnode.c b/module/zfs/dnode.c
index b7a7f90cf006c265e4065484975b29cfc7570d42..9a19ddabba5ea3094dcb5f9e694c079cfab5b5fe 100644 (file)
--- a/module/zfs/dnode.c
+++ b/module/zfs/dnode.c
@@ -1153,8 +1153,10 @@ dnode_free_interior_slots(dnode_t *dn)
 
        ASSERT3S(idx + slots, <=, DNODES_PER_BLOCK);
 
-       while (!dnode_slots_tryenter(children, idx, slots))
+       while (!dnode_slots_tryenter(children, idx, slots)) {
                DNODE_STAT_BUMP(dnode_free_interior_lock_retry);
+               cond_resched();
+       }
 
        dnode_set_slots(children, idx, slots, DN_SLOT_FREE);
        dnode_slots_rele(children, idx, slots);
@@ -1401,34 +1403,30 @@ dnode_hold_impl(objset_t *os, uint64_t object, int flag, int slots,
        }
 
        ASSERT(dnc->dnc_count == epb);
-       dn = DN_SLOT_UNINIT;
 
        if (flag & DNODE_MUST_BE_ALLOCATED) {
                slots = 1;
 
-               while (dn == DN_SLOT_UNINIT) {
-                       dnode_slots_hold(dnc, idx, slots);
-                       dnh = &dnc->dnc_children[idx];
-
-                       if (DN_SLOT_IS_PTR(dnh->dnh_dnode)) {
-                               dn = dnh->dnh_dnode;
-                               break;
-                       } else if (dnh->dnh_dnode == DN_SLOT_INTERIOR) {
-                               DNODE_STAT_BUMP(dnode_hold_alloc_interior);
-                               dnode_slots_rele(dnc, idx, slots);
-                               dbuf_rele(db, FTAG);
-                               return (SET_ERROR(EEXIST));
-                       } else if (dnh->dnh_dnode != DN_SLOT_ALLOCATED) {
-                               DNODE_STAT_BUMP(dnode_hold_alloc_misses);
-                               dnode_slots_rele(dnc, idx, slots);
-                               dbuf_rele(db, FTAG);
-                               return (SET_ERROR(ENOENT));
-                       }
+               dnode_slots_hold(dnc, idx, slots);
+               dnh = &dnc->dnc_children[idx];
 
+               if (DN_SLOT_IS_PTR(dnh->dnh_dnode)) {
+                       dn = dnh->dnh_dnode;
+               } else if (dnh->dnh_dnode == DN_SLOT_INTERIOR) {
+                       DNODE_STAT_BUMP(dnode_hold_alloc_interior);
                        dnode_slots_rele(dnc, idx, slots);
-                       if (!dnode_slots_tryenter(dnc, idx, slots)) {
+                       dbuf_rele(db, FTAG);
+                       return (SET_ERROR(EEXIST));
+               } else if (dnh->dnh_dnode != DN_SLOT_ALLOCATED) {
+                       DNODE_STAT_BUMP(dnode_hold_alloc_misses);
+                       dnode_slots_rele(dnc, idx, slots);
+                       dbuf_rele(db, FTAG);
+                       return (SET_ERROR(ENOENT));
+               } else {
+                       dnode_slots_rele(dnc, idx, slots);
+                       while (!dnode_slots_tryenter(dnc, idx, slots)) {
                                DNODE_STAT_BUMP(dnode_hold_alloc_lock_retry);
-                               continue;
+                               cond_resched();
                        }
 
                        /*
@@ -1463,45 +1461,43 @@ dnode_hold_impl(objset_t *os, uint64_t object, int flag, int slots,
                        return (SET_ERROR(ENOSPC));
                }
 
-               while (dn == DN_SLOT_UNINIT) {
-                       dnode_slots_hold(dnc, idx, slots);
-
-                       if (!dnode_check_slots_free(dnc, idx, slots)) {
-                               DNODE_STAT_BUMP(dnode_hold_free_misses);
-                               dnode_slots_rele(dnc, idx, slots);
-                               dbuf_rele(db, FTAG);
-                               return (SET_ERROR(ENOSPC));
-                       }
+               dnode_slots_hold(dnc, idx, slots);
 
+               if (!dnode_check_slots_free(dnc, idx, slots)) {
+                       DNODE_STAT_BUMP(dnode_hold_free_misses);
                        dnode_slots_rele(dnc, idx, slots);
-                       if (!dnode_slots_tryenter(dnc, idx, slots)) {
-                               DNODE_STAT_BUMP(dnode_hold_free_lock_retry);
-                               continue;
-                       }
+                       dbuf_rele(db, FTAG);
+                       return (SET_ERROR(ENOSPC));
+               }
 
-                       if (!dnode_check_slots_free(dnc, idx, slots)) {
-                               DNODE_STAT_BUMP(dnode_hold_free_lock_misses);
-                               dnode_slots_rele(dnc, idx, slots);
-                               dbuf_rele(db, FTAG);
-                               return (SET_ERROR(ENOSPC));
-                       }
+               dnode_slots_rele(dnc, idx, slots);
+               while (!dnode_slots_tryenter(dnc, idx, slots)) {
+                       DNODE_STAT_BUMP(dnode_hold_free_lock_retry);
+                       cond_resched();
+               }
 
-                       /*
-                        * Allocated but otherwise free dnodes which would
-                        * be in the interior of a multi-slot dnodes need
-                        * to be freed.  Single slot dnodes can be safely
-                        * re-purposed as a performance optimization.
-                        */
-                       if (slots > 1)
-                               dnode_reclaim_slots(dnc, idx + 1, slots - 1);
+               if (!dnode_check_slots_free(dnc, idx, slots)) {
+                       DNODE_STAT_BUMP(dnode_hold_free_lock_misses);
+                       dnode_slots_rele(dnc, idx, slots);
+                       dbuf_rele(db, FTAG);
+                       return (SET_ERROR(ENOSPC));
+               }
 
-                       dnh = &dnc->dnc_children[idx];
-                       if (DN_SLOT_IS_PTR(dnh->dnh_dnode)) {
-                               dn = dnh->dnh_dnode;
-                       } else {
-                               dn = dnode_create(os, dn_block + idx, db,
-                                   object, dnh);
-                       }
+               /*
+                * Allocated but otherwise free dnodes which would
+                * be in the interior of a multi-slot dnodes need
+                * to be freed.  Single slot dnodes can be safely
+                * re-purposed as a performance optimization.
+                */
+               if (slots > 1)
+                       dnode_reclaim_slots(dnc, idx + 1, slots - 1);
+
+               dnh = &dnc->dnc_children[idx];
+               if (DN_SLOT_IS_PTR(dnh->dnh_dnode)) {
+                       dn = dnh->dnh_dnode;
+               } else {
+                       dn = dnode_create(os, dn_block + idx, db,
+                           object, dnh);
                }
 
                mutex_enter(&dn->dn_mtx);