DF-5037 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 | --- a/sys/dev/disk/dm/dm_pdev.c +++ b/sys/dev/disk/dm/dm_pdev.c @@ -115,11 +115,24 @@ * Create entry for device with name dev_name and open vnode for it. * If entry already exists in global TAILQ I will only increment * reference counter. + * + * NOTE: vn_open()/VOP_GETATTR()/dev_dioctl() below are blocking VFS + * operations that can re-enter the device-mapper subsystem (dm-on-dm + * stacking: vn_open of /dev/mapper/X runs dmopen() -> dm_dev_lookup() + * which takes dm_dev_mutex). They MUST NOT run while dm_pdev_mutex is + * held: dm_dev.c holds dm_dev_mutex across reference-drain waits and + * table destruction (-> target destroy -> dm_pdev_decr -> + * dm_pdev_mutex), so holding dm_pdev_mutex across the vnode open + * creates an AB-BA lock cycle that permanently deadlocks the whole dm + * subsystem. dm_pdev_mutex is therefore only held for the list + * lookup/insertion; a concurrent insert of the same name is resolved + * by re-checking the list under the lock after the vnode was opened. */ dm_pdev_t * dm_pdev_insert(const char *dev_name) { dm_pdev_t *dmp; + dm_pdev_t *dmp2; struct vattr va; int error; @@ -134,18 +147,18 @@ lockmgr(&dm_pdev_mutex, LK_RELEASE); return dmp; } + lockmgr(&dm_pdev_mutex, LK_RELEASE); if ((dmp = dm_pdev_alloc(dev_name)) == NULL) { - lockmgr(&dm_pdev_mutex, LK_RELEASE); return NULL; } + /* Open the backing device WITHOUT holding dm_pdev_mutex. */ error = dm_dk_lookup(dev_name, &dmp->pdev_vnode); if (error) { dmdebug("Lookup on %s failed with error %d!\n", dev_name, error); dm_pdev_free(dmp); - lockmgr(&dm_pdev_mutex, LK_RELEASE); return NULL; } dmp->ref_cnt = 1; @@ -153,7 +166,6 @@ if (dm_pdev_get_vattr(dmp, &va) == -1) { dmdebug("getattr on %s failed\n", dev_name); dm_pdev_free(dmp); - lockmgr(&dm_pdev_mutex, LK_RELEASE); return NULL; } ksnprintf(dmp->udev_name, sizeof(dmp->udev_name), @@ -178,6 +190,19 @@ kprintf("dmp_pdev_insert DIOCGPART failed %d\n", error); } + /* + * Re-take the lock and insert, unless another thread created a + * pdev with the same name while we had the lock dropped. + */ + lockmgr(&dm_pdev_mutex, LK_EXCLUSIVE); + dmp2 = dm_pdev_lookup_name(dev_name); + if (dmp2 != NULL) { + /* Lost the race: bump the existing entry and drop ours. */ + dmp2->ref_cnt++; + lockmgr(&dm_pdev_mutex, LK_RELEASE); + dm_pdev_free(dmp); + return dmp2; + } TAILQ_INSERT_TAIL(&dm_pdev_list, dmp, next_pdev); lockmgr(&dm_pdev_mutex, LK_RELEASE); --- a/sys/dev/disk/dm/dm_dev.c +++ b/sys/dev/disk/dm/dm_dev.c @@ -61,6 +61,14 @@ /* * dm_dev_mutex must be held by caller before using disable_dev. + * + * NOTE: disable_dev only unlinks the device from the global list now. + * It must NOT wait for ref_cnt to drain (or destroy the device) while + * dm_dev_mutex is held: a thread holding a dm_dev busy reference may + * need dm_dev_mutex itself before it can unbusy (dm-on-dm stacking: + * dmopen/dmclose -> dm_dev_lookup inside a table load/destroy), so + * sleeping on the reference count with dm_dev_mutex held deadlocks the + * whole subsystem. */ static void disable_dev(dm_dev_t *dmv) @@ -69,7 +77,15 @@ TAILQ_REMOVE(&dm_dev_list, dmv, next_devlist); dm_dev_counter--; +} +/* + * Wait for all busy references to drain. Must be called WITHOUT any + * dm locks held (see the note above disable_dev). + */ +static void +dm_dev_wait_refs(dm_dev_t *dmv) +{ lockmgr(&dmv->dev_mtx, LK_EXCLUSIVE); while (dmv->ref_cnt != 0) cv_wait(&dmv->dev_cv, &dmv->dev_mtx); @@ -304,11 +320,14 @@ int dm_dev_remove(dm_dev_t *dmv) { - /* Remove device from list and wait for refcnt to drop to zero */ + /* Remove device from list and wait for refcnt to drop to zero, + * then destroy -- all without holding dm_dev_mutex. */ lockmgr(&dm_dev_mutex, LK_EXCLUSIVE); disable_dev(dmv); lockmgr(&dm_dev_mutex, LK_RELEASE); + dm_dev_wait_refs(dmv); + /* Destroy and free the device */ dm_dev_destroy(dmv); |