Re: [RFC PATCH] btrfs: Introduce developer-oriented check to ensure all tree blocks are written back before writing super blocks

[Qu Wenruo <wqu@suse.de>]

On 2019/2/21 下午10:25, Nikolay Borisov wrote:
> 
> 
> On 21.02.19 г. 10:22 ч., Qu Wenruo wrote:
>> There are a lot of error reports complaining about transid error in the
>> mail list.
>>
>> Under most case, the on-disk transid is lower than expected transid.
>> This may indicate that some tree blocks are not written back to disk
>> before writing super blocks.
>>
>> This patch will add a safe net for developers, by calling
>> btrfs_write_and_wait_transaction() before setting transaction unblocked
>> and double check btree_inode and dirty_pages io_tree, to ensure no tree
>> blocks are still dirty or under writeback.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> The reason for RFC is, I'm not sure why we currently call
>> btrfs_write_and_wait_transaction() after setting transaction UNBLOCKED.
>>
>> It looks like an optimization, but I don't see much performance
>> difference during regression test.
>>
>> I hope to move the call before we unblock transaction so we can do such
>> sanity check for all builds and hope to catch some clue of transid
>> error.
> 
> Even current code ensures that all allocated blocks in the current
> transaction (which is what all those EXTENT_DIRTY extents in the
> dirty_pages tree ) are written before the new superblocks are.
> 
> Slight offtopic: In fact instead of playing games with the flags and
> having an extent_io_tree called dirty_pages o_O it can be replaced with
> a simple linked list that holds all newly allocated buffers so writing
> all such buffers will result in simply iterating the list.
> 
> In any case this patch is buggy, see below on why
> 
>> ---
>>  fs/btrfs/transaction.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 4ec2b660d014..30b7ed0bf873 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -2213,6 +2213,44 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>>  
>>  	btrfs_trans_release_chunk_metadata(trans);
>>  
>> +	/* Last safenet or developer to catch any unwritten tree blocks */
>> +	if (IS_ENABLED(CONFIG_BTRFS_DEBUG)) {
>> +		u64 found_start = 0;
>> +		u64 found_end = 0;
>> +
>> +		ret = btrfs_write_and_wait_transaction(trans);
>> +		if (ret) {
>> +			btrfs_handle_fs_error(fs_info, ret,
>> +					      "Error while writing out transaction");
>> +			mutex_unlock(&fs_info->tree_log_mutex);
>> +			goto scrub_continue;
>> +		}
>> +
>> +		/* No dirty extent should exist in btree inode */
>> +		ret = test_range_bit(&trans->transaction->dirty_pages, 0,
>> +				(u64)-1, EXTENT_DIRTY | EXTENT_WRITEBACK,
> 
> Why do you check EXTENT_WRITEBACK, AFAICS that flag is not currently
> used in the code and should perhahps be deleted? I don't see anything
> setting it, it's only being checked for (as part of EXTENT_IOBITS).
> 
> Additionally this check is pointless because
> btrfs_write_and_wait_transaction calls clear_btree_io_tree which purges
> the tree.

But we still have BTRFS_I(fs_info->btree_inode)->io_tree, and that's the
main part of the check.

I don't really think the dirty_pages is really an important thing
compared to btree_inode.
If there is some way to find any dirty pages from an address_space, it
would be even better.

Thanks,
Qu

> 
>> +				0, NULL);
>> +		if (ret > 0) {
>> +			WARN(1,
>> +		"dirty_pages not fully written back, start=%llu len=%llu\n",
>> +			     found_start, found_end + 1 - found_start);
>> +			ret = -EUCLEAN;
>> +			mutex_unlock(&fs_info->tree_log_mutex);
>> +			goto scrub_continue;
>> +		}
>> +		ret = test_range_bit(&BTRFS_I(fs_info->btree_inode)->io_tree, 0,
>> +				     (u64)-1, EXTENT_DIRTY | EXTENT_WRITEBACK,
>> +				     0, NULL);
>> +		if (ret > 0) {
>> +			WARN(1,
>> +		"btree io_tree not fully written back, start=%llu len=%llu\n",
>> +			     found_start, found_end + 1 - found_start);
>> +			ret = -EUCLEAN;
>> +			mutex_unlock(&fs_info->tree_log_mutex);
>> +			goto scrub_continue;
>> +		}
>> +	}
>> +
>>  	spin_lock(&fs_info->trans_lock);
>>  	cur_trans->state = TRANS_STATE_UNBLOCKED;
>>  	fs_info->running_transaction = NULL;
>>