From 1350922d09e7e2d9abd4c25743cb850fdedf269f Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Tue, 28 Jul 2020 10:43:06 -0400 Subject: [PATCH] bdev/ocf: take additional reference for ocf_cache Fixes #1498 When shutting down the application, it was possible to reference stale ocf_cache pointer. This was the case when two or more vbdev_ocf devices were based on top of single cache bdev. This issue did not occur outside of the shutdown case, since RPC only allows deletion of the vbdev_ocf. This erases on disk metadata and next run of the application, would not detect such vbdev_ocf. Shutdown meanwhile works different, by first stopping the instance of running "ocf_mngt_cache" and later detaching "core" devices (the ones being cached). This prevented erasing the on disk metadata and allowed for restarted application to detect vbdev_ocf. See patch (1292ef2) for details. Since references to ocf_cache are copied between vbdev_ocf [see start_cache()], the reference count inside ocf_cache was limited to original ocf_mngt_cache_start() and management queue creation. First call into ocf_mngt_cache_stop() released all references to ocf_cache. Leaving other vbdev_ocfs pointing to released memory. This patch works around this issue by increasing ref cnt on ocf_cache for each vbdev based on top of it. It allows to call into ocf_mngt_cache_stop(), but not release the memory for ocf_cache until last vbdev. Note: A proper redesign here is in order: - either rearranging structures to be based around single ocf_cache, rather than multiple vbdev_ocf instances - better use of OCF API to reduce book keeping logic in vbdev There are plans to implement detach/attach in RPC, so it should be a focus during that effort. Signed-off-by: Tomasz Zawadzki Change-Id: I560a7fbb1c052bf53970e655bdb60803c561a252 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/3574 Tested-by: SPDK CI Jenkins Reviewed-by: Vitaliy Mysak Reviewed-by: Jim Harris Reviewed-by: Ben Walker --- module/bdev/ocf/vbdev_ocf.c | 3 +++ test/ocf/ocf.sh | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/module/bdev/ocf/vbdev_ocf.c b/module/bdev/ocf/vbdev_ocf.c index 3c2bb4abc..4997772cd 100644 --- a/module/bdev/ocf/vbdev_ocf.c +++ b/module/bdev/ocf/vbdev_ocf.c @@ -199,6 +199,7 @@ static void unregister_finish(struct vbdev_ocf *vbdev) { spdk_bdev_destruct_done(&vbdev->exp_bdev, vbdev->state.stop_status); + ocf_mngt_cache_put(vbdev->ocf_cache); vbdev_ocf_cache_ctx_put(vbdev->cache_ctx); vbdev_ocf_mngt_continue(vbdev, 0); } @@ -1059,6 +1060,7 @@ start_cache(struct vbdev_ocf *vbdev) SPDK_NOTICELOG("OCF bdev %s connects to existing cache device %s\n", vbdev->name, vbdev->cache.name); vbdev->ocf_cache = existing; + ocf_mngt_cache_get(vbdev->ocf_cache); vbdev->cache_ctx = ocf_cache_get_priv(existing); vbdev_ocf_cache_ctx_get(vbdev->cache_ctx); vbdev_ocf_mngt_continue(vbdev, 0); @@ -1079,6 +1081,7 @@ start_cache(struct vbdev_ocf *vbdev) vbdev_ocf_mngt_exit(vbdev, unregister_path_dirty, rc); return; } + ocf_mngt_cache_get(vbdev->ocf_cache); ocf_cache_set_priv(vbdev->ocf_cache, vbdev->cache_ctx); diff --git a/test/ocf/ocf.sh b/test/ocf/ocf.sh index ff626fd5f..415befc67 100755 --- a/test/ocf/ocf.sh +++ b/test/ocf/ocf.sh @@ -10,6 +10,5 @@ run_test "ocf_bdevperf_iotypes" "$testdir/integrity/bdevperf-iotypes.sh" run_test "ocf_stats" "$testdir/integrity/stats.sh" run_test "ocf_create_destruct" "$testdir/management/create-destruct.sh" run_test "ocf_multicore" "$testdir/management/multicore.sh" -# Disabled due to issue #1498 -# run_test "ocf_persistent_metadata" "$testdir/management/persistent-metadata.sh" +run_test "ocf_persistent_metadata" "$testdir/management/persistent-metadata.sh" run_test "ocf_remove" "$testdir/management/remove.sh"