]> git.proxmox.com Git - mirror_zfs.git/commitdiff
Introduce zv_state_lock
authorBoris Protopopov <boris.protopopov@actifio.com>
Wed, 10 May 2017 17:51:29 +0000 (13:51 -0400)
committerBrian Behlendorf <behlendorf1@llnl.gov>
Tue, 16 May 2017 23:44:06 +0000 (19:44 -0400)
The lock is designed to protect internal state of zvol_state_t and
to avoid taking spa_namespace_lock (e.g. in dmu_objset_own() code path)
while holding zvol_stat_lock. Refactor the code accordingly.

Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #3484
Closes #6065
Closes #6134

module/zfs/zvol.c

index 3bf28e1d40c37aed1bcb57b4df41080fc88e7254..121f75e4e257de77e5e637cb64dc062104446a70 100644 (file)
  * Copyright (c) 2016 Actifio, Inc. All rights reserved.
  */
 
+/*
+ * Note on locking of zvol state structures.
+ *
+ * These structures are used to maintain internal state used to emulate block
+ * devices on top of zvols. In particular, management of device minor number
+ * operations - create, remove, rename, and set_snapdev - involves access to
+ * these structures. The zvol_state_lock is primarily used to protect the
+ * zvol_state_list. The zv->zv_state_lock is used to protect the contents
+ * of the zvol_state_t structures, as well as to make sure that when the
+ * time comes to remove the structure from the list, it is not in use, and
+ * therefore, it can be taken off zvol_state_list and freed.
+ *
+ * The minor operations are issued to the spa->spa_zvol_taskq quues, that are
+ * single-threaded (to preserve order of minor operations), and are executed
+ * through the zvol_task_cb that dispatches the specific operations. Therefore,
+ * these operations are serialized per pool. Consequently, we can be certain
+ * that for a given zvol, there is only one operation at a time in progress.
+ * That is why one can be sure that first, zvol_state_t for a given zvol is
+ * allocated and placed on zvol_state_list, and then other minor operations
+ * for this zvol are going to proceed in the order of issue.
+ *
+ * It is also worth keeping in mind that once add_disk() is called, the zvol is
+ * announced to the world, and zvol_open()/zvol_release() can be called at any
+ * time. Incidentally, add_disk() itself calls zvol_open()->zvol_first_open()
+ * and zvol_release()->zvol_last_close() directly as well.
+ */
+
 #include <sys/dbuf.h>
 #include <sys/dmu_traverse.h>
 #include <sys/dsl_dataset.h>
@@ -91,6 +118,7 @@ struct zvol_state {
        list_node_t             zv_next;        /* next zvol_state_t linkage */
        uint64_t                zv_hash;        /* name hash */
        struct hlist_node       zv_hlink;       /* hash link */
+       kmutex_t                zv_state_lock;  /* protects zvol_state_t */
        atomic_t                zv_suspend_ref; /* refcount for suspend */
        krwlock_t               zv_suspend_lock;        /* suspend lock */
 };
@@ -306,8 +334,6 @@ zvol_update_volsize(uint64_t volsize, objset_t *os)
        int error;
        uint64_t txg;
 
-       ASSERT(MUTEX_HELD(&zvol_state_lock));
-
        tx = dmu_tx_create(os);
        dmu_tx_hold_zap(tx, ZVOL_ZAP_OBJ, TRUE, NULL);
        dmu_tx_mark_netfree(tx);
@@ -367,11 +393,15 @@ zvol_set_volsize(const char *name, uint64_t volsize)
 
        mutex_enter(&zvol_state_lock);
        zv = zvol_find_by_name(name);
+       if (zv != NULL)
+               mutex_enter(&zv->zv_state_lock);
+       mutex_exit(&zvol_state_lock);
 
        if (zv == NULL || zv->zv_objset == NULL) {
                if ((error = dmu_objset_own(name, DMU_OST_ZVOL, B_FALSE,
                    FTAG, &os)) != 0) {
-                       mutex_exit(&zvol_state_lock);
+                       if (zv != NULL)
+                               mutex_exit(&zv->zv_state_lock);
                        return (SET_ERROR(error));
                }
                owned = B_TRUE;
@@ -401,8 +431,10 @@ out:
        } else {
                rw_exit(&zv->zv_suspend_lock);
        }
