From cd9e42f987e3f230847fd9df908a8b0566012d16 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 15 Feb 2021 19:42:42 -0600 Subject: [PATCH 1/2] Properly lock hash bucket for status bits operations Signed-off-by: Adam Rutkowski --- src/engine/engine_rd.c | 4 ++-- src/engine/engine_wb.c | 22 ++++++++++++---------- src/engine/engine_wo.c | 14 ++++++++++++++ src/engine/engine_wt.c | 33 +++++++++++++++------------------ 4 files changed, 43 insertions(+), 30 deletions(-) diff --git a/src/engine/engine_rd.c b/src/engine/engine_rd.c index 3c8160e..b18369a 100644 --- a/src/engine/engine_rd.c +++ b/src/engine/engine_rd.c @@ -164,12 +164,12 @@ static int _ocf_read_generic_do(struct ocf_request *req) return 0; } - ocf_hb_req_prot_lock_rd(req); + ocf_hb_req_prot_lock_wr(req); /* Set valid status bits map */ ocf_set_valid_map_info(req); - ocf_hb_req_prot_unlock_rd(req); + ocf_hb_req_prot_unlock_wr(req); } if (ocf_engine_needs_repart(req)) { diff --git a/src/engine/engine_wb.c b/src/engine/engine_wb.c index 8c9c296..95bd8ee 100644 --- a/src/engine/engine_wb.c +++ b/src/engine/engine_wb.c @@ -27,22 +27,24 @@ static const struct ocf_io_if _io_if_wb_resume = { static void _ocf_write_wb_update_bits(struct ocf_request *req) { - if (ocf_engine_is_miss(req)) { - ocf_hb_req_prot_lock_rd(req); + bool miss = ocf_engine_is_miss(req); + bool clean_any = !ocf_engine_is_dirty_all(req); + + if (!miss && !clean_any) { + ocf_req_set_cleaning_hot(req); + return; + } + + ocf_hb_req_prot_lock_wr(req); + if (miss) { /* Update valid status bits */ ocf_set_valid_map_info(req); - - ocf_hb_req_prot_unlock_rd(req); } - - if (!ocf_engine_is_dirty_all(req)) { - ocf_hb_req_prot_lock_wr(req); - + if (clean_any) { /* set dirty bits, and mark if metadata flushing is required */ ocf_set_dirty_map_info(req); - - ocf_hb_req_prot_unlock_wr(req); } + ocf_hb_req_prot_unlock_wr(req); ocf_req_set_cleaning_hot(req); } diff --git a/src/engine/engine_wo.c b/src/engine/engine_wo.c index 777df45..0b4a4bc 100644 --- a/src/engine/engine_wo.c +++ b/src/engine/engine_wo.c @@ -71,6 +71,10 @@ static int ocf_read_wo_cache_do(struct ocf_request *req) s = ocf_map_line_start_sector(req, line); e = ocf_map_line_end_sector(req, line); + ocf_hb_cline_prot_lock_rd(&cache->metadata.lock, + req->lock_idx, entry->core_id, + entry->core_line); + /* if cacheline mapping is not sequential, send cache IO to * previous cacheline(s) */ phys_prev = phys_curr; @@ -112,6 +116,10 @@ static int ocf_read_wo_cache_do(struct ocf_request *req) == valid); } + ocf_hb_cline_prot_unlock_rd(&cache->metadata.lock, + req->lock_idx, entry->core_id, + entry->core_line); + if (io && !valid) { /* end of sequential valid region */ ocf_read_wo_cache_io(req, io_start, @@ -126,6 +134,12 @@ static int ocf_read_wo_cache_do(struct ocf_request *req) } offset += increment; + + if (i <= e) { + ocf_hb_cline_prot_lock_rd(&cache->metadata.lock, + req->lock_idx, entry->core_id, + entry->core_line); + } } while (i <= e); } diff --git a/src/engine/engine_wt.c b/src/engine/engine_wt.c index 83b0176..d786a0f 100644 --- a/src/engine/engine_wt.c +++ b/src/engine/engine_wt.c @@ -97,39 +97,36 @@ static inline void _ocf_write_wt_submit(struct ocf_request *req) static void _ocf_write_wt_update_bits(struct ocf_request *req) { - if (ocf_engine_is_miss(req)) { - ocf_hb_req_prot_lock_rd(req); + bool miss = ocf_engine_is_miss(req); + bool dirty_any = req->info.dirty_any; + bool repart = ocf_engine_needs_repart(req); + if (!miss && !dirty_any && !repart) + return; + + ocf_hb_req_prot_lock_wr(req); + + if (miss) { /* Update valid status bits */ ocf_set_valid_map_info(req); - - ocf_hb_req_prot_unlock_rd(req); } - if (req->info.dirty_any) { - ocf_hb_req_prot_lock_wr(req); - - /* Writes goes to SDD and HDD, need to update status bits from - * dirty to clean + if (dirty_any) { + /* Writes goes to both cache and core, need to update + * status bits from dirty to clean */ - ocf_set_clean_map_info(req); - - ocf_hb_req_prot_unlock_wr(req); } - if (ocf_engine_needs_repart(req)) { + if (repart) { OCF_DEBUG_RQ(req, "Re-Part"); - - ocf_hb_req_prot_lock_wr(req); - /* Probably some cache lines are assigned into wrong * partition. Need to move it to new one */ ocf_part_move(req); - - ocf_hb_req_prot_unlock_wr(req); } + + ocf_hb_req_prot_unlock_wr(req); } static int _ocf_write_wt_do(struct ocf_request *req) From c95f6358abc4e062a35da960594781f3a2e59ba3 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 15 Feb 2021 19:23:54 -0600 Subject: [PATCH 2/2] Get rid of status bits lock All the status bits operations are now protectec by hash bucket locks Signed-off-by: Adam Rutkowski --- src/concurrency/ocf_metadata_concurrency.c | 6 - src/concurrency/ocf_metadata_concurrency.h | 38 ------ src/metadata/metadata_status.h | 144 ++------------------- src/metadata/metadata_structs.h | 1 - 4 files changed, 13 insertions(+), 176 deletions(-) diff --git a/src/concurrency/ocf_metadata_concurrency.c b/src/concurrency/ocf_metadata_concurrency.c index 4ebdad3..4a42806 100644 --- a/src/concurrency/ocf_metadata_concurrency.c +++ b/src/concurrency/ocf_metadata_concurrency.c @@ -17,8 +17,6 @@ int ocf_metadata_concurrency_init(struct ocf_metadata_lock *metadata_lock) for (evp_iter = 0; evp_iter < OCF_NUM_EVICTION_LISTS; evp_iter++) env_rwlock_init(&metadata_lock->eviction[evp_iter]); - env_rwlock_init(&metadata_lock->status); - for (global_iter = 0; global_iter < OCF_NUM_GLOBAL_META_LOCKS; global_iter++) { err = env_rwsem_init(&metadata_lock->global[global_iter].sem); @@ -42,8 +40,6 @@ global_err: while (global_iter--) env_rwsem_destroy(&metadata_lock->global[global_iter].sem); - env_rwlock_destroy(&metadata_lock->status); - while (evp_iter--) env_rwlock_destroy(&metadata_lock->eviction[evp_iter]); @@ -62,8 +58,6 @@ void ocf_metadata_concurrency_deinit(struct ocf_metadata_lock *metadata_lock) for (i = 0; i < OCF_NUM_GLOBAL_META_LOCKS; i++) env_rwsem_destroy(&metadata_lock->global[i].sem); - - env_rwlock_destroy(&metadata_lock->status); } int ocf_metadata_concurrency_attached_init( diff --git a/src/concurrency/ocf_metadata_concurrency.h b/src/concurrency/ocf_metadata_concurrency.h index e8fed21..d258b3e 100644 --- a/src/concurrency/ocf_metadata_concurrency.h +++ b/src/concurrency/ocf_metadata_concurrency.h @@ -127,44 +127,6 @@ void ocf_metadata_end_shared_access( struct ocf_metadata_lock *metadata_lock, unsigned lock_idx); -static inline void ocf_metadata_status_bits_lock( - struct ocf_metadata_lock *metadata_lock, int rw) -{ - if (rw == OCF_METADATA_WR) - env_rwlock_write_lock(&metadata_lock->status); - else if (rw == OCF_METADATA_RD) - env_rwlock_read_lock(&metadata_lock->status); - else - ENV_BUG(); -} - -static inline void ocf_metadata_status_bits_unlock( - struct ocf_metadata_lock *metadata_lock, int rw) -{ - if (rw == OCF_METADATA_WR) - env_rwlock_write_unlock(&metadata_lock->status); - else if (rw == OCF_METADATA_RD) - env_rwlock_read_unlock(&metadata_lock->status); - else - ENV_BUG(); -} - -#define OCF_METADATA_BITS_LOCK_RD() \ - ocf_metadata_status_bits_lock(&cache->metadata.lock, \ - OCF_METADATA_RD) - -#define OCF_METADATA_BITS_UNLOCK_RD() \ - ocf_metadata_status_bits_unlock(&cache->metadata.lock, \ - OCF_METADATA_RD) - -#define OCF_METADATA_BITS_LOCK_WR() \ - ocf_metadata_status_bits_lock(&cache->metadata.lock, \ - OCF_METADATA_WR) - -#define OCF_METADATA_BITS_UNLOCK_WR() \ - ocf_metadata_status_bits_unlock(&cache->metadata.lock, \ - OCF_METADATA_WR) - void ocf_hb_cline_prot_lock_rd(struct ocf_metadata_lock *metadata_lock, uint32_t lock_idx, uint32_t core_id, uint64_t core_line); void ocf_hb_cline_prot_unlock_rd(struct ocf_metadata_lock *metadata_lock, diff --git a/src/metadata/metadata_status.h b/src/metadata/metadata_status.h index 4aceea8..7249888 100644 --- a/src/metadata/metadata_status.h +++ b/src/metadata/metadata_status.h @@ -29,16 +29,12 @@ bool ocf_metadata_test_and_clear_valid(struct ocf_cache *cache, ocf_cache_line_t static inline void metadata_init_status_bits(struct ocf_cache *cache, ocf_cache_line_t line) { - OCF_METADATA_BITS_LOCK_WR(); - ocf_metadata_clear_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); ocf_metadata_clear_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); - - OCF_METADATA_BITS_UNLOCK_WR(); } static inline bool metadata_test_dirty_all(struct ocf_cache *cache, @@ -46,11 +42,9 @@ static inline bool metadata_test_dirty_all(struct ocf_cache *cache, { bool test; - OCF_METADATA_BITS_LOCK_RD(); test = ocf_metadata_test_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, true); - OCF_METADATA_BITS_UNLOCK_RD(); return test; } @@ -60,11 +54,9 @@ static inline bool metadata_test_dirty(struct ocf_cache *cache, { bool test; - OCF_METADATA_BITS_LOCK_RD(); test = ocf_metadata_test_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, false); - OCF_METADATA_BITS_UNLOCK_RD(); return test; } @@ -72,49 +64,33 @@ static inline bool metadata_test_dirty(struct ocf_cache *cache, static inline void metadata_set_dirty(struct ocf_cache *cache, ocf_cache_line_t line) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_set_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_clear_dirty(struct ocf_cache *cache, ocf_cache_line_t line) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline bool metadata_test_and_clear_dirty( struct ocf_cache *cache, ocf_cache_line_t line) { - bool test; - - OCF_METADATA_BITS_LOCK_WR(); - test = ocf_metadata_test_and_clear_dirty(cache, line, + return ocf_metadata_test_and_clear_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, false); - OCF_METADATA_BITS_UNLOCK_WR(); - - return test; } static inline bool metadata_test_and_set_dirty(struct ocf_cache *cache, ocf_cache_line_t line) { - bool test; - - OCF_METADATA_BITS_LOCK_WR(); - test = ocf_metadata_test_and_set_dirty(cache, line, + return ocf_metadata_test_and_set_dirty(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, false); - OCF_METADATA_BITS_UNLOCK_WR(); - - return test; } /******************************************************************************* @@ -124,27 +100,15 @@ static inline bool metadata_test_and_set_dirty(struct ocf_cache *cache, static inline bool metadata_test_dirty_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_dirty(cache, line, + return ocf_metadata_test_dirty(cache, line, start, stop, false); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline bool metadata_test_dirty_all_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_dirty(cache, line, + return ocf_metadata_test_dirty(cache, line, start, stop, true); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline bool metadata_test_dirty_one(struct ocf_cache *cache, @@ -156,59 +120,39 @@ static inline bool metadata_test_dirty_one(struct ocf_cache *cache, static inline bool metadata_test_dirty_out_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_out_dirty(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; + return ocf_metadata_test_out_dirty(cache, line, start, stop); } static inline void metadata_set_dirty_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_set_dirty(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_clear_dirty_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_dirty(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_set_dirty_sec_one(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t pos) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_set_dirty(cache, line, pos, pos); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_clear_dirty_sec_one(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t pos) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_dirty(cache, line, pos, pos); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline bool metadata_test_and_clear_dirty_sec( struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test = false; - - OCF_METADATA_BITS_LOCK_WR(); - test = ocf_metadata_test_and_clear_dirty(cache, line, + return ocf_metadata_test_and_clear_dirty(cache, line, start, stop, false); - OCF_METADATA_BITS_UNLOCK_WR(); - - return test; } /* @@ -223,15 +167,11 @@ static inline bool metadata_clear_dirty_sec_changed( { bool sec_changed; - OCF_METADATA_BITS_LOCK_WR(); - sec_changed = ocf_metadata_test_dirty(cache, line, start, stop, false); *line_is_clean = !ocf_metadata_clear_dirty(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); - return sec_changed; } @@ -247,12 +187,10 @@ static inline bool metadata_set_dirty_sec_changed( { bool sec_changed; - OCF_METADATA_BITS_LOCK_WR(); sec_changed = !ocf_metadata_test_dirty(cache, line, start, stop, true); *line_was_dirty = ocf_metadata_set_dirty(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); return sec_changed; } @@ -264,77 +202,49 @@ static inline bool metadata_set_dirty_sec_changed( static inline bool metadata_test_valid_any(struct ocf_cache *cache, ocf_cache_line_t line) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_valid(cache, line, + return ocf_metadata_test_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, false); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline bool metadata_test_valid(struct ocf_cache *cache, ocf_cache_line_t line) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_valid(cache, line, + return ocf_metadata_test_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, true); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline void metadata_set_valid(struct ocf_cache *cache, ocf_cache_line_t line) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_set_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_clear_valid(struct ocf_cache *cache, ocf_cache_line_t line) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline bool metadata_test_and_clear_valid( struct ocf_cache *cache, ocf_cache_line_t line) { - bool test = false; - - OCF_METADATA_BITS_LOCK_WR(); - test = ocf_metadata_test_and_clear_valid(cache, line, + return ocf_metadata_test_and_clear_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, true); - OCF_METADATA_BITS_UNLOCK_WR(); - - return test; } static inline bool metadata_test_and_set_valid(struct ocf_cache *cache, ocf_cache_line_t line) { - bool test = false; - - OCF_METADATA_BITS_LOCK_WR(); - test = ocf_metadata_test_and_set_valid(cache, line, + return ocf_metadata_test_and_set_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, true); - OCF_METADATA_BITS_UNLOCK_WR(); - - return test; } /******************************************************************************* @@ -344,28 +254,16 @@ static inline bool metadata_test_and_set_valid(struct ocf_cache *cache, static inline bool metadata_test_valid_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_valid(cache, line, + return ocf_metadata_test_valid(cache, line, start, stop, true); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline bool metadata_test_valid_any_out_sec( struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool test = false; - - OCF_METADATA_BITS_LOCK_RD(); - test = ocf_metadata_test_out_valid(cache, line, + return ocf_metadata_test_out_valid(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_RD(); - - return test; } static inline bool metadata_test_valid_one(struct ocf_cache *cache, @@ -385,38 +283,26 @@ static inline bool metadata_set_valid_sec_changed( struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - bool was_any_valid; - - OCF_METADATA_BITS_LOCK_WR(); - was_any_valid = ocf_metadata_set_valid(cache, line, + return !ocf_metadata_set_valid(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); - - return !was_any_valid; } static inline void metadata_clear_valid_sec(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t start, uint8_t stop) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_valid(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_clear_valid_sec_one(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t pos) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_clear_valid(cache, line, pos, pos); - OCF_METADATA_BITS_UNLOCK_WR(); } static inline void metadata_set_valid_sec_one(struct ocf_cache *cache, ocf_cache_line_t line, uint8_t pos) { - OCF_METADATA_BITS_LOCK_WR(); ocf_metadata_set_valid(cache, line, pos, pos); - OCF_METADATA_BITS_UNLOCK_WR(); } /* * Marks given cache line's bits as invalid @@ -431,8 +317,6 @@ static inline bool metadata_clear_valid_sec_changed( { bool was_any_valid; - OCF_METADATA_BITS_LOCK_WR(); - was_any_valid = ocf_metadata_test_valid(cache, line, cache->metadata.settings.sector_start, cache->metadata.settings.sector_end, false); @@ -440,8 +324,6 @@ static inline bool metadata_clear_valid_sec_changed( *is_valid = ocf_metadata_clear_valid(cache, line, start, stop); - OCF_METADATA_BITS_UNLOCK_WR(); - return was_any_valid && !*is_valid; } diff --git a/src/metadata/metadata_structs.h b/src/metadata/metadata_structs.h index 43feeb4..d2e7d3f 100644 --- a/src/metadata/metadata_structs.h +++ b/src/metadata/metadata_structs.h @@ -55,7 +55,6 @@ struct ocf_metadata_lock { struct ocf_metadata_global_lock global[OCF_NUM_GLOBAL_META_LOCKS]; /*!< global metadata lock (GML) */ - env_rwlock status; /*!< Fast lock for status bits */ env_rwlock eviction[OCF_NUM_EVICTION_LISTS]; /*!< Fast lock for eviction policy */ env_rwsem *hash; /*!< Hash bucket locks */ env_rwsem *collision_pages; /*!< Collision table page locks */