* cache_detach_sub_transaction(), and cache_abort_sub_transaction() now support

discarded blocks correctly as well.
* cache_detach_sub_transaction() left cached_block::original_data unchanged even
  if the parent data was to become current (in case the sub transaction didn't
  change the block yet). This could cause outdated blocks to be written back.
* cache_detach_sub_transaction() also set cached_block::previous_transaction
  for all blocks, not just the ones with a previous transaction. This could
  cause blocks to be written twice for no reason.
* cache_start_sub_transaction() did not change the num_blocks count for
  discarded blocks.
* block_cache_discard() now panics if the block was already changed in the
  current transaction.
* Improved test application, added more tests, revealing the above bugs in
  cache_detach_sub_transaction().
* Minor cleanup.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@28514 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/system/kernel/cache/block_cache.cpp

@@ -566,6 +566,8 @@ notify_transaction_listeners(block_cache* cache, cache_transaction* transaction,
 static void
 flush_pending_notifications(block_cache* cache)
 {
+	ASSERT_LOCKED_MUTEX(&sCachesLock);
+
 	while (true) {
 		MutexLocker locker(sNotificationsLock);
 
@@ -1203,11 +1205,11 @@ get_writable_cached_block(block_cache* cache, off_t blockNumber, off_t base,
 		if (cleared)
 			memset(block->current_data, 0, cache->block_size);
 
-		if (!block->is_dirty)
+		if (!block->is_dirty) {
 			cache->num_dirty_blocks++;
-
+			block->is_dirty = true;
-		block->is_dirty = true;
+				// mark the block as dirty
-			// mark the block as dirty
+		}
 
 		TB(Get(cache, block));
 		return block->current_data;
@@ -2112,9 +2114,13 @@ cache_detach_sub_transaction(void* _cache, int32 id,
 			// need to write back pending changes
 			write_cached_block(cache, block);
 		}
+		if (block->discard) {
+			cache->DiscardBlock(block);
+			transaction->main_num_blocks--;
+			continue;
+		}
 
-		if (block->original_data != NULL && block->parent_data != NULL
+		if (block->original_data != NULL && block->parent_data != NULL) {
-			&& block->parent_data != block->current_data) {
 			// free the original data if the parent data of the transaction
 			// will be made current - but keep them otherwise
 			cache->Free(block->original_data);
@@ -2137,10 +2143,10 @@ cache_detach_sub_transaction(void* _cache, int32 id,
 		if (block->parent_data != NULL) {
 			// move the block to the previous transaction list
 			transaction->blocks.Add(block);
+			block->previous_transaction = transaction;
 			block->parent_data = NULL;
 		}
 
-		block->previous_transaction = transaction;
 		block->transaction_next = NULL;
 	}
 
@@ -2200,6 +2206,7 @@ cache_abort_sub_transaction(void* _cache, int32 id)
 		}
 
 		block->parent_data = NULL;
+		block->discard = false;
 	}
 
 	// all subsequent changes will go into the main transaction
@@ -2242,6 +2249,7 @@ cache_start_sub_transaction(void* _cache, int32 id)
 				transaction->first_block = next;
 
 			cache->DiscardBlock(block);
+			transaction->num_blocks--;
 			continue;
 		}
 		if (transaction->has_sub_transaction
@@ -2556,6 +2564,12 @@ block_cache_discard(void* _cache, off_t blockNumber, size_t numBlocks)
 			cache->unused_blocks.Remove(block);
 			cache->RemoveBlock(block);
 		} else {
+			if (block->transaction != NULL && block->parent_data != NULL
+				&& block->parent_data != block->current_data) {
+				panic("Discarded block %Ld has already been changed in this "
+					"transaction!", blockNumber);
+			}
+
 			// mark it as discarded (in the current transaction only, if any)
 			block->discard = true;
 		}

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

@@ -13,17 +13,29 @@
 #undef read_pos
 
 
-#define MAX_BLOCKS	100
+#define MAX_BLOCKS				100
+#define BLOCK_CHANGED_IN_MAIN	(1L << 28)
+#define BLOCK_CHANGED_IN_SUB	(2L << 28)
+
+#define TEST_BLOCKS(number, count) \
+	test_blocks(number, count, __LINE__)
 
 #define TEST_BLOCK_DATA(block, number, type) \
 	if ((block)->type ## _data != NULL && gBlocks[(number)]. type == 0) \
-		error("Block %Ld: " #type " should be NULL!", (number)); \
+		error(line, "Block %Ld: " #type " should be NULL!", (number)); \
 	if ((block)->type ## _data != NULL && gBlocks[(number)]. type != 0 \
 		&& *(int32*)(block)->type ## _data != gBlocks[(number)]. type) { \
-		error("Block %Ld: " #type " wrong (%ld should be %ld)!", (number), \
+		error(line, "Block %Ld: " #type " wrong (0x%lx should be 0x%lx)!", \
-			*(int32*)(block)->type ## _data, gBlocks[(number)]. type); \
+			(number), *(int32*)(block)->type ## _data, \
+			gBlocks[(number)]. type); \
 	}
 
+#define TEST_ASSERT(statement) \
+	if (!(statement)) { \
+		error(__LINE__, "Assertion failed: " #statement); \
+	}
+
+
 struct test_block {
 	int32	current;
 	int32	original;
@@ -45,50 +57,17 @@ test_block gBlocks[MAX_BLOCKS];
 block_cache* gCache;
 size_t gBlockSize;
 int32 gTest;
+int32 gSubTest;
 const char* gTestName;
 
 
-ssize_t
-block_cache_write_pos(int fd, off_t offset, const void* buffer, size_t size)
-{
-	//printf("write: %Ld, %p, %lu\n", offset, buffer, size);
-	gBlocks[offset / gBlockSize].written = true;
-	return size;
-}
-
-
-ssize_t
-block_cache_read_pos(int fd, off_t offset, void* buffer, size_t size)
-{
-	//printf("read: %Ld, %p, %lu\n", offset, buffer, size);
-	memset(buffer, 0xcc, size);
-	int32* value = (int32*)buffer;
-	*value = offset / gBlockSize + 1;
-	gBlocks[offset / gBlockSize].read = true;
-
-	return size;
-}
-
-
 void
-init_test_blocks()
+error(int32 line, const char* format, ...)
-{
-	memset(gBlocks, 0, sizeof(test_block) * MAX_BLOCKS);
-
-	for (uint32 i = 0; i < MAX_BLOCKS; i++) {
-		gBlocks[i].current = i + 1;
-		gBlocks[i].unused = true;
-	}
-}
-
-
-void
-error(const char* format, ...)
 {
 	va_list args;
 	va_start(args, format);
 
-	fprintf(stderr, "ERROR: ");
+	fprintf(stderr, "ERROR IN TEST LINE %ld: ", line);
 	vfprintf(stderr, format, args);
 	fprintf(stderr, "\n");
 
@@ -108,23 +87,88 @@ error(const char* format, ...)
 
 
 void
-test_blocks(off_t number, int32 count)
+or_block(void* block, int32 value)
 {
+	int32* data = (int32*)block;
+	*data |= value;
+}
+
+
+void
+set_block(void* block, int32 value)
+{
+	int32* data = (int32*)block;
+	*data = value;
+}
+
+
+void
+reset_block(void* block, int32 index)
+{
+	int32* data = (int32*)block;
+	*data = index + 1;
+}
+
+
+ssize_t
+block_cache_write_pos(int fd, off_t offset, const void* buffer, size_t size)
+{
+	int32 index = offset / gBlockSize;
+
+	gBlocks[index].written = true;
+	if (!gBlocks[index].write)
+		error(__LINE__, "Block %ld should not be written!\n", index);
+
+	return size;
+}
+
+
+ssize_t
+block_cache_read_pos(int fd, off_t offset, void* buffer, size_t size)
+{
+	int32 index = offset / gBlockSize;
+
+	memset(buffer, 0xcc, size);
+	reset_block(buffer, index);
+	if (!gBlocks[index].read)
+		error(__LINE__, "Block %ld should not be written!\n", index);
+
+	return size;
+}
+
+
+void
+init_test_blocks()
+{
+	memset(gBlocks, 0, sizeof(test_block) * MAX_BLOCKS);
+
+	for (uint32 i = 0; i < MAX_BLOCKS; i++) {
+		gBlocks[i].current = i + 1;
+		gBlocks[i].unused = true;
+	}
+}
+
+
+void
+test_blocks(off_t number, int32 count, int32 line)
+{
+	printf("  %ld\n", gSubTest++);
+
 	for (int32 i = 0; i < count; i++, number++) {
 		MutexLocker locker(&gCache->lock);
 
 		cached_block* block = (cached_block*)hash_lookup(gCache->hash, &number);
 		if (block == NULL) {
 			if (gBlocks[number].present)
-				error("Block %Ld not found!", number);
+				error(line, "Block %Ld not found!", number);
 			continue;
 		}
 		if (!gBlocks[number].present)
-			error("Block %Ld is present, but should not!", number);
+			error(line, "Block %Ld is present, but should not!", number);
 
 		if (block->is_dirty != gBlocks[number].is_dirty) {
-			error("Block %Ld: dirty bit differs (%d should be %d)!", number,
+			error(line, "Block %Ld: dirty bit differs (%d should be %d)!",
-				block->is_dirty, gBlocks[number].is_dirty);
+				number, block->is_dirty, gBlocks[number].is_dirty);
 		}
 #if 0
 		if (block->unused != gBlocks[number].unused) {
@@ -133,11 +177,11 @@ test_blocks(off_t number, int32 count)
 		}
 #endif
 		if (block->discard != gBlocks[number].discard) {
-			error("Block %Ld: discard bit differs (%d should be %d)!", number,
+			error(line, "Block %Ld: discard bit differs (%d should be %d)!",
-				block->discard, gBlocks[number].discard);
+				number, block->discard, gBlocks[number].discard);
 		}
 		if (gBlocks[number].write && !gBlocks[number].written)
-			error("Block %Ld: has not been written yet!", number);
+			error(line, "Block %Ld: has not been written yet!", number);
 
 		TEST_BLOCK_DATA(block, number, current);
 		TEST_BLOCK_DATA(block, number, original);
@@ -146,26 +190,19 @@ test_blocks(off_t number, int32 count)
 }
 
 
-void
-test_block(off_t block)
-{
-	test_blocks(block, 1);
-}
-
-
 void
 stop_test(void)
 {
 	if (gCache == NULL)
 		return;
-	test_blocks(0, MAX_BLOCKS);
+	TEST_BLOCKS(0, MAX_BLOCKS);
 
 	block_cache_delete(gCache, true);
 }
 
 
 void
-start_test(int32 test, const char* name, bool init = false)
+start_test(const char* name, bool init = false)
 {
 	if (init) {
 		stop_test();
@@ -177,33 +214,99 @@ start_test(int32 test, const char* name, bool init = false)
 		init_test_blocks();
 	}
 
-	gTest = test;
+	gTest++;
 	gTestName = name;
+	gSubTest = 1;
+
 	printf("----------- Test %ld%s%s -----------\n", gTest,
 		gTestName[0] ? " - " : "", gTestName);
 }
 
 
+/*!	Changes block 1 in main, block 2 if touchedInMain is \c true.
+	Changes block 0 in sub, discards block 2.
+	Performs two block tests.
+*/
+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);
+	or_block(block, BLOCK_CHANGED_IN_MAIN);
+	block_cache_put(gCache, 1);
+
+	TEST_BLOCKS(0, 2);
+
+	if (touchedInMain) {
+		gBlocks[2].present = true;
+		gBlocks[2].is_dirty = true;
+		gBlocks[2].current |= BLOCK_CHANGED_IN_MAIN;
+
+		block = block_cache_get_empty(gCache, 2, id);
+		reset_block(block, 2);
+		or_block(block, BLOCK_CHANGED_IN_MAIN);
+		block_cache_put(gCache, 2);
+	}
+
+	cache_start_sub_transaction(gCache, id);
+
+	gBlocks[0].present = true;
+	gBlocks[0].read = true;
+	gBlocks[0].is_dirty = true;
+	if ((gBlocks[0].current & BLOCK_CHANGED_IN_MAIN) != 0)
+		gBlocks[0].parent = gBlocks[0].current;
+	else
+		gBlocks[0].original = gBlocks[0].current;
+	gBlocks[0].current |= BLOCK_CHANGED_IN_SUB;
+
+	gBlocks[1].parent = gBlocks[1].current;
+
+	block = block_cache_get_writable(gCache, 0, id);
+	or_block(block, BLOCK_CHANGED_IN_SUB);
+	block_cache_put(gCache, 0);
+
+	gBlocks[2].discard = true;
+
+	block_cache_discard(gCache, 2, 1);
+
+	TEST_BLOCKS(0, 2);
+
+	gBlocks[0].is_dirty = false;
+	gBlocks[0].write = true;
+	gBlocks[1].is_dirty = false;
+	gBlocks[1].write = true;
+	gBlocks[2].present = false;
+}
+
+
 int
 main(int argc, char** argv)
 {
 	block_cache_init();
 
-	const void* block;
+	void* block;
+	int32 id;
 
 	// TODO: test transaction-less block caches
 	// TODO: test read-only block caches
 
 	// Test transactions and block caches
 
-	start_test(1, "Discard simple", true);
+	start_test("Discard in main", true);
 
-	int32 id = cache_start_transaction(gCache);
+	id = cache_start_transaction(gCache);
 
 	gBlocks[0].present = true;
 	gBlocks[0].read = true;
 
-	block = block_cache_get(gCache, 0);
+	block_cache_get(gCache, 0);
 	block_cache_put(gCache, 0);
 
 	gBlocks[1].present = true;
@@ -222,6 +325,109 @@ main(int argc, char** argv)
 	cache_end_transaction(gCache, id, NULL, NULL);
 	cache_sync_transaction(gCache, id);
 
+	start_test("Discard in sub", true);
+
+	id = cache_start_transaction(gCache);
+
+	basic_test_discard_in_sub(id, false);
+	TEST_ASSERT(cache_blocks_in_sub_transaction(gCache, id) == 1);
+
+	cache_end_transaction(gCache, id, NULL, NULL);
+	cache_sync_transaction(gCache, id);
+
+	start_test("Discard in sub, present in main", true);
+
+	id = cache_start_transaction(gCache);
+
+	basic_test_discard_in_sub(id, true);
+
+	cache_end_transaction(gCache, id, NULL, NULL);
+	cache_sync_transaction(gCache, id);
+
+	start_test("Discard in sub, changed in main, abort sub", true);
+
+	id = cache_start_transaction(gCache);
+
+	basic_test_discard_in_sub(id, true);
+	TEST_ASSERT(cache_blocks_in_sub_transaction(gCache, id) == 1);
+
+	gBlocks[0].current &= ~BLOCK_CHANGED_IN_SUB;
+	gBlocks[2].present = 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);
+
+	id = cache_start_transaction(gCache);
+
+	basic_test_discard_in_sub(id, true);
+
+	cache_start_sub_transaction(gCache, id);
+	cache_end_transaction(gCache, id, NULL, NULL);
+	cache_sync_transaction(gCache, id);
+
+	start_test("Discard in sub, changed in main, detach sub", true);
+
+	id = cache_start_transaction(gCache);
+
+	basic_test_discard_in_sub(id, true);
+
+	id = cache_detach_sub_transaction(gCache, id, NULL, NULL);
+
+	gBlocks[0].is_dirty = true;
+	gBlocks[0].write = false;
+	gBlocks[1].is_dirty = true;
+	gBlocks[1].write = false;
+
+	TEST_BLOCKS(0, 2);
+
+	gBlocks[0].is_dirty = false;
+	gBlocks[0].write = true;
+	gBlocks[1].is_dirty = false;
+	gBlocks[1].write = true;
+
+	cache_end_transaction(gCache, id, NULL, NULL);
+	cache_sync_transaction(gCache, id);
+
+	start_test("Discard in sub, all changed in main, detach sub", true);
+
+	id = cache_start_transaction(gCache);
+
+	gBlocks[0].present = true;
+	gBlocks[0].read = true;
+	gBlocks[0].is_dirty = true;
+	gBlocks[0].original = gBlocks[0].current;
+	gBlocks[0].current |= BLOCK_CHANGED_IN_MAIN;
+
+	block = block_cache_get_writable(gCache, 0, id);
+	or_block(block, BLOCK_CHANGED_IN_MAIN);
+	block_cache_put(gCache, 0);
+
+	basic_test_discard_in_sub(id, true);
+
+	id = cache_detach_sub_transaction(gCache, id, NULL, NULL);
+
+	gBlocks[0].is_dirty = true;
+	gBlocks[0].write = false;
+	gBlocks[0].original |= BLOCK_CHANGED_IN_MAIN;
+	gBlocks[1].is_dirty = true;
+	gBlocks[1].write = false;
+
+	TEST_BLOCKS(0, 2);
+
+	gBlocks[0].is_dirty = false;
+	gBlocks[0].write = true;
+	gBlocks[1].is_dirty = false;
+	gBlocks[1].write = true;
+
+	cache_end_transaction(gCache, id, NULL, NULL);
+	cache_sync_transaction(gCache, id);
+
 	stop_test();
 	return 0;
 }