-       mutex_exit(&zvol_state_lock);
-       return (error);
+
+       if (zv != NULL)
+               mutex_exit(&zv->zv_state_lock);
+       return (SET_ERROR(error));
 }
 
 /*
@@ -456,13 +488,15 @@ zvol_set_volblocksize(const char *name, uint64_t volblocksize)
 
        zv = zvol_find_by_name(name);
        if (zv == NULL) {
-               error = SET_ERROR(ENXIO);
-               goto out;
+               mutex_exit(&zvol_state_lock);
+               return (SET_ERROR(ENXIO));
        }
+       mutex_enter(&zv->zv_state_lock);
+       mutex_exit(&zvol_state_lock);
 
        if (zv->zv_flags & ZVOL_RDONLY) {
-               error = SET_ERROR(EROFS);
-               goto out;
+               mutex_exit(&zv->zv_state_lock);
+               return (SET_ERROR(EROFS));
        }
 
        rw_enter(&zv->zv_suspend_lock, RW_READER);
@@ -482,8 +516,8 @@ zvol_set_volblocksize(const char *name, uint64_t volblocksize)
                        zv->zv_volblocksize = volblocksize;
        }
        rw_exit(&zv->zv_suspend_lock);
-out:
-       mutex_exit(&zvol_state_lock);
+
+       mutex_exit(&zv->zv_state_lock);
 
        return (SET_ERROR(error));
 }
@@ -1105,8 +1139,12 @@ zvol_suspend(const char *name)
 
        mutex_enter(&zvol_state_lock);
        zv = zvol_find_by_name(name);
-       if (zv == NULL)
-               goto out;
+       if (zv == NULL) {
+               mutex_exit(&zvol_state_lock);
+               return (NULL);
+       }
+       mutex_enter(&zv->zv_state_lock);
+       mutex_exit(&zvol_state_lock);
 
        /* block all I/O, release in zvol_resume. */
        rw_enter(&zv->zv_suspend_lock, RW_WRITER);
@@ -1115,8 +1153,8 @@ zvol_suspend(const char *name)
 
        if (zv->zv_open_count > 0)
                zvol_shutdown_zv(zv);
-out:
-       mutex_exit(&zvol_state_lock);
+
+       mutex_exit(&zv->zv_state_lock);
        return (zv);
 }
 
