lvol: add destroy_lvol_bdev RPC method

This is the first step toward fixing the behavior of the lvol bdev
module so that it handles spdk_bdev_unregister() correctly.

Currently, when an lvol bdev is unregistered, this causes the associated
lvol to be deleted; this isn't the desired behavior, since
spdk_bdev_unregister() is just meant to drop the currently-exposed bdev,
as in a hot-unplug event, not destroy the backing storage.

The current implementation of the new destroy_lvol_bdev method still
calls into the exsting (broken) spdk_bdev_unregister() path, but this at
least lets us fix the existing delete_bdev RPC callers that really
intended to destroy the lvol.

The next step is to make vbdev_lvol_destroy() call spdk_lvol_destroy()
instead of unregistering the bdev, which will require cleanup in the
generic lvol layer to remove the close_only flag.

Change-Id: Ic4cb51305d31f6d7366a82ba7bf7b19ebbcd3e9c
Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/408084
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

CHANGELOG.md

@@ -62,6 +62,10 @@ spdk_blob_resize() is now an asynchronous operation to enable resizing a blob wh
 are in progress to that blob on other threads.  An explicit spdk_blob_sync_md() is still
 required to sync the updated metadata to disk.
 
+### Logical Volumes
+
+A new `destroy_lvol_bdev` RPC method to delete logical volumes has been added.
+
 ### Lib
 
 A set of changes were made in the SPDK's lib code altering,

doc/lvol.md

@@ -58,7 +58,7 @@ destroy_lvol_store [-h] [-u UUID] [-l LVS_NAME]
     Destroy lvolstore on specified bdev. Removes lvolstore along with lvols on
     it. User can identify lvol store by UUID or its name. Note that destroying
     lvolstore requires using this call, while deleting single lvol requires
-    using delete_bdev rpc call.
+    using destroy_lvol_bdev rpc call.
     optional arguments:
     -h, --help  show help
 get_lvol_stores [-h] [-u UUID] [-l LVS_NAME]
@@ -88,8 +88,8 @@ get_bdevs [-h] [-b NAME]
     optional arguments:
     -h, --help  show help
     -b NAME, --name NAME  Name of the block device. Example: Nvme0n1
-delete_bdev [-h] bdev_name
+destroy_lvol_bdev [-h] bdev_name
-    Deletes spdk bdev
+    Deletes a logical volume previously created by construct_lvol_bdev.
     optional arguments:
     -h, --help  show help
 snapshot_lvol_bdev [-h] lvol_name snapshot_name

lib/bdev/lvol/vbdev_lvol.c

@@ -562,6 +562,16 @@ vbdev_lvol_destruct(void *ctx)
 	return 1;
 }
 
+void
+vbdev_lvol_destroy(struct spdk_lvol *lvol, spdk_lvol_op_complete cb_fn, void *cb_arg)
+{
+	/*
+	 * TODO: This should call spdk_lvol_destroy() directly, and the bdev unregister path
+	 * should be changed so that it does not destroy the lvol.
+	 */
+	spdk_bdev_unregister(lvol->bdev, cb_fn, cb_arg);
+}
+
 static char *
 vbdev_lvol_find_name(struct spdk_lvol *lvol, spdk_blob_id blob_id)
 {

lib/bdev/lvol/vbdev_lvol.h

@@ -75,6 +75,14 @@ void vbdev_lvol_resize(struct spdk_lvol *lvol, uint64_t sz, spdk_lvol_op_complet
 void vbdev_lvol_rename(struct spdk_lvol *lvol, const char *new_lvol_name,
 		       spdk_lvol_op_complete cb_fn, void *cb_arg);
 
+/**
+ * Destroy a logical volume
+ * \param lvol Handle to lvol
+ * \param cb_fn Completion callback
+ * \param cb_arg Completion callback custom arguments
+ */
+void vbdev_lvol_destroy(struct spdk_lvol *lvol, spdk_lvol_op_complete cb_fn, void *cb_arg);
+
 /**
  * \brief Renames given lvolstore.
  *

lib/bdev/lvol/vbdev_lvol_rpc.c

@@ -768,6 +768,87 @@ invalid:
 
 SPDK_RPC_REGISTER("resize_lvol_bdev", spdk_rpc_resize_lvol_bdev)
 
+struct rpc_destroy_lvol_bdev {
+	char *name;
+};
+
+static void
+free_rpc_destroy_lvol_bdev(struct rpc_destroy_lvol_bdev *req)
+{
+	free(req->name);
+}
+
+static const struct spdk_json_object_decoder rpc_destroy_lvol_bdev_decoders[] = {
+	{"name", offsetof(struct rpc_destroy_lvol_bdev, name), spdk_json_decode_string},
+};
+
+static void
+_spdk_rpc_destroy_lvol_bdev_cb(void *cb_arg, int lvolerrno)
+{
+	struct spdk_json_write_ctx *w;
+	struct spdk_jsonrpc_request *request = cb_arg;
+
+	if (lvolerrno != 0) {
+		goto invalid;
+	}
+
+	w = spdk_jsonrpc_begin_result(request);
+	if (w == NULL) {
+		return;
+	}
+
+	spdk_json_write_bool(w, true);
+	spdk_jsonrpc_end_result(request, w);
+	return;
+
+invalid:
+	spdk_jsonrpc_send_error_response(request, SPDK_JSONRPC_ERROR_INTERNAL_ERROR,
+					 spdk_strerror(-lvolerrno));
+}
+
+static void
+spdk_rpc_destroy_lvol_bdev(struct spdk_jsonrpc_request *request,
+			   const struct spdk_json_val *params)
+{
+	struct rpc_destroy_lvol_bdev req = {};
+	struct spdk_bdev *bdev;
+	struct spdk_lvol *lvol;
+	int rc;
+
+	if (spdk_json_decode_object(params, rpc_destroy_lvol_bdev_decoders,
+				    SPDK_COUNTOF(rpc_destroy_lvol_bdev_decoders),
+				    &req)) {
+		SPDK_INFOLOG(SPDK_LOG_LVOL_RPC, "spdk_json_decode_object failed\n");
+		rc = -EINVAL;
+		goto invalid;
+	}
+
+	bdev = spdk_bdev_get_by_name(req.name);
+	if (bdev == NULL) {
+		SPDK_ERRLOG("no bdev for provided name %s\n", req.name);
+		rc = -ENODEV;
+		goto invalid;
+	}
+
+	lvol = vbdev_lvol_get_from_bdev(bdev);
+	if (lvol == NULL) {
+		rc = -ENODEV;
+		goto invalid;
+	}
+
+	vbdev_lvol_destroy(lvol, _spdk_rpc_destroy_lvol_bdev_cb, request);
+
+	free_rpc_destroy_lvol_bdev(&req);
+	return;
+
+invalid:
+	spdk_jsonrpc_send_error_response(request, SPDK_JSONRPC_ERROR_INVALID_PARAMS,
+					 spdk_strerror(-rc));
+	free_rpc_destroy_lvol_bdev(&req);
+}
+
+SPDK_RPC_REGISTER("destroy_lvol_bdev", spdk_rpc_destroy_lvol_bdev)
+
 struct rpc_get_lvol_stores {
 	char *uuid;
 	char *lvs_name;

scripts/rpc.py

@@ -582,6 +582,15 @@ if __name__ == "__main__":
     p.add_argument('size', help='new size in MiB for this bdev', type=int)
     p.set_defaults(func=resize_lvol_bdev)
 
+    @call_cmd
+    def destroy_lvol_bdev(args):
+        rpc.lvol.destroy_lvol_bdev(args.client,
+                                   name=args.name)
+
+    p = subparsers.add_parser('destroy_lvol_bdev', help='Destroy a logical volume')
+    p.add_argument('name', help='lvol bdev name')
+    p.set_defaults(func=destroy_lvol_bdev)
+
     @call_cmd
     def destroy_lvol_store(args):
         rpc.lvol.destroy_lvol_store(args.client,

scripts/rpc/lvol.py

@@ -110,6 +110,18 @@ def resize_lvol_bdev(client, name, size):
     return client.call('resize_lvol_bdev', params)
 
 
+def destroy_lvol_bdev(client, name):
+    """Destroy a logical volume.
+
+    Args:
+        name: name of logical volume to destroy
+    """
+    params = {
+        'name': name,
+    }
+    return client.call('destroy_lvol_bdev', params)
+
+
 def destroy_lvol_store(client, uuid=None, lvs_name=None):
     """Destroy a logical volume store.
 

test/lvol/rpc_commands_lib.py

@@ -147,6 +147,11 @@ class Commands_Rpc(object):
         output, rc = self.rpc.delete_bdev(base_name)
         return rc
 
+    def destroy_lvol_bdev(self, bdev_name):
+        print("INFO: RPC COMMAND destroy_lvol_bdev")
+        output, rc = self.rpc.destroy_lvol_bdev(bdev_name)
+        return rc
+
     def resize_lvol_bdev(self, uuid, new_size):
         print("INFO: RPC COMMAND resize_lvol_bdev")
         output, rc = self.rpc.resize_lvol_bdev(uuid, new_size)

test/lvol/test_cases.py

@@ -295,7 +295,7 @@ class TestCases(object):
                                                self.total_size - 1)
         fail_count += self.c.check_get_bdevs_methods(uuid_bdev,
                                                      self.total_size - 1)
-        self.c.delete_bdev(uuid_bdev)
+        self.c.destroy_lvol_bdev(uuid_bdev)
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
         return fail_count
@@ -321,7 +321,7 @@ class TestCases(object):
                 fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size)
 
             for uuid_bdev in uuid_bdevs:
-                self.c.delete_bdev(uuid_bdev)
+                self.c.destroy_lvol_bdev(uuid_bdev)
 
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
@@ -345,7 +345,7 @@ class TestCases(object):
         fail_count += self.c.check_get_bdevs_methods(uuid_bdev,
                                                      self.total_size - 1)
 
-        fail_count += self.c.delete_bdev(uuid_bdev)
+        fail_count += self.c.destroy_lvol_bdev(uuid_bdev)
         fail_count += self.c.destroy_lvol_store(uuid_store)
         fail_count += self.c.delete_bdev(base_name)
         return fail_count
@@ -377,8 +377,8 @@ class TestCases(object):
         fail_count += self.c.check_get_bdevs_methods(uuid_bdev_1, self.total_size - 1)
         fail_count += self.c.check_get_bdevs_methods(uuid_bdev_2, self.total_size - 1)
 
-        fail_count += self.c.delete_bdev(uuid_bdev_1)
+        fail_count += self.c.destroy_lvol_bdev(uuid_bdev_1)
-        fail_count += self.c.delete_bdev(uuid_bdev_2)
+        fail_count += self.c.destroy_lvol_bdev(uuid_bdev_2)
         fail_count += self.c.destroy_lvol_store(uuid_store_1)
         fail_count += self.c.destroy_lvol_store(uuid_store_2)
         fail_count += self.c.delete_bdev(base_name_1)
@@ -413,7 +413,7 @@ class TestCases(object):
                                       self.total_size - 1) == 0:
             fail_count += 1
 
-        self.c.delete_bdev(uuid_bdev)
+        self.c.destroy_lvol_bdev(uuid_bdev)
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
         return fail_count
@@ -438,7 +438,7 @@ class TestCases(object):
                                       size) == 0:
             fail_count += 1
 
-        self.c.delete_bdev(uuid_bdev)
+        self.c.destroy_lvol_bdev(uuid_bdev)
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
         return fail_count
@@ -472,7 +472,7 @@ class TestCases(object):
         self.c.resize_lvol_bdev(uuid_bdev, size)
         fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size)
 
-        self.c.delete_bdev(uuid_bdev)
+        self.c.destroy_lvol_bdev(uuid_bdev)
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
         return fail_count
@@ -501,7 +501,7 @@ class TestCases(object):
         if self.c.resize_lvol_bdev(uuid_bdev, self.total_size + 1) == 0:
             fail_count += 1
 
-        self.c.delete_bdev(uuid_bdev)
+        self.c.destroy_lvol_bdev(uuid_bdev)
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)
         return fail_count
@@ -839,7 +839,7 @@ class TestCases(object):
 
         fail_count += self.c.stop_nbd_disk(nbd_name)
         # destroy thin provisioned lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         lvs = self.c.get_lvol_stores(self.lvs_name)[0]
         free_clusters_end = int(lvs[u'free_clusters'])
         # check that saved number of free clusters equals to current free clusters
@@ -901,8 +901,8 @@ class TestCases(object):
         fail_count += self.c.stop_nbd_disk(nbd_name0)
         fail_count += self.c.stop_nbd_disk(nbd_name1)
         # destroy thin provisioned lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev0['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev0['name'])
-        fail_count += self.c.delete_bdev(lvol_bdev1['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev1['name'])
         # destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # destroy malloc bdev
@@ -944,7 +944,7 @@ class TestCases(object):
 
         fail_count += self.c.stop_nbd_disk(nbd_name)
         # destroy thin provisioned lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # destroy malloc bdev
@@ -1047,8 +1047,8 @@ class TestCases(object):
         fail_count += self.c.stop_nbd_disk(nbd_name0)
         fail_count += self.c.stop_nbd_disk(nbd_name1)
         # destroy thin provisioned lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev0['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev0['name'])
-        fail_count += self.c.delete_bdev(lvol_bdev1['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev1['name'])
         # destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # destroy malloc bdev
@@ -1105,8 +1105,8 @@ class TestCases(object):
         fail_count += self.c.stop_nbd_disk(nbd_name0)
         fail_count += self.c.stop_nbd_disk(nbd_name1)
         # destroy thin provisioned lvol bdevs
-        fail_count += self.c.delete_bdev(lvol_bdev0['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev0['name'])
-        fail_count += self.c.delete_bdev(lvol_bdev1['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev1['name'])
         # destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # destroy malloc bdev
@@ -1194,7 +1194,7 @@ class TestCases(object):
             fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size)
 
         for uuid_bdev in uuid_bdevs:
-            self.c.delete_bdev(uuid_bdev)
+            self.c.destroy_lvol_bdev(uuid_bdev)
 
         if self.c.destroy_lvol_store(uuid_store) != 0:
             fail_count += 1
@@ -1282,9 +1282,9 @@ class TestCases(object):
 
         fail_count += self.c.stop_nbd_disk(nbd_name0)
         # Destroy lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # Destroy snapshot
-        fail_count += self.c.delete_bdev(snapshot_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(snapshot_bdev['name'])
         # Destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Delete malloc bdev
@@ -1357,11 +1357,11 @@ class TestCases(object):
         for nbd in nbd_name:
             fail_count += self.c.stop_nbd_disk(nbd)
         # Delete lvol bdevs
-        fail_count += self.c.delete_bdev(lvol_bdev0['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev0['name'])
-        fail_count += self.c.delete_bdev(lvol_bdev1['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev1['name'])
         # Delete snapshots
-        fail_count += self.c.delete_bdev(self.lvs_name + "/" + snapshot_name0)
+        fail_count += self.c.destroy_lvol_bdev(self.lvs_name + "/" + snapshot_name0)
-        fail_count += self.c.delete_bdev(self.lvs_name + "/" + snapshot_name1)
+        fail_count += self.c.destroy_lvol_bdev(self.lvs_name + "/" + snapshot_name1)
         # Destroy snapshot
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Delete malloc bdev
@@ -1419,9 +1419,9 @@ class TestCases(object):
         fail_count += thread.rv
         fail_count += self.c.stop_nbd_disk(nbd_name)
         # Destroy lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # Delete snapshot
-        fail_count += self.c.delete_bdev(self.lvs_name + "/" + snapshot_name)
+        fail_count += self.c.destroy_lvol_bdev(self.lvs_name + "/" + snapshot_name)
         # Destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Delete malloc bdevs
@@ -1466,9 +1466,9 @@ class TestCases(object):
             print("ERROR: Creating snapshot of snapshot should fail")
             fail_count += 1
         # Delete lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # Destroy snapshot
-        fail_count += self.c.delete_bdev(self.lvs_name + "/" + snapshot_name0)
+        fail_count += self.c.destroy_lvol_bdev(self.lvs_name + "/" + snapshot_name0)
         # Destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Delete malloc bdev
@@ -1527,11 +1527,11 @@ class TestCases(object):
         clone_bdev = self.c.get_lvol_bdev_with_name(self.lvs_name + "/" + clone_name)
 
         # Delete lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # Destroy clone
-        fail_count += self.c.delete_bdev(clone_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(clone_bdev['name'])
         # Delete snapshot
-        fail_count += self.c.delete_bdev(self.lvs_name + "/" + snapshot_name)
+        fail_count += self.c.destroy_lvol_bdev(self.lvs_name + "/" + snapshot_name)
         # Delete lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Destroy malloc bdev
@@ -1600,12 +1600,12 @@ class TestCases(object):
         for nbd in nbd_name:
             fail_count += self.c.stop_nbd_disk(nbd)
         # Destroy lvol bdev
-        fail_count += self.c.delete_bdev(lvol_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_bdev['name'])
         # Destroy two clones
-        fail_count += self.c.delete_bdev(lvol_clone0['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_clone0['name'])
-        fail_count += self.c.delete_bdev(lvol_clone1['name'])
+        fail_count += self.c.destroy_lvol_bdev(lvol_clone1['name'])
         # Delete snapshot
-        fail_count += self.c.delete_bdev(snapshot_bdev['name'])
+        fail_count += self.c.destroy_lvol_bdev(snapshot_bdev['name'])
         # Destroy lvol store
         fail_count += self.c.destroy_lvol_store(uuid_store)
         # Delete malloc
@@ -1682,7 +1682,7 @@ class TestCases(object):
 
         # Delete configuration using names after rename operation
         for bdev in new_bdev_aliases:
-            fail_count += self.c.delete_bdev(bdev)
+            fail_count += self.c.destroy_lvol_bdev(bdev)
         fail_count += self.c.destroy_lvol_store(new_lvs_name)
         fail_count += self.c.delete_bdev(base_name)
 
@@ -1776,7 +1776,7 @@ class TestCases(object):
 
         # Clean configuration
         for lvol_uuid in bdev_uuids_1 + bdev_uuids_2:
-            fail_count += self.c.delete_bdev(lvol_uuid)
+            fail_count += self.c.destroy_lvol_bdev(lvol_uuid)
         fail_count += self.c.destroy_lvol_store(lvs_uuid_1)
         fail_count += self.c.destroy_lvol_store(lvs_uuid_2)
         fail_count += self.c.delete_bdev(base_bdev_1)
@@ -1822,8 +1822,8 @@ class TestCases(object):
                                                      bdev_size,
                                                      "/".join([self.lvs_name, self.lbd_name + "1"]))
 
-        fail_count += self.c.delete_bdev(bdev_uuid_1)
+        fail_count += self.c.destroy_lvol_bdev(bdev_uuid_1)
-        fail_count += self.c.delete_bdev(bdev_uuid_2)
+        fail_count += self.c.destroy_lvol_bdev(bdev_uuid_2)
         fail_count += self.c.destroy_lvol_store(lvs_uuid)
         fail_count += self.c.delete_bdev(base_bdev)
 

test/nvmf/host/fio.sh

@@ -66,9 +66,9 @@ if [ $RUN_NIGHTLY -eq 1 ]; then
 
 	sync
 	# Delete lvol_bdev and destroy lvol_store.
-	$rpc_py delete_bdev "$lb_nested_guid"
+	$rpc_py destroy_lvol_bdev "$lb_nested_guid"
 	$rpc_py destroy_lvol_store -l lvs_n_0
-	$rpc_py delete_bdev "$lb_guid"
+	$rpc_py destroy_lvol_bdev "$lb_guid"
 	$rpc_py destroy_lvol_store -l lvs_0
 	$rpc_py delete_bdev "Nvme0n1"
 fi

test/vhost/integrity/integrity_start.sh

@@ -25,7 +25,7 @@ function usage()
 function clean_lvol_cfg()
 {
 	notice "Removing lvol bdev and lvol store"
-	$rpc_py delete_bdev lvol_store/lvol_bdev
+	$rpc_py destroy_lvol_bdev lvol_store/lvol_bdev
 	$rpc_py destroy_lvol_store -l lvol_store
 }
 

test/vhost/lvol/lvol_test.sh

@@ -54,7 +54,7 @@ function clean_lvol_cfg()
 {
     notice "Removing nested lvol bdevs"
     for lvol_bdev in "${nest_lvol_bdevs[@]}"; do
-        $rpc_py delete_bdev $lvol_bdev
+        $rpc_py destroy_lvol_bdev $lvol_bdev
         notice "nested lvol bdev $lvol_bdev removed"
     done
 
@@ -66,7 +66,7 @@ function clean_lvol_cfg()
 
     notice "Removing lvol bdevs"
     for lvol_bdev in "${lvol_bdevs[@]}"; do
-        $rpc_py delete_bdev $lvol_bdev
+        $rpc_py destroy_lvol_bdev $lvol_bdev
         notice "lvol bdev $lvol_bdev removed"
     done