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

@@ -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!
+		ASSERT(block->original_data == NULL && block->parent_data == NULL);
-		if (block->parent_data != NULL
+		block->unused = false;
-			&& block->parent_data != block->original_data)
-			Free(block->parent_data);
-		if (block->original_data != NULL)
-			Free(block->original_data);
 
+		// 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

src/tests/system/kernel/cache/block_cache_test.cpp

@@ -13,9 +13,10 @@
 #undef read_pos
 
 
-#define MAX_BLOCKS				100
+#define MAX_BLOCKS					100
-#define BLOCK_CHANGED_IN_MAIN	(1L << 28)
+#define BLOCK_CHANGED_IN_MAIN		(1L << 16)
-#define BLOCK_CHANGED_IN_SUB	(2L << 28)
+#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];
+	dump_cache();
-	sprintf(cacheString, "%p", gCache);
-	char* argv[4];
-	argv[0] = "dump";
-	argv[1] = "-bt";
-	argv[2] = cacheString;
-	argv[3] = NULL;
-	dump_cache(3, argv);
 
 	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
+// #pragma mark - Tests
-main(int argc, char** argv)
+
+
+void
+test_abort_transaction()
 {
-	block_cache_init();
+	start_test("Abort main");
 
-	void* block;
+	int32 id = cache_start_transaction(gCache);
-	int32 id;
 
+	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;
 }