@@ -1209,30 +1247,24 @@ static int
 zvol_open(struct block_device *bdev, fmode_t flag)
 {
        zvol_state_t *zv;
-       int error = 0, drop_mutex = 0, drop_suspend = 0;
+       int error = 0, drop_suspend = 0;
 
-       /*
-        * If the caller is already holding the mutex do not take it
-        * again, this will happen as part of zvol_create_minor_impl().
-        * Once add_disk() is called the device is live and the kernel
-        * will attempt to open it to read the partition information.
-        */
-       if (!mutex_owned(&zvol_state_lock)) {
-               mutex_enter(&zvol_state_lock);
-               drop_mutex = 1;
-       }
+       ASSERT(!mutex_owned(&zvol_state_lock));
 
+       mutex_enter(&zvol_state_lock);
        /*
         * Obtain a copy of private_data under the lock to make sure
-        * that either the result of zvol_free() setting
+        * that either the result of zvol free code path setting
         * bdev->bd_disk->private_data to NULL is observed, or zvol_free()
         * is not called on this zv because of the positive zv_open_count.
         */
        zv = bdev->bd_disk->private_data;
        if (zv == NULL) {
-               error = -ENXIO;
-               goto out_mutex;
+               mutex_exit(&zvol_state_lock);
+               return (SET_ERROR(-ENXIO));
        }
+       mutex_enter(&zv->zv_state_lock);
+       mutex_exit(&zvol_state_lock);
 
        if (zv->zv_open_count == 0) {
                /* make sure zvol is not suspended when first open */
@@ -1259,8 +1291,7 @@ out_open_count:
 out_mutex:
        if (drop_suspend)
                rw_exit(&zv->zv_suspend_lock);
-       if (drop_mutex)
-               mutex_exit(&zvol_state_lock);
+       mutex_exit(&zv->zv_state_lock);
 
        return (SET_ERROR(error));
 }
@@ -1272,15 +1303,15 @@ static int
 #endif
 zvol_release(struct gendisk *disk, fmode_t mode)
 {
-       zvol_state_t *zv = disk->private_data;
-       int drop_mutex = 0;
+       zvol_state_t *zv;
 
-       ASSERT(zv && zv->zv_open_count > 0);
+       ASSERT(!mutex_owned(&zvol_state_lock));
 
-       if (!mutex_owned(&zvol_state_lock)) {
-               mutex_enter(&zvol_state_lock);
-               drop_mutex = 1;
-       }
+       mutex_enter(&zvol_state_lock);
+       zv = disk->private_data;
+       ASSERT(zv && zv->zv_open_count > 0);
+       mutex_enter(&zv->zv_state_lock);
+       mutex_exit(&zvol_state_lock);
 
        /* make sure zvol is not suspended when last close */
        if (zv->zv_open_count == 1)
@@ -1292,8 +1323,7 @@ zvol_release(struct gendisk *disk, fmode_t mode)
                rw_exit(&zv->zv_suspend_lock);
        }
 
-       if (drop_mutex)
-               mutex_exit(&zvol_state_lock);
+       mutex_exit(&zv->zv_state_lock);
 
 #ifndef HAVE_BLOCK_DEVICE_OPERATIONS_RELEASE_VOID
        return (0);
@@ -1323,9 +1353,9 @@ zvol_ioctl(struct block_device *bdev, fmode_t mode,
                break;
 
        case BLKZNAME:
-               mutex_enter(&zvol_state_lock);
+               mutex_enter(&zv->zv_state_lock);
                error = copy_to_user((void *)arg, zv->zv_name, MAXNAMELEN);
-               mutex_exit(&zvol_state_lock);
+               mutex_exit(&zv->zv_state_lock);
                break;
 
        default:
@@ -1488,6 +1518,8 @@ zvol_alloc(dev_t dev, const char *name)
 
        list_link_init(&zv->zv_next);
 
+       mutex_init(&zv->zv_state_lock, NULL, MUTEX_DEFAULT, NULL);
+
        zv->zv_queue = blk_alloc_queue(GFP_ATOMIC);
        if (zv->zv_queue == NULL)
                goto out_kmem;
@@ -1532,36 +1564,32 @@ out_kmem:
 }
 
 /*
- * Used for taskq, if used out side zvol_state_lock, you need to clear
- * zv_disk->private_data inside lock first.
+ * Cleanup then free a zvol_state_t which was created by zvol_alloc().
+ * At this time, the structure is not opened by anyone, is taken off
+ * the zvol_state_list, and has its private data set to NULL.
+ * The zvol_state_lock is dropped.
  */
 static void
-zvol_free_impl(void *arg)
+zvol_free(void *arg)
 {
        zvol_state_t *zv = arg;
+
+       ASSERT(!MUTEX_HELD(&zvol_state_lock));
        ASSERT(zv->zv_open_count == 0);
+       ASSERT(zv->zv_disk->private_data == NULL);
 
        rw_destroy(&zv->zv_suspend_lock);
        zfs_rlock_destroy(&zv->zv_range_lock);
 
-       zv->zv_disk->private_data = NULL;
-
        del_gendisk(zv->zv_disk);
        blk_cleanup_queue(zv->zv_queue);
        put_disk(zv->zv_disk);
 
        ida_simple_remove(&zvol_ida, MINOR(zv->zv_dev) >> ZVOL_MINOR_BITS);
-       kmem_free(zv, sizeof (zvol_state_t));
-}
 
-/*
- * Cleanup then free a zvol_state_t which was created by zvol_alloc().
- */
-static void
-zvol_free(zvol_state_t *zv)
-{
-       ASSERT(MUTEX_HELD(&zvol_state_lock));
-       zvol_free_impl(zv);
+       mutex_destroy(&zv->zv_state_lock);
+
+       kmem_free(zv, sizeof (zvol_state_t));
 }
 
 /*
@@ -1591,10 +1619,12 @@ zvol_create_minor_impl(const char *name)
 
        zv = zvol_find_by_name_hash(name, hash);
        if (zv) {
-               error = SET_ERROR(EEXIST);
-               goto out;
+               mutex_exit(&zvol_state_lock);
+               return (SET_ERROR(EEXIST));
        }
 
+       mutex_exit(&zvol_state_lock);
+
        doi = kmem_alloc(sizeof (dmu_object_info_t), KM_SLEEP);
 
        error = dmu_objset_own(name, DMU_OST_ZVOL, B_TRUE, FTAG, &os);
@@ -1666,20 +1696,13 @@ out_dmu_objset_disown:
        dmu_objset_disown(os, FTAG);
 out_doi:
        kmem_free(doi, sizeof (dmu_object_info_t));
-out:
 
        if (error == 0) {
+               mutex_enter(&zvol_state_lock);
                zvol_insert(zv);
-               /*
-                * Drop the lock to prevent deadlock with sys_open() ->
-                * zvol_open(), which first takes bd_disk->bd_mutex and then
-                * takes zvol_state_lock, whereas this code path first takes
-                * zvol_state_lock, and then takes bd_disk->bd_mutex.
-                */
                mutex_exit(&zvol_state_lock);
                add_disk(zv->zv_disk);
        } else {
-               mutex_exit(&zvol_state_lock);
                ida_simple_remove(&zvol_ida, idx);
        }
 
