Re: [PATCH] btrfs: fix error handling in commit_fs_roots

[Josef Bacik <josef@toxicpanda.com>]

On 12/4/20 11:52 AM, David Sterba wrote:
> On Tue, Dec 01, 2020 at 09:53:23AM -0500, Josef Bacik wrote:
>> While doing error injection I would sometimes get a corrupt file system.
>> This is because I was injecting errors at btrfs_search_slot, but would
>> only do it one time per stack.  This uncovered a problem in
>> commit_fs_roots, where if we get an error we would just break.  However
>> we're in a second loop, the first loop being a loop to find all the
>> dirty fs roots, and then subsequent root updates would succeed clearing
>> the error value.
>>
>> This isn't likely to happen in real scenarios, however we could
>> potentially get a random ENOMEM once and then not again, and we'd end up
>> with a corrupted file system.  Fix this by moving the error checking
>> around a bit to the main loop, as this is the only place where something
>> will fail, and return the error as soon as it occurs.
>>
>> With this patch my reproducer no longer corrupts the file system.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/transaction.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 1dac76b7ea96..b05f75654b16 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1328,7 +1328,6 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>>   	struct btrfs_root *gang[8];
>>   	int i;
>>   	int ret;
>> -	int err = 0;
>>   
>>   	spin_lock(&fs_info->fs_roots_radix_lock);
>>   	while (1) {
>> @@ -1340,6 +1339,8 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>>   			break;
>>   		for (i = 0; i < ret; i++) {
>>   			struct btrfs_root *root = gang[i];
>> +			int err;
> 
> I'd rather get rid of 'err' for the return values, in this case we can
> reuse 'ret'.
> 

Sure, I'll fix and respin.

>> +
>>   			radix_tree_tag_clear(&fs_info->fs_roots_radix,
>>   					(unsigned long)root->root_key.objectid,
>>   					BTRFS_ROOT_TRANS_TAG);
>> @@ -1366,14 +1367,14 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>>   			err = btrfs_update_root(trans, fs_info->tree_root,
>>   						&root->root_key,
>>   						&root->root_item);
>> -			spin_lock(&fs_info->fs_roots_radix_lock);
>>   			if (err)
>> -				break;
>> +				return err;
>> +			spin_lock(&fs_info->fs_roots_radix_lock);
>>   			btrfs_qgroup_free_meta_all_pertrans(root);
> 
> Do we need to call btrfs_qgroup_free_meta_all_pertrans before returning?
> 

It doesn't look like it, and we'd miss any existing roots if we did.  It doesn't 
appear that any qgroup accounting gets cleaned up in the case of an error, so it 
must work out already?  If not then we need to address that separately, because 
if we're relying on the cleanup to happen here we'll mess up if the error 
happens before we even get to commit_fs_roots.  Thanks,

Josef