From 713945cecb4e18a400b5dc92797a6defe3f8a4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Mon, 27 Aug 2012 19:56:06 +0200 Subject: [PATCH] The original_data could be freed late. * In cache_abort_sub_transaction(), the original_data can already be freed when the block is being removed from the transaction. * block_cache::_GetUnusedBlock() no longer frees original/parent data - it now requires them to be freed already (it makes no sense to have them still around at this point). * AFAICT the previous version did not have any negative consequences besides freeing the original data late. --- src/system/kernel/cache/block_cache.cpp | 22 +- .../system/kernel/cache/block_cache_test.cpp | 213 +++++++++++++++--- 2 files changed, 192 insertions(+), 43 deletions(-) diff --git a/src/system/kernel/cache/block_cache.cpp b/src/system/kernel/cache/block_cache.cpp index 28c4752452..7cd01571e0 100644 --- a/src/system/kernel/cache/block_cache.cpp +++ b/src/system/kernel/cache/block_cache.cpp @@ -1291,6 +1291,7 @@ BlockWriter::_BlockDone(cached_block* block, hash_iterator* iterator) } if (block->transaction == NULL && block->ref_count == 0 && !block->unused) { // the block is no longer used + ASSERT(block->original_data == NULL && block->parent_data == NULL); block->unused = true; fCache->unused_blocks.Add(block); fCache->unused_block_count++; @@ -1639,13 +1640,10 @@ block_cache::_GetUnusedBlock() unused_block_count--; hash_remove(hash, block); - // TODO: see if parent/compare data is handled correctly here! - if (block->parent_data != NULL - && block->parent_data != block->original_data) - Free(block->parent_data); - if (block->original_data != NULL) - Free(block->original_data); + ASSERT(block->original_data == NULL && block->parent_data == NULL); + block->unused = false; + // TODO: see if compare data is handled correctly here! #if BLOCK_CACHE_DEBUG_CHANGED if (block->compare != NULL) Free(block->compare); @@ -3059,8 +3057,18 @@ cache_abort_sub_transaction(void* _cache, int32 id) block->transaction_next = NULL; block->transaction = NULL; - if (block->previous_transaction == NULL) + if (block->previous_transaction == NULL) { + cache->Free(block->original_data); + block->original_data = NULL; block->is_dirty = false; + + if (block->ref_count == 0) { + // Move the block into the unused list if possible + block->unused = true; + cache->unused_blocks.Add(block); + cache->unused_block_count++; + } + } } else { if (block->parent_data != block->current_data) { // The block has been changed and must be restored - the block diff --git a/src/tests/system/kernel/cache/block_cache_test.cpp b/src/tests/system/kernel/cache/block_cache_test.cpp index e9ed5f1396..f03f37b4eb 100644 --- a/src/tests/system/kernel/cache/block_cache_test.cpp +++ b/src/tests/system/kernel/cache/block_cache_test.cpp @@ -13,9 +13,10 @@ #undef read_pos -#define MAX_BLOCKS 100 -#define BLOCK_CHANGED_IN_MAIN (1L << 28) -#define BLOCK_CHANGED_IN_SUB (2L << 28) +#define MAX_BLOCKS 100 +#define BLOCK_CHANGED_IN_MAIN (1L << 16) +#define BLOCK_CHANGED_IN_SUB (2L << 16) +#define BLOCK_CHANGED_IN_PREVIOUS (4L << 16) #define TEST_BLOCKS(number, count) \ test_blocks(number, count, __LINE__) @@ -61,6 +62,20 @@ int32 gSubTest; const char* gTestName; +void +dump_cache() +{ + char cacheString[32]; + sprintf(cacheString, "%p", gCache); + char* argv[4]; + argv[0] = "dump"; + argv[1] = "-bt"; + argv[2] = cacheString; + argv[3] = NULL; + dump_cache(3, argv); +} + + void error(int32 line, const char* format, ...) { @@ -73,14 +88,7 @@ error(int32 line, const char* format, ...) va_end(args); - char cacheString[32]; - sprintf(cacheString, "%p", gCache); - char* argv[4]; - argv[0] = "dump"; - argv[1] = "-bt"; - argv[2] = cacheString; - argv[3] = NULL; - dump_cache(3, argv); + dump_cache(); exit(1); } @@ -131,7 +139,7 @@ block_cache_read_pos(int fd, off_t offset, void* buffer, size_t size) memset(buffer, 0xcc, size); reset_block(buffer, index); if (!gBlocks[index].read) - error(__LINE__, "Block %ld should not be written!\n", index); + error(__LINE__, "Block %ld should not be read!\n", index); return size; } @@ -167,17 +175,17 @@ test_blocks(off_t number, int32 count, int32 line) error(line, "Block %Ld is present, but should not!", number); if (block->is_dirty != gBlocks[number].is_dirty) { - error(line, "Block %Ld: dirty bit differs (%d should be %d)!", + error(line, "Block %Ld: dirty bit differs (is %d should be %d)!", number, block->is_dirty, gBlocks[number].is_dirty); } #if 0 if (block->unused != gBlocks[number].unused) { - error("Block %ld: discard bit differs (%d should be %d)!", number, + error("Block %ld: unused bit differs (%d should be %d)!", number, block->unused, gBlocks[number].unused); } #endif if (block->discard != gBlocks[number].discard) { - error(line, "Block %Ld: discard bit differs (%d should be %d)!", + error(line, "Block %Ld: discard bit differs (is %d should be %d)!", number, block->discard, gBlocks[number].discard); } if (gBlocks[number].write && !gBlocks[number].written) @@ -197,12 +205,13 @@ stop_test(void) return; TEST_BLOCKS(0, MAX_BLOCKS); +// dump_cache(); block_cache_delete(gCache, true); } void -start_test(const char* name, bool init = false) +start_test(const char* name, bool init = true) { if (init) { stop_test(); @@ -230,15 +239,13 @@ start_test(const char* name, bool init = false) void basic_test_discard_in_sub(int32 id, bool touchedInMain) { - void* block; - gBlocks[1].present = true; gBlocks[1].read = true; gBlocks[1].is_dirty = true; gBlocks[1].original = gBlocks[1].current; gBlocks[1].current |= BLOCK_CHANGED_IN_MAIN; - block = block_cache_get_writable(gCache, 1, id); + void* block = block_cache_get_writable(gCache, 1, id); or_block(block, BLOCK_CHANGED_IN_MAIN); block_cache_put(gCache, 1); @@ -267,6 +274,8 @@ basic_test_discard_in_sub(int32 id, bool touchedInMain) gBlocks[0].current |= BLOCK_CHANGED_IN_SUB; gBlocks[1].parent = gBlocks[1].current; + if (touchedInMain) + gBlocks[2].parent = gBlocks[2].current; block = block_cache_get_writable(gCache, 0, id); or_block(block, BLOCK_CHANGED_IN_SUB); @@ -283,25 +292,128 @@ basic_test_discard_in_sub(int32 id, bool touchedInMain) gBlocks[1].is_dirty = false; gBlocks[1].write = true; gBlocks[2].present = false; + gBlocks[2].write = touchedInMain; + gBlocks[2].is_dirty = false; } -int -main(int argc, char** argv) +// #pragma mark - Tests + + +void +test_abort_transaction() { - block_cache_init(); + start_test("Abort main"); - void* block; - int32 id; + int32 id = cache_start_transaction(gCache); + gBlocks[0].present = true; + gBlocks[0].read = false; + gBlocks[0].write = false; + gBlocks[0].is_dirty = true; + gBlocks[1].present = true; + gBlocks[1].read = true; + gBlocks[1].write = false; + gBlocks[1].is_dirty = true; + + void* block = block_cache_get_empty(gCache, 0, id); + or_block(block, BLOCK_CHANGED_IN_PREVIOUS); + gBlocks[0].current = BLOCK_CHANGED_IN_PREVIOUS; + + block = block_cache_get_writable(gCache, 1, id); + or_block(block, BLOCK_CHANGED_IN_PREVIOUS); + gBlocks[1].original = gBlocks[1].current; + gBlocks[1].current |= BLOCK_CHANGED_IN_PREVIOUS; + + block_cache_put(gCache, 0); + block_cache_put(gCache, 1); + + cache_end_transaction(gCache, id, NULL, NULL); + TEST_BLOCKS(0, 2); + + id = cache_start_transaction(gCache); + + block = block_cache_get_writable(gCache, 0, id); + or_block(block, BLOCK_CHANGED_IN_MAIN); + + block = block_cache_get_writable(gCache, 1, id); + or_block(block, BLOCK_CHANGED_IN_MAIN); + + block_cache_put(gCache, 0); + block_cache_put(gCache, 1); + + cache_abort_transaction(gCache, id); + + gBlocks[0].write = true; + gBlocks[0].is_dirty = false; + gBlocks[1].write = true; + gBlocks[1].is_dirty = false; + cache_sync_transaction(gCache, id); +} + + +void +test_abort_sub_transaction() +{ + start_test("Abort sub"); + + int32 id = cache_start_transaction(gCache); + + gBlocks[0].present = true; + gBlocks[0].read = false; + gBlocks[0].write = false; + gBlocks[0].is_dirty = true; + gBlocks[1].present = true; + gBlocks[1].read = true; + gBlocks[1].write = false; + gBlocks[1].is_dirty = true; + + void* block = block_cache_get_empty(gCache, 0, id); + or_block(block, BLOCK_CHANGED_IN_PREVIOUS); + gBlocks[0].current = BLOCK_CHANGED_IN_PREVIOUS; + + block = block_cache_get_writable(gCache, 1, id); + or_block(block, BLOCK_CHANGED_IN_PREVIOUS); + gBlocks[1].original = gBlocks[1].current; + gBlocks[1].current |= BLOCK_CHANGED_IN_PREVIOUS; + + block_cache_put(gCache, 0); + block_cache_put(gCache, 1); + + cache_start_sub_transaction(gCache, id); + + gBlocks[0].parent = gBlocks[0].current; + gBlocks[1].parent = gBlocks[1].current; + TEST_BLOCKS(0, 2); + + block = block_cache_get_writable(gCache, 1, id); + or_block(block, BLOCK_CHANGED_IN_MAIN); + + block_cache_put(gCache, 1); + + cache_abort_sub_transaction(gCache, id); + + gBlocks[0].write = true; + gBlocks[0].is_dirty = false; + gBlocks[1].write = true; + gBlocks[1].is_dirty = false; + + cache_end_transaction(gCache, id, NULL, NULL); + cache_sync_transaction(gCache, id); +} + + +void +test_block_cache_discard() +{ // TODO: test transaction-less block caches // TODO: test read-only block caches // Test transactions and block caches - start_test("Discard in main", true); + start_test("Discard in main"); - id = cache_start_transaction(gCache); + int32 id = cache_start_transaction(gCache); gBlocks[0].present = true; gBlocks[0].read = true; @@ -313,7 +425,7 @@ main(int argc, char** argv) gBlocks[1].read = true; gBlocks[1].write = true; - block = block_cache_get_writable(gCache, 1, id); + void* block = block_cache_get_writable(gCache, 1, id); block_cache_put(gCache, 1); gBlocks[2].present = false; @@ -325,7 +437,7 @@ main(int argc, char** argv) cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub", true); + start_test("Discard in sub"); id = cache_start_transaction(gCache); @@ -335,7 +447,10 @@ main(int argc, char** argv) cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub, present in main", true); + gBlocks[0].is_dirty = false; + gBlocks[1].is_dirty = false; + + start_test("Discard in sub, present in main"); id = cache_start_transaction(gCache); @@ -344,7 +459,10 @@ main(int argc, char** argv) cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub, changed in main, abort sub", true); + gBlocks[0].is_dirty = false; + gBlocks[1].is_dirty = false; + + start_test("Discard in sub, changed in main, abort sub"); id = cache_start_transaction(gCache); @@ -352,16 +470,25 @@ main(int argc, char** argv) TEST_ASSERT(cache_blocks_in_sub_transaction(gCache, id) == 1); gBlocks[0].current &= ~BLOCK_CHANGED_IN_SUB; + gBlocks[0].is_dirty = false; + gBlocks[0].write = false; + gBlocks[1].write = false; + gBlocks[1].is_dirty = true; gBlocks[2].present = true; + gBlocks[2].is_dirty = true; + gBlocks[2].write = false; + gBlocks[2].discard = false; + cache_abort_sub_transaction(gCache, id); + TEST_BLOCKS(0, 2); + + gBlocks[1].is_dirty = false; + gBlocks[1].write = true; gBlocks[2].is_dirty = false; gBlocks[2].write = true; - gBlocks[2].discard = false; - - cache_abort_sub_transaction(gCache, id); cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub, changed in main, new sub", true); + start_test("Discard in sub, changed in main, new sub"); id = cache_start_transaction(gCache); @@ -371,7 +498,7 @@ main(int argc, char** argv) cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub, changed in main, detach sub", true); + start_test("Discard in sub, changed in main, detach sub"); id = cache_start_transaction(gCache); @@ -394,7 +521,7 @@ main(int argc, char** argv) cache_end_transaction(gCache, id, NULL, NULL); cache_sync_transaction(gCache, id); - start_test("Discard in sub, all changed in main, detach sub", true); + start_test("Discard in sub, all changed in main, detach sub"); id = cache_start_transaction(gCache); @@ -429,5 +556,19 @@ main(int argc, char** argv) cache_sync_transaction(gCache, id); stop_test(); +} + + +// #pragma mark - + + +int +main(int argc, char** argv) +{ + block_cache_init(); + + test_abort_transaction(); + test_abort_sub_transaction(); + test_block_cache_discard(); return 0; }