@@ -1927,10 +1950,14 @@ zvol_remove_minors_impl(const char *name)
        zvol_state_t *zv, *zv_next;
        int namelen = ((name) ? strlen(name) : 0);
        taskqid_t t, tid = TASKQID_INVALID;
+       list_t free_list;
 
        if (zvol_inhibit_dev)
                return;
 
+       list_create(&free_list, sizeof (zvol_state_t),
+           offsetof(zvol_state_t, zv_next));
+
        mutex_enter(&zvol_state_lock);
 
        for (zv = list_head(&zvol_state_list); zv != NULL; zv = zv_next) {
@@ -1945,22 +1972,36 @@ zvol_remove_minors_impl(const char *name)
                        if (zv->zv_open_count > 0 ||
                            atomic_read(&zv->zv_suspend_ref))
                                continue;
-
+                       /*
+                        * By taking zv_state_lock here, we guarantee that no
+                        * one is currently using this zv
+                        */
+                       mutex_enter(&zv->zv_state_lock);
                        zvol_remove(zv);
+                       mutex_exit(&zv->zv_state_lock);
 
                        /* clear this so zvol_open won't open it */
                        zv->zv_disk->private_data = NULL;
 
                        /* try parallel zv_free, if failed do it in place */
-                       t = taskq_dispatch(system_taskq, zvol_free_impl, zv,
+                       t = taskq_dispatch(system_taskq, zvol_free, zv,
                            TQ_SLEEP);
                        if (t == TASKQID_INVALID)
-                               zvol_free(zv);
+                               list_insert_head(&free_list, zv);
                        else
                                tid = t;
                }
        }
        mutex_exit(&zvol_state_lock);
+
+       /*
+        * Drop zvol_state_lock before calling zvol_free()
+        */
+       while ((zv = list_head(&free_list)) != NULL) {
+               list_remove(&free_list, zv);
+               zvol_free(zv);
+       }
+
        if (tid != TASKQID_INVALID)
                taskq_wait_outstanding(system_taskq, tid);
 }
@@ -1987,13 +2028,25 @@ zvol_remove_minor_impl(const char *name)
                        if (zv->zv_open_count > 0 ||
                            atomic_read(&zv->zv_suspend_ref))
                                continue;
+                       /*
+                        * By taking zv_state_lock here, we guarantee that no
+                        * one is currently using this zv
+                        */
+                       mutex_enter(&zv->zv_state_lock);
                        zvol_remove(zv);
-                       zvol_free(zv);
+                       mutex_exit(&zv->zv_state_lock);
+                       /* clear this so zvol_open won't open it */
+                       zv->zv_disk->private_data = NULL;
                        break;
                }
        }
 
        mutex_exit(&zvol_state_lock);
+
+       /*
+        * Drop zvol_state_lock before calling zvol_free()
+        */
+       zvol_free(zv);
 }
 
 /*