* Thanks to Janito I had a closer look to cache_detach_sub_transaction(), and it
turned out it's pretty much broken. Not only did it potentially leak memory, it would also potentially replace the original data with a NULL pointer, making the current transaction non revertable. The code should now be much clearer. This fixes bug #6378. * Changed a few "if (... != NULL)" into ASSERTs, since the code should bail out earlier in these cases already. * Added a TODO comment to cache_start_sub_transaction() about its broken discard handling. This can cause FS corruptions in case the parent transaction is ever going to be aborted (which shouldn't happen in real life, though). * Added a bit more and better comments. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@37899 a95241bf-73f2-0310-859d-f6bbb57e9c96
This commit is contained in:
parent
c6247544ed
commit
3426ce1404
|
@ -60,8 +60,19 @@ struct cached_block {
|
|||
block_link link;
|
||||
off_t block_number;
|
||||
void* current_data;
|
||||
// The data that is seen by everyone using the API; this one is always
|
||||
// present.
|
||||
void* original_data;
|
||||
// When in a transaction, this contains the original data from before
|
||||
// the transaction.
|
||||
void* parent_data;
|
||||
// This is a lazily alloced buffer that represents the contents of the
|
||||
// block in the parent transaction. It may point to current_data if the
|
||||
// contents have been changed only in the parent transaction, or, if the
|
||||
// block has been changed in the current sub transaction already, to a
|
||||
// new block containing the contents changed in the parent transaction.
|
||||
// If this is NULL, the block has not been changed in the parent
|
||||
// transaction at all.
|
||||
#if BLOCK_CACHE_DEBUG_CHANGED
|
||||
void* compare;
|
||||
#endif
|
||||
|
@ -2838,9 +2849,10 @@ cache_abort_transaction(void* _cache, int32 id)
|
|||
next = block->transaction_next;
|
||||
|
||||
if (block->original_data != NULL) {
|
||||
TRACE(("cache_abort_transaction(id = %ld): restored contents of block %Ld\n",
|
||||
transaction->id, block->block_number));
|
||||
memcpy(block->current_data, block->original_data, cache->block_size);
|
||||
TRACE(("cache_abort_transaction(id = %ld): restored contents of "
|
||||
"block %Ld\n", transaction->id, block->block_number));
|
||||
memcpy(block->current_data, block->original_data,
|
||||
cache->block_size);
|
||||
cache->Free(block->original_data);
|
||||
block->original_data = NULL;
|
||||
}
|
||||
|
@ -2925,16 +2937,31 @@ cache_detach_sub_transaction(void* _cache, int32 id,
|
|||
continue;
|
||||
}
|
||||
|
||||
if (block->original_data != NULL && block->parent_data != NULL) {
|
||||
// free the original data if the parent data of the transaction
|
||||
// will be made current - but keep them otherwise
|
||||
if (block->parent_data != NULL) {
|
||||
// The block changed in the parent - free the original data, since
|
||||
// they will be replaced by what is in current.
|
||||
ASSERT(block->original_data != NULL);
|
||||
cache->Free(block->original_data);
|
||||
block->original_data = NULL;
|
||||
|
||||
if (block->parent_data != block->current_data) {
|
||||
// The block had been changed in both transactions
|
||||
block->original_data = block->parent_data;
|
||||
} else {
|
||||
// The block has only been changed in the parent
|
||||
block->original_data = NULL;
|
||||
}
|
||||
|
||||
// move the block to the previous transaction list
|
||||
transaction->blocks.Add(block);
|
||||
block->previous_transaction = transaction;
|
||||
block->parent_data = NULL;
|
||||
}
|
||||
if (block->parent_data == NULL
|
||||
|| block->parent_data != block->current_data) {
|
||||
// we need to move this block over to the new transaction
|
||||
block->original_data = block->parent_data;
|
||||
|
||||
if (block->original_data != NULL) {
|
||||
// This block had been changed in the current sub transaction,
|
||||
// we need to move this block over to the new transaction.
|
||||
ASSERT(block->parent_data == NULL);
|
||||
|
||||
if (last == NULL)
|
||||
newTransaction->first_block = block;
|
||||
else
|
||||
|
@ -2945,13 +2972,6 @@ cache_detach_sub_transaction(void* _cache, int32 id,
|
|||
} else
|
||||
block->transaction = NULL;
|
||||
|
||||
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->transaction_next = NULL;
|
||||
}
|
||||
|
||||
|
@ -2996,16 +3016,15 @@ cache_abort_sub_transaction(void* _cache, int32 id)
|
|||
next = block->transaction_next;
|
||||
|
||||
if (block->parent_data == NULL) {
|
||||
if (block->original_data != NULL) {
|
||||
// the parent transaction didn't change the block, but the sub
|
||||
// transaction did - we need to revert from the original data
|
||||
memcpy(block->current_data, block->original_data,
|
||||
cache->block_size);
|
||||
}
|
||||
// the parent transaction didn't change the block, but the sub
|
||||
// transaction did - we need to revert from the original data
|
||||
ASSERT(block->original_data != NULL);
|
||||
memcpy(block->current_data, block->original_data,
|
||||
cache->block_size);
|
||||
} else if (block->parent_data != block->current_data) {
|
||||
// the block has been changed and must be restored
|
||||
TRACE(("cache_abort_sub_transaction(id = %ld): restored contents of block %Ld\n",
|
||||
transaction->id, block->block_number));
|
||||
TRACE(("cache_abort_sub_transaction(id = %ld): restored contents "
|
||||
"of block %Ld\n", transaction->id, block->block_number));
|
||||
memcpy(block->current_data, block->parent_data, cache->block_size);
|
||||
cache->Free(block->parent_data);
|
||||
}
|
||||
|
@ -3032,7 +3051,8 @@ cache_start_sub_transaction(void* _cache, int32 id)
|
|||
|
||||
cache_transaction* transaction = lookup_transaction(cache, id);
|
||||
if (transaction == NULL) {
|
||||
panic("cache_start_sub_transaction(): invalid transaction ID %ld\n", id);
|
||||
panic("cache_start_sub_transaction(): invalid transaction ID %ld\n",
|
||||
id);
|
||||
return B_BAD_VALUE;
|
||||
}
|
||||
|
||||
|
@ -3048,6 +3068,9 @@ cache_start_sub_transaction(void* _cache, int32 id)
|
|||
|
||||
if (block->discard) {
|
||||
// This block has been discarded in the parent transaction
|
||||
// TODO: this is wrong: since the parent transaction is not
|
||||
// finished here (just extended), this block cannot be reverted
|
||||
// anymore in case the parent transaction is aborted!!!
|
||||
if (last != NULL)
|
||||
last->transaction_next = next;
|
||||
else
|
||||
|
@ -3057,6 +3080,7 @@ cache_start_sub_transaction(void* _cache, int32 id)
|
|||
transaction->num_blocks--;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (transaction->has_sub_transaction
|
||||
&& block->parent_data != NULL
|
||||
&& block->parent_data != block->current_data) {
|
||||
|
|
Loading…
Reference in New Issue