From 888ac74e3264abf93d48fd1277dd0863dcae76e6 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 08:27:45 +0200 Subject: [PATCH 1/6] Removed redundand include Signed-off-by: Michal Rakowski --- src/metadata/metadata_superblock.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/metadata/metadata_superblock.h b/src/metadata/metadata_superblock.h index c08e90d..5caff24 100644 --- a/src/metadata/metadata_superblock.h +++ b/src/metadata/metadata_superblock.h @@ -6,7 +6,6 @@ #ifndef __METADATA_SUPERBLOCK_H__ #define __METADATA_SUPERBLOCK_H__ -#include #include #define CACHE_MAGIC_NUMBER 0x187E1CA6 From f1cfc800e2b5472011e87a0866036c9b136455b2 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 08:34:14 +0200 Subject: [PATCH 2/6] Add check for part_id in ocf_stats_collect_part_* Signed-off-by: Michal Rakowski --- src/ocf_stats_builder.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ocf_stats_builder.c b/src/ocf_stats_builder.c index bdc2a6f..dcee54b 100644 --- a/src/ocf_stats_builder.c +++ b/src/ocf_stats_builder.c @@ -266,6 +266,9 @@ int ocf_stats_collect_part_core(ocf_core_t core, ocf_part_id_t part_id, OCF_CHECK_NULL(core); + if (part_id > OCF_IO_CLASS_ID_MAX) + return -OCF_ERR_INVAL; + cache = ocf_core_get_cache(core); _ocf_stats_zero(usage); @@ -291,6 +294,9 @@ int ocf_stats_collect_part_cache(ocf_cache_t cache, ocf_part_id_t part_id, OCF_CHECK_NULL(cache); + if (part_id > OCF_IO_CLASS_ID_MAX) + return -OCF_ERR_INVAL; + _ocf_stats_zero(usage); _ocf_stats_zero(req); _ocf_stats_zero(blocks); From 9504cb044df104ed9c0cf9ce525de31211f64ef7 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 11:47:29 +0200 Subject: [PATCH 3/6] discard: Added missing io_put in case of error Signed-off-by: Michal Rakowski --- src/engine/engine_discard.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/engine_discard.c b/src/engine/engine_discard.c index 62807dd..e8cca8c 100644 --- a/src/engine/engine_discard.c +++ b/src/engine/engine_discard.c @@ -77,7 +77,7 @@ static int _ocf_discard_core(struct ocf_request *req) ocf_io_set_cmpl(io, req, NULL, _ocf_discard_core_complete); err = ocf_io_set_data(io, req->data, 0); if (err) { - _ocf_discard_complete_req(req, err); + _ocf_discard_core_complete(io, err); return err; } From 8426d662cbf89e4ed290f26e9967a6a240184d41 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 11:49:54 +0200 Subject: [PATCH 4/6] Changed err handling to BUG_ON in case of refcnt_int fail durign cache init. Since we do not expect that incrementing cache's reference counter during cache init will fail at any condition it is can be changed to an assert instead of error handling. Signed-off-by: Michal Rakowski --- src/mngt/ocf_mngt_cache.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/mngt/ocf_mngt_cache.c b/src/mngt/ocf_mngt_cache.c index 06965f4..57f39de 100644 --- a/src/mngt/ocf_mngt_cache.c +++ b/src/mngt/ocf_mngt_cache.c @@ -568,10 +568,7 @@ static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) goto lock_err; } - if (!ocf_refcnt_inc(&cache->refcnt.cache)){ - result = -OCF_ERR_START_CACHE_FAIL; - goto flush_mutex_err; - } + ENV_BUG_ON(!ocf_refcnt_inc(&cache->refcnt.cache)); /* start with freezed metadata ref counter to indicate detached device*/ ocf_refcnt_freeze(&cache->refcnt.metadata); @@ -586,8 +583,6 @@ static int _ocf_mngt_init_new_cache(struct ocf_cache_mngt_init_params *params) return 0; -flush_mutex_err: - env_mutex_destroy(&cache->flush_mutex); lock_err: ocf_mngt_cache_lock_deinit(cache); alloc_err: From b78557a2cc5cc12b38251786fe41920f55de5ea5 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 14:57:55 +0200 Subject: [PATCH 5/6] Change env_spinlock_init to non-void function Signed-off-by: Michal Rakowski --- env/posix/ocf_env.h | 4 ++-- src/cleaning/alru.c | 16 +++++++++++++-- src/concurrency/ocf_cache_line_concurrency.c | 8 ++++++-- src/concurrency/ocf_metadata_concurrency.c | 21 +++++++++++++++++--- src/concurrency/ocf_metadata_concurrency.h | 2 +- src/metadata/metadata.c | 9 ++++++++- src/ocf_freelist.c | 21 +++++++++++++------- src/ocf_queue.c | 8 +++++++- src/promotion/nhit/nhit_hash.c | 8 +++++--- src/utils/utils_async_lock.c | 7 ++++++- 10 files changed, 81 insertions(+), 23 deletions(-) diff --git a/env/posix/ocf_env.h b/env/posix/ocf_env.h index 7e81924..16c2682 100644 --- a/env/posix/ocf_env.h +++ b/env/posix/ocf_env.h @@ -450,9 +450,9 @@ typedef struct { pthread_spinlock_t lock; } env_spinlock; -static inline void env_spinlock_init(env_spinlock *l) +static inline int env_spinlock_init(env_spinlock *l) { - ENV_BUG_ON(pthread_spin_init(&l->lock, 0)); + return pthread_spin_init(&l->lock, 0); } static inline int env_spinlock_trylock(env_spinlock *l) diff --git a/src/cleaning/alru.c b/src/cleaning/alru.c index 3d6cb63..91208bc 100644 --- a/src/cleaning/alru.c +++ b/src/cleaning/alru.c @@ -467,6 +467,7 @@ int cleaning_policy_alru_initialize(ocf_cache_t cache, int init_metadata) struct ocf_user_part *part; ocf_part_id_t part_id; struct alru_context *alru; + int error = 0; unsigned i; alru = env_vzalloc(sizeof(*alru)); @@ -475,8 +476,19 @@ int cleaning_policy_alru_initialize(ocf_cache_t cache, int init_metadata) return -OCF_ERR_NO_MEM; } - for (i = 0; i < OCF_IO_CLASS_MAX; i++) - env_spinlock_init(&alru->list_lock[i]); + for (i = 0; i < OCF_IO_CLASS_MAX; i++) { + error = env_spinlock_init(&alru->list_lock[i]); + if (error) + break; + } + + if (error) { + while (i--) + env_spinlock_destroy(&alru->list_lock[i]); + env_vfree(alru); + return error; + } + cache->cleaner.cleaning_policy_context = alru; diff --git a/src/concurrency/ocf_cache_line_concurrency.c b/src/concurrency/ocf_cache_line_concurrency.c index c0cb54c..3ed0b54 100644 --- a/src/concurrency/ocf_cache_line_concurrency.c +++ b/src/concurrency/ocf_cache_line_concurrency.c @@ -110,13 +110,17 @@ int ocf_cache_line_concurrency_init(struct ocf_cache *cache) /* Init concurrency control table */ for (i = 0; i < _WAITERS_LIST_ENTRIES; i++) { INIT_LIST_HEAD(&c->waiters_lsts[i].head); - env_spinlock_init(&c->waiters_lsts[i].lock); + error = env_spinlock_init(&c->waiters_lsts[i].lock); + if (error) + goto spinlock_err; } return 0; +spinlock_err: + while (i--) + env_spinlock_destroy(&c->waiters_lsts[i].lock); ocf_cache_line_concurrency_init: - ocf_cache_log(cache, log_err, "Cannot initialize cache concurrency, " "ERROR %d", error); diff --git a/src/concurrency/ocf_metadata_concurrency.c b/src/concurrency/ocf_metadata_concurrency.c index df996b4..0863957 100644 --- a/src/concurrency/ocf_metadata_concurrency.c +++ b/src/concurrency/ocf_metadata_concurrency.c @@ -6,17 +6,32 @@ #include "ocf_metadata_concurrency.h" #include "../metadata/metadata_misc.h" -void ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock) +int ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock) { + int err = 0; unsigned i; - env_spinlock_init(&metadata_lock->eviction); + err = env_spinlock_init(&metadata_lock->eviction); + if (err) + return err; + + //@TODO handle env_rwlock_init return val env_rwlock_init(&metadata_lock->status); env_rwsem_init(&metadata_lock->global); for (i = 0; i < OCF_IO_CLASS_MAX; i++) { - env_spinlock_init(&metadata_lock->partition[i]); + err = env_spinlock_init(&metadata_lock->partition[i]); + if (err) + break; } + + if (err) { + while (i--) + env_spinlock_destroy(&metadata_lock->partition[i]); + return err; + } + + return err; } void ocf_metadata_concurrency_deinit(struct ocf_metadata_lock *metadata_lock) diff --git a/src/concurrency/ocf_metadata_concurrency.h b/src/concurrency/ocf_metadata_concurrency.h index e7d3a23..224d8a7 100644 --- a/src/concurrency/ocf_metadata_concurrency.h +++ b/src/concurrency/ocf_metadata_concurrency.h @@ -10,7 +10,7 @@ #define OCF_METADATA_RD 0 #define OCF_METADATA_WR 1 -void ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock); +int ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock); void ocf_metadata_concurrency_deinit(struct ocf_metadata_lock *metadata_lock); diff --git a/src/metadata/metadata.c b/src/metadata/metadata.c index e38de62..b8f7c36 100644 --- a/src/metadata/metadata.c +++ b/src/metadata/metadata.c @@ -39,7 +39,14 @@ int ocf_metadata_init(struct ocf_cache *cache, return ret; } - ocf_metadata_concurrency_init(&cache->metadata.lock); + ret = ocf_metadata_concurrency_init(&cache->metadata.lock); + if (ret) { + if (cache->metadata.iface.deinit) + cache->metadata.iface.deinit(cache); + + ocf_metadata_io_deinit(cache); + return ret; + } return 0; } diff --git a/src/ocf_freelist.c b/src/ocf_freelist.c index fdff6bb..bf2c60f 100644 --- a/src/ocf_freelist.c +++ b/src/ocf_freelist.c @@ -385,21 +385,28 @@ ocf_freelist_t ocf_freelist_init(struct ocf_cache *cache) freelist->lock = env_vzalloc(sizeof(freelist->lock[0]) * num); freelist->part = env_vzalloc(sizeof(freelist->part[0]) * num); - if (!freelist->lock || !freelist->part) { - env_vfree(freelist->lock); - env_vfree(freelist->part); - env_vfree(freelist); - return NULL; - } + if (!freelist->lock || !freelist->part) + goto free_allocs; for (i = 0; i < num; i++) { - env_spinlock_init(&freelist->lock[i]); + if (env_spinlock_init(&freelist->lock[i])) + goto spinlock_err; + freelist->part[i].head = line_entries; freelist->part[i].tail = line_entries; env_atomic64_set(&freelist->part[i].curr_size, 0); } return freelist; + +spinlock_err: + while (i--) + env_spinlock_destroy(&freelist->lock[i]); +free_allocs: + env_vfree(freelist->lock); + env_vfree(freelist->part); + env_vfree(freelist); + return NULL; } void ocf_freelist_deinit(ocf_freelist_t freelist) diff --git a/src/ocf_queue.c b/src/ocf_queue.c index 7657eee..5c8b631 100644 --- a/src/ocf_queue.c +++ b/src/ocf_queue.c @@ -32,7 +32,13 @@ int ocf_queue_create(ocf_cache_t cache, ocf_queue_t *queue, } env_atomic_set(&tmp_queue->io_no, 0); - env_spinlock_init(&tmp_queue->io_list_lock); + result = env_spinlock_init(&tmp_queue->io_list_lock); + if (result) { + ocf_mngt_cache_put(cache); + free(tmp_queue); + return result; + } + INIT_LIST_HEAD(&tmp_queue->io_list); env_atomic_set(&tmp_queue->ref_count, 1); tmp_queue->cache = cache; diff --git a/src/promotion/nhit/nhit_hash.c b/src/promotion/nhit/nhit_hash.c index cb07c75..f5ab85d 100644 --- a/src/promotion/nhit/nhit_hash.c +++ b/src/promotion/nhit/nhit_hash.c @@ -146,7 +146,6 @@ ocf_error_t nhit_hash_init(uint64_t hash_size, nhit_hash_t *ctx) for (i_locks = 0; i_locks < new_ctx->hash_entries; i_locks++) { if (env_rwsem_init(&new_ctx->hash_locks[i_locks])) { result = -OCF_ERR_UNKNOWN; - i_locks--; goto dealloc_locks; } } @@ -163,14 +162,17 @@ ocf_error_t nhit_hash_init(uint64_t hash_size, nhit_hash_t *ctx) env_atomic_set(&new_ctx->ring_buffer[i].counter, 0); } - env_spinlock_init(&new_ctx->rb_pointer_lock); + result = env_spinlock_init(&new_ctx->rb_pointer_lock); + if (result) + goto dealloc_locks; + new_ctx->rb_pointer = 0; *ctx = new_ctx; return 0; dealloc_locks: - for (; i_locks >= 0; i_locks--) + while (i_locks--) ENV_BUG_ON(env_rwsem_destroy(&new_ctx->hash_locks[i_locks])); env_vfree(new_ctx->hash_locks); dealloc_hash: diff --git a/src/utils/utils_async_lock.c b/src/utils/utils_async_lock.c index d01cc2b..d9679a2 100644 --- a/src/utils/utils_async_lock.c +++ b/src/utils/utils_async_lock.c @@ -47,7 +47,12 @@ void _ocf_async_lock_run_waiters(struct ocf_async_lock *lock, int ocf_async_lock_init(struct ocf_async_lock *lock, uint32_t waiter_priv_size) { - env_spinlock_init(&lock->waiters_lock); + int err = 0; + + err = env_spinlock_init(&lock->waiters_lock); + if (err); + return err; + INIT_LIST_HEAD(&lock->waiters); lock->rd = 0; lock->wr = 0; From 2575be83fa8de203e8bbad692c7a3487ff7b3e20 Mon Sep 17 00:00:00 2001 From: Michal Rakowski Date: Fri, 27 Sep 2019 15:21:53 +0200 Subject: [PATCH 6/6] Error handling for env_rwsem_init added Signed-off-by: Michal Rakowski --- src/cleaning/acp.c | 9 +++++++-- src/concurrency/ocf_metadata_concurrency.c | 19 +++++++++++-------- src/utils/utils_async_lock.c | 2 +- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/cleaning/acp.c b/src/cleaning/acp.c index a294bd0..85d7be9 100644 --- a/src/cleaning/acp.c +++ b/src/cleaning/acp.c @@ -302,11 +302,16 @@ int cleaning_policy_acp_initialize(struct ocf_cache *cache, ocf_cache_log(cache, log_err, "acp context allocation error\n"); return -OCF_ERR_NO_MEM; } + + err = env_rwsem_init(&acp->chunks_lock); + if (err) { + env_vfree(acp); + return err; + } + cache->cleaner.cleaning_policy_context = acp; acp->cache = cache; - env_rwsem_init(&acp->chunks_lock); - for (i = 0; i < ACP_MAX_BUCKETS; i++) { INIT_LIST_HEAD(&acp->bucket_info[i].chunk_list); acp->bucket_info[i].threshold = diff --git a/src/concurrency/ocf_metadata_concurrency.c b/src/concurrency/ocf_metadata_concurrency.c index 0863957..6502983 100644 --- a/src/concurrency/ocf_metadata_concurrency.c +++ b/src/concurrency/ocf_metadata_concurrency.c @@ -15,22 +15,25 @@ int ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock) if (err) return err; - //@TODO handle env_rwlock_init return val env_rwlock_init(&metadata_lock->status); - env_rwsem_init(&metadata_lock->global); + err = env_rwsem_init(&metadata_lock->global); + if (err) + goto rwsem_err; for (i = 0; i < OCF_IO_CLASS_MAX; i++) { err = env_spinlock_init(&metadata_lock->partition[i]); if (err) - break; + goto spinlocks_err; } - if (err) { - while (i--) - env_spinlock_destroy(&metadata_lock->partition[i]); - return err; - } + return err; +spinlocks_err: + while (i--) + env_spinlock_destroy(&metadata_lock->partition[i]); +rwsem_err: + env_rwlock_destroy(&metadata_lock->status); + env_spinlock_destroy(&metadata_lock->eviction); return err; } diff --git a/src/utils/utils_async_lock.c b/src/utils/utils_async_lock.c index d9679a2..2321c28 100644 --- a/src/utils/utils_async_lock.c +++ b/src/utils/utils_async_lock.c @@ -50,7 +50,7 @@ int ocf_async_lock_init(struct ocf_async_lock *lock, uint32_t waiter_priv_size) int err = 0; err = env_spinlock_init(&lock->waiters_lock); - if (err); + if (err) return err; INIT_LIST_HEAD(&lock->waiters);