From fae30462b19e444da390f4528c64ce849033eb1a Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Fri, 4 Oct 2024 15:41:23 +0200 Subject: [PATCH 01/10] Decrement cache.refcnt if locking cache failed Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/mngt/ocf_mngt_common.c b/src/mngt/ocf_mngt_common.c index 85dc1990..611aee2b 100644 --- a/src/mngt/ocf_mngt_common.c +++ b/src/mngt/ocf_mngt_common.c @@ -270,8 +270,10 @@ static int _ocf_mngt_cache_trylock(ocf_cache_t cache, return -OCF_ERR_CACHE_NOT_EXIST; result = trylock_fn(&cache->lock); - if (result) + if (result) { + ocf_mngt_cache_put(cache); return result; + } if (env_bit_test(ocf_cache_state_stopping, &cache->cache_state)) { /* Cache already stopping, do not allow any operation */ From 0bb2621c50d6181e3d1609e18f441f517ff2a3f1 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Mon, 7 Oct 2024 16:07:15 +0200 Subject: [PATCH 02/10] Increment ctx.refcnt before creating a new cache Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index 41cf6986..b8798c90 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -1493,6 +1493,8 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, params.metadata.promotion_policy = cfg->promotion_policy; params.locked = cfg->locked; + ocf_ctx_get(ctx); + result = env_rmutex_lock_interruptible(&ctx->lock); if (result) goto _cache_mngt_init_instance_ERROR; @@ -1533,8 +1535,6 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, _ocf_mngt_cache_init(tmp_cache, ¶ms); - ocf_ctx_get(ctx); - if (!params.locked) { /* User did not request to lock cache instance after creation - unlock it here since we have acquired the lock to @@ -1550,6 +1550,7 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, _cache_mngt_init_instance_ERROR: _ocf_mngt_init_handle_error(ctx, ¶ms); *cache = NULL; + ocf_ctx_put(ctx); return result; } From 40a850cd9c6e415f8940eb5bd0fb686447b75939 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Thu, 28 Sep 2023 09:17:51 +0200 Subject: [PATCH 03/10] Remove dead code Signed-off-by: Michal Mielewczyk --- src/metadata/metadata.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/metadata/metadata.c b/src/metadata/metadata.c index c890eed8..3257b956 100644 --- a/src/metadata/metadata.c +++ b/src/metadata/metadata.c @@ -463,7 +463,6 @@ static inline void ocf_metadata_config_init(ocf_cache_t cache, size_t size) static void ocf_metadata_deinit_fixed_size(struct ocf_cache *cache) { - int result = 0; uint32_t i; struct ocf_metadata_ctrl *ctrl = (struct ocf_metadata_ctrl *) @@ -481,9 +480,6 @@ static void ocf_metadata_deinit_fixed_size(struct ocf_cache *cache) env_vfree(ctrl); cache->metadata.priv = NULL; - - if (result) - ENV_BUG(); } static struct ocf_metadata_ctrl *ocf_metadata_ctrl_init( From 6f02a625ad7291ce7424f85b7249bf4f48794e9e Mon Sep 17 00:00:00 2001 From: Robert Baldyga Date: Tue, 7 Nov 2023 12:29:39 +0100 Subject: [PATCH 04/10] pipeline: Introduce debug logging Signed-off-by: Robert Baldyga Signed-off-by: Michal Mielewczyk --- src/utils/utils_pipeline.c | 1 + src/utils/utils_pipeline.h | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/utils/utils_pipeline.c b/src/utils/utils_pipeline.c index 8744af7b..fe300f65 100644 --- a/src/utils/utils_pipeline.c +++ b/src/utils/utils_pipeline.c @@ -37,6 +37,7 @@ static int _ocf_pipeline_run_step(struct ocf_request *req) while (true) { step = &pipeline->properties->steps[pipeline->next_step]; + ocf_cache_log(req->cache, log_debug, "PL STEP: %s\n", step->name); switch (step->type) { case ocf_pipeline_step_single: pipeline->next_step++; diff --git a/src/utils/utils_pipeline.h b/src/utils/utils_pipeline.h index bbf41c98..997c812a 100644 --- a/src/utils/utils_pipeline.h +++ b/src/utils/utils_pipeline.h @@ -1,5 +1,6 @@ /* * Copyright(c) 2019-2022 Intel Corporation + * Copyright(c) 2024 Huawei Technologies * SPDX-License-Identifier: BSD-3-Clause */ @@ -70,6 +71,7 @@ typedef bool (*ocf_pipeline_cond_step_predicate_t)(ocf_pipeline_t pipeline, void *priv, ocf_pipeline_arg_t arg); struct ocf_pipeline_step { + char *name; enum ocf_pipeline_step_type type; ocf_pipeline_step_hndl_t hndl; ocf_pipeline_cond_step_predicate_t pred; @@ -79,14 +81,19 @@ struct ocf_pipeline_step { }; }; +#define xstr(a) str(a) +#define str(a) #a + #define OCF_PL_STEP(_hndl) \ { \ + .name = xstr(_hndl), \ .type = ocf_pipeline_step_single, \ .hndl = _hndl, \ } #define OCF_PL_STEP_ARG_INT(_hndl, _int) \ { \ + .name = xstr(_hndl), \ .type = ocf_pipeline_step_single, \ .hndl = _hndl, \ .arg = { \ @@ -97,6 +104,7 @@ struct ocf_pipeline_step { #define OCF_PL_STEP_ARG_PTR(_hndl, _ptr) \ { \ + .name = xstr(_hndl), \ .type = ocf_pipeline_step_single, \ .hndl = _hndl, \ .arg = { \ @@ -107,6 +115,7 @@ struct ocf_pipeline_step { #define OCF_PL_STEP_FOREACH(_hndl, _args) \ { \ + .name = xstr(_hndl), \ .type = ocf_pipeline_step_foreach, \ .hndl = _hndl, \ .args = _args, \ @@ -114,11 +123,13 @@ struct ocf_pipeline_step { #define OCF_PL_STEP_TERMINATOR() \ { \ + .name = "", \ .type = ocf_pipeline_step_terminator, \ } #define OCF_PL_STEP_COND(_pred, _hndl) \ { \ + .name = xstr(_hndl), \ .pred = _pred, \ .type = ocf_pipeline_step_conditional, \ .hndl = _hndl, \ @@ -126,6 +137,7 @@ struct ocf_pipeline_step { #define OCF_PL_STEP_COND_ARG_INT(_pred, _hndl, _int) \ { \ + .name = xstr(_hndl), \ .pred = _pred, \ .type = ocf_pipeline_step_conditional, \ .hndl = _hndl, \ @@ -137,6 +149,7 @@ struct ocf_pipeline_step { #define OCF_PL_STEP_COND_ARG_PTR(_pred, _hndl, _ptr) \ { \ + .name = xstr(_hndl), \ .pred = _pred, \ .type = ocf_pipeline_step_conditional, \ .hndl = _hndl, \ From 1d1561649c01621c4b89feb595992752492d01fa Mon Sep 17 00:00:00 2001 From: Roel Apfelbaum Date: Thu, 30 Nov 2023 03:12:38 -0500 Subject: [PATCH 05/10] Remove redundant fallback-PT counter accesses The fewer (atomic variable accesses on IO path) the better fare Signed-off-by: Roel Apfelbaum Signed-off-by: Michal Mielewczyk --- src/engine/cache_engine.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/engine/cache_engine.c b/src/engine/cache_engine.c index c291214f..f7f160e0 100644 --- a/src/engine/cache_engine.c +++ b/src/engine/cache_engine.c @@ -139,12 +139,17 @@ static ocf_req_cb ocf_cache_mode_to_engine_cb( bool ocf_fallback_pt_is_on(ocf_cache_t cache) { - ENV_BUG_ON(env_atomic_read(&cache->fallback_pt_error_counter) < 0); + int threshold = cache->fallback_pt_error_threshold; + int counter; - return (cache->fallback_pt_error_threshold != - OCF_CACHE_FALLBACK_PT_INACTIVE && - env_atomic_read(&cache->fallback_pt_error_counter) >= - cache->fallback_pt_error_threshold); + if (threshold == OCF_CACHE_FALLBACK_PT_INACTIVE) + return false; + + counter = env_atomic_read(&cache->fallback_pt_error_counter); + + ENV_BUG_ON(counter < 0); + + return counter >= threshold; } void ocf_resolve_effective_cache_mode(ocf_cache_t cache, From f6bdd354d0c25efaa06d50de2bf16bb48a5bff02 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Fri, 4 Oct 2024 14:05:25 +0200 Subject: [PATCH 06/10] Don't bug on cache init Even if locking the new cache should never fail it's not an unrecoverable state. Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_cache.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index b8798c90..c3ba7bb6 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -840,7 +840,9 @@ static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) } /* Lock cache during setup - this trylock should always succeed */ - ENV_BUG_ON(ocf_mngt_cache_trylock(cache)); + result = ocf_mngt_cache_trylock(cache); + if (result) + goto lock_init_err; if (env_mutex_init(&cache->flush_mutex)) { result = -OCF_ERR_NO_MEM; @@ -852,7 +854,9 @@ static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) if (result) goto mutex_err; - ENV_BUG_ON(!ocf_refcnt_inc(&cache->refcnt.cache)); + result = !ocf_refcnt_inc(&cache->refcnt.cache); + if (result) + goto cache_refcnt_inc_err; /* start with freezed metadata ref counter to indicate detached device*/ ocf_refcnt_freeze(&cache->refcnt.metadata); @@ -869,10 +873,13 @@ static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) return 0; +cache_refcnt_inc_err: + env_spinlock_destroy(&cache->io_queues_lock); mutex_err: env_mutex_destroy(&cache->flush_mutex); lock_err: ocf_mngt_cache_unlock(cache); +lock_init_err: ocf_mngt_cache_lock_deinit(cache); alloc_err: env_vfree(cache); From e8e7a1600c4da88d25bfa769995fb3d56045fb45 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Fri, 4 Oct 2024 14:19:15 +0200 Subject: [PATCH 07/10] Log errors on cache init The more (logging) the merrier Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_cache.c | 40 +++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index c3ba7bb6..408639be 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -826,37 +826,59 @@ static void _ocf_mngt_load_metadata(ocf_pipeline_t pipeline, * @brief allocate memory for new cache, add it to cache queue, set initial * values and running state */ -static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) +static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params, + char *new_cache_name) { ocf_cache_t cache = env_vzalloc(sizeof(*cache)); int result; - if (!cache) + if (!cache) { + ocf_log(params->ctx, log_err, "Failed to allocate cache %s\n", + new_cache_name); return -OCF_ERR_NO_MEM; + } if (ocf_mngt_cache_lock_init(cache)) { + ocf_log(params->ctx, log_err, + "Failed to allocate cache %s lock\n", + new_cache_name); result = -OCF_ERR_NO_MEM; goto alloc_err; } /* Lock cache during setup - this trylock should always succeed */ result = ocf_mngt_cache_trylock(cache); - if (result) + if (result) { + ocf_log(params->ctx, log_crit, + "Failed to lock the newly created cache %s\n", + new_cache_name); goto lock_init_err; + } if (env_mutex_init(&cache->flush_mutex)) { + ocf_log(params->ctx, log_err, + "Failed to allocate cache %s flush lock\n", + new_cache_name); result = -OCF_ERR_NO_MEM; goto lock_err; } INIT_LIST_HEAD(&cache->io_queues); result = env_spinlock_init(&cache->io_queues_lock); - if (result) + if (result) { + ocf_log(params->ctx, log_err, + "Failed to allocate cache %s queue lock\n", + new_cache_name); goto mutex_err; + } result = !ocf_refcnt_inc(&cache->refcnt.cache); - if (result) + if (result) { + ocf_log(params->ctx, log_crit, + "Failed to increment %s refcnt\n", + new_cache_name); goto cache_refcnt_inc_err; + } /* start with freezed metadata ref counter to indicate detached device*/ ocf_refcnt_freeze(&cache->refcnt.metadata); @@ -957,7 +979,7 @@ static int _ocf_mngt_init_prepare_cache(struct ocf_cache_mngt_init_params *param ocf_log(param->ctx, log_info, "Inserting cache %s\n", cfg->name); - ret = _ocf_mngt_init_new_cache(param); + ret = _ocf_mngt_init_new_cache(param, cfg->name); if (ret) goto out; @@ -1510,6 +1532,8 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, result = _ocf_mngt_init_prepare_cache(¶ms, cfg); if (result) { env_rmutex_unlock(&ctx->lock); + ocf_log(ctx, log_err, "Failed to prepare cache %s\n", + cfg->name); goto _cache_mngt_init_instance_ERROR; } @@ -1523,6 +1547,8 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, result = ocf_metadata_init(tmp_cache, params.metadata.line_size); if (result) { env_rmutex_unlock(&ctx->lock); + ocf_log(ctx, log_err, "Failed to initialize cache %s " + "metadata\n", cfg->name); result = -OCF_ERR_NO_MEM; goto _cache_mngt_init_instance_ERROR; } @@ -1530,6 +1556,8 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, result = ocf_cache_set_name(tmp_cache, cfg->name, OCF_CACHE_NAME_SIZE); if (result) { + ocf_log(ctx, log_err, "Failed to set cache %s name\n", + cfg->name); env_rmutex_unlock(&ctx->lock); goto _cache_mngt_init_instance_ERROR; } From d22885ef7d3f0a978e1656e155a6578f9a25c526 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Mon, 7 Oct 2024 12:41:45 +0200 Subject: [PATCH 08/10] example: Introduce error handling Signed-off-by: Michal Mielewczyk --- example/simple/src/main.c | 65 ++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/example/simple/src/main.c b/example/simple/src/main.c index 7758da63..eae97458 100644 --- a/example/simple/src/main.c +++ b/example/simple/src/main.c @@ -22,15 +22,6 @@ struct cache_priv { ocf_queue_t io_queue; }; -/* - * Helper function for error handling. - */ -void error(char *msg) -{ - printf("ERROR: %s", msg); - exit(1); -} - /* * Queue ops providing interface for running queue thread in asynchronous * way. Optional synchronous kick callback is not provided. The stop() @@ -318,8 +309,10 @@ void perform_workload(ocf_core_t core) /* Allocate data buffer and fill it with example data */ data1 = ctx_data_alloc(1); - if (!data1) - error("Unable to allocate data1\n"); + if (!data1) { + printf("Error: Unable to allocate data1\n"); + return; + } strcpy(data1->ptr, "This is some test data"); /* Prepare and submit write IO to the core */ submit_io(core, data1, 0, 512, OCF_WRITE, complete_write); @@ -332,8 +325,10 @@ void perform_workload(ocf_core_t core) /* Allocate data buffer for read */ data2 = ctx_data_alloc(1); - if (!data2) - error("Unable to allocate data2\n"); + if (!data2) { + printf("Error: Unable to allocate data2\n"); + return; + } /* Prepare and submit read IO to the core */ submit_io(core, data2, 0, 512, OCF_READ, complete_read); /* After read completes, complete_read() callback will be called, @@ -360,21 +355,29 @@ int main(int argc, char *argv[]) /* Initialize completion semaphore */ ret = sem_init(&context.sem, 0, 0); - if (ret) - error("Unable to initialize completion semaphore\n"); + if (ret) { + printf("Error: Unable to initialize completion semaphore\n"); + goto sem_err; + } context.error = &ret; /* Initialize OCF context */ - if (ctx_init(&ctx)) - error("Unable to initialize context\n"); + if (ctx_init(&ctx)) { + printf("Error: Unable to initialize context\n"); + goto ctx_err; + } /* Start cache */ - if (initialize_cache(ctx, &cache1)) - error("Unable to start cache\n"); + if (initialize_cache(ctx, &cache1)) { + printf("Error: Unable to start cache\n"); + goto cache_err; + } /* Add core */ - if (initialize_core(cache1, &core1)) - error("Unable to add core\n"); + if (initialize_core(cache1, &core1)) { + printf("Error: Unable to add core\n"); + goto core_err; + } /* Do some actual io operations */ perform_workload(core1); @@ -382,27 +385,31 @@ int main(int argc, char *argv[]) /* Remove core from cache */ ocf_mngt_cache_remove_core(core1, remove_core_complete, &context); sem_wait(&context.sem); - if (ret) - error("Unable to remove core\n"); + if (ret) { + printf("Error: Unable to remove core\n"); + goto core_err; + } /* Stop cache */ ocf_mngt_cache_stop(cache1, simple_complete, &context); sem_wait(&context.sem); - if (ret) - error("Unable to stop cache\n"); + if (ret) { + printf("Error: Unable to stop cache\n"); + } +core_err: cache_priv = ocf_cache_get_priv(cache1); /* Put the management queue */ ocf_queue_put(cache_priv->mngt_queue); free(cache_priv); - +cache_err: /* Deinitialize context */ ctx_cleanup(ctx); - +ctx_err: /* Destroy completion semaphore */ sem_destroy(&context.sem); - - return 0; +sem_err: + return ret; } From c82fd173c65c613a30ad06e30206e5a0f42708a5 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Mon, 7 Oct 2024 12:51:19 +0200 Subject: [PATCH 09/10] Remove redundant list_del(ctx->caches) during init New caches are added to the list at the point where they are already initialized and no errors are possible at this point, hence list_del() in error handling is redundant. Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_cache.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index 408639be..67466bcf 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -66,9 +66,6 @@ struct ocf_cache_mngt_init_params { bool metadata_inited : 1; /*!< Metadata is inited to valid state */ - bool added_to_list : 1; - /*!< Cache is added to context list */ - bool cache_locked : 1; /*!< Cache has been locked */ } flags; @@ -1476,15 +1473,7 @@ static void _ocf_mngt_init_handle_error(ocf_ctx_t ctx, if (params->flags.metadata_inited) ocf_metadata_deinit(cache); - if (!params->flags.added_to_list) - return; - - env_rmutex_lock(&ctx->lock); - - list_del(&cache->list); env_vfree(cache); - - env_rmutex_unlock(&ctx->lock); } static void _ocf_mngt_cache_init(ocf_cache_t cache, @@ -1563,7 +1552,6 @@ static int _ocf_mngt_cache_start(ocf_ctx_t ctx, ocf_cache_t *cache, } list_add_tail(&tmp_cache->list, &ctx->caches); - params.flags.added_to_list = true; env_rmutex_unlock(&ctx->lock); ocf_cache_log(tmp_cache, log_debug, "Metadata initialized\n"); From 1b2a9e03c32afc82099e87550db22ee2c7347686 Mon Sep 17 00:00:00 2001 From: Michal Mielewczyk Date: Mon, 7 Oct 2024 10:10:43 +0200 Subject: [PATCH 10/10] Add missing cache unlock in init rollback Signed-off-by: Michal Mielewczyk --- src/mngt/ocf_mngt_cache.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index 67466bcf..d1c8ad76 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -1468,6 +1468,9 @@ static void _ocf_mngt_init_handle_error(ocf_ctx_t ctx, env_mutex_destroy(&cache->flush_mutex); + if (params->flags.cache_locked) + ocf_mngt_cache_unlock(cache); + ocf_mngt_cache_lock_deinit(cache); if (params->flags.metadata_inited)