bdev: remove vbdevs during spdk_bdev_unregister()

spdk_vbdev_unregister() is part of internal bdev API,
yet bdev module that uses spdk_vbdev_register() directly
will not be removed correctly when using delete_bdev RPC.
spdk_vbdev_unregister() is now consolidated with
spdk_bdev_unregister().

This comes up when deleting lvol bdev, as it does not use
spdk_bdev_part_* functions.
base_bdev->vbdevs entry was not removed for bdev that lvs
is created on.

Additionally patch expands test to create lvol bdev,
after removing it using delete_bdev RPC.
With ASAN enabled this would report accessing
already freed memory previously.

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

include/spdk_internal/bdev.h

@@ -398,7 +398,6 @@ void spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn,
 void spdk_bdev_unregister_done(struct spdk_bdev *bdev, int bdeverrno);
 int spdk_vbdev_register(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs,
 			int base_bdev_count);
-void spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg);
 
 void spdk_bdev_module_examine_done(struct spdk_bdev_module_if *module);
 void spdk_bdev_module_init_done(struct spdk_bdev_module_if *module);

lib/bdev/bdev.c

@@ -2110,11 +2110,18 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void
 	struct spdk_bdev_desc	*desc, *tmp;
 	int			rc;
 	bool			do_destruct = true;
+	struct spdk_bdev	*base_bdev;
 
 	SPDK_DEBUGLOG(SPDK_LOG_BDEV, "Removing bdev %s from list\n", bdev->name);
 
 	pthread_mutex_lock(&bdev->mutex);
 
+	if (!TAILQ_EMPTY(&bdev->base_bdevs)) {
+		TAILQ_FOREACH(base_bdev, &bdev->base_bdevs, base_bdev_link) {
+			TAILQ_REMOVE(&base_bdev->vbdevs, bdev, vbdev_link);
+		}
+	}
+
 	bdev->status = SPDK_BDEV_STATUS_REMOVING;
 	bdev->unregister_cb = cb_fn;
 	bdev->unregister_ctx = cb_arg;
@@ -2149,18 +2156,6 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void
 	}
 }
 
-void
-spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg)
-{
-	struct spdk_bdev *base_bdev;
-
-	assert(!TAILQ_EMPTY(&vbdev->base_bdevs));
-	TAILQ_FOREACH(base_bdev, &vbdev->base_bdevs, base_bdev_link) {
-		TAILQ_REMOVE(&base_bdev->vbdevs, vbdev, vbdev_link);
-	}
-	spdk_bdev_unregister(vbdev, cb_fn, cb_arg);
-}
-
 int
 spdk_bdev_open(struct spdk_bdev *bdev, bool write, spdk_bdev_remove_cb_t remove_cb,
 	       void *remove_ctx, struct spdk_bdev_desc **_desc)
@@ -2332,7 +2327,7 @@ spdk_bdev_part_base_hotremove(struct spdk_bdev *base_bdev, struct bdev_part_tail
 
 	TAILQ_FOREACH_SAFE(part, tailq, tailq, tmp) {
 		if (part->base->bdev == base_bdev) {
-			spdk_vbdev_unregister(&part->bdev, NULL, NULL);
+			spdk_bdev_unregister(&part->bdev, NULL, NULL);
 		}
 	}
 }

lib/bdev/lvol/vbdev_lvol.c

@@ -275,7 +275,7 @@ _vbdev_lvs_remove(struct spdk_lvol_store *lvs, spdk_lvs_op_complete cb_fn, void
 		lvs->destruct = destroy;
 		TAILQ_FOREACH_SAFE(lvol, &lvs->lvols, link, tmp) {
 			lvol->close_only = !destroy;
-			spdk_vbdev_unregister(lvol->bdev, NULL, NULL);
+			spdk_bdev_unregister(lvol->bdev, NULL, NULL);
 		}
 	}
 }

test/lvol/test_cases.py

@@ -201,16 +201,17 @@ class TestCases(object):
                                                   self.cluster_size)
         size = ((self.total_size - 1) / 4)
 
-        uuid_bdevs = []
-        for i in range(4):
-            uuid_bdev = self.c.construct_lvol_bdev(uuid_store,
-                                                   self.lbd_name + str(i),
-                                                   size)
-            uuid_bdevs.append(uuid_bdev)
-            fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size)
+        for j in range(2):
+            uuid_bdevs = []
+            for i in range(4):
+                uuid_bdev = self.c.construct_lvol_bdev(uuid_store,
+                                                       self.lbd_name + str(i),
+                                                       size)
+                uuid_bdevs.append(uuid_bdev)
+                fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size)
 
-        for uuid_bdev in uuid_bdevs:
-            self.c.delete_bdev(uuid_bdev)
+            for uuid_bdev in uuid_bdevs:
+                self.c.delete_bdev(uuid_bdev)
 
         self.c.destroy_lvol_store(uuid_store)
         self.c.delete_bdev(base_name)

test/lvol/test_plan.md

@@ -69,6 +69,7 @@ Steps:
   because of lvol metadata)
 - repeat the previous step three more times
 - delete lvol bdevs
+- create and delete four lvol bdevs again from steps above
 - destroy lvol store
 - delete malloc bdev
 

test/unit/lib/bdev/bdev.c/bdev_ut.c

@@ -138,7 +138,8 @@ free_bdev(struct spdk_bdev *bdev)
 static void
 free_vbdev(struct spdk_bdev *bdev)
 {
-	spdk_vbdev_unregister(bdev, NULL, NULL);
+	CU_ASSERT(!TAILQ_EMPTY(&bdev->base_bdevs));
+	spdk_bdev_unregister(bdev, NULL, NULL);
 	free(bdev);
 }
 

test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c

@@ -110,7 +110,7 @@ spdk_bs_bdev_claim(struct spdk_bs_dev *bs_dev, struct spdk_bdev_module_if *modul
 }
 
 void
-spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg)
+spdk_bdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg)
 {
 	SPDK_CU_ASSERT_FATAL(vbdev != NULL);
 	vbdev->fn_table->destruct(vbdev->ctxt);