Re: [PATCH v2] btrfs-progs: free-space-cache: Enhance free space cache free space check

[Nikolay Borisov <nborisov@suse.com>]

On  9.03.2018 01:27, Qu Wenruo wrote:
> 
> 
> On 2018年03月08日 22:05, Nikolay Borisov wrote:
>>
>>
>> On  8.03.2018 09:02, Qu Wenruo wrote:
>>> When we found free space difference between free space cache and block
>>> group item, we just discard this free space cache.
>>>
>>> Normally such difference is caused by btrfs_reserve_extent() called by
>>> delalloc which is out of a transaction.
>>> And since all btrfs_release_extent() is called with a transaction, under
>>> heavy race free space cache can have less free space than block group
>>> item.
>>>
>>> Normally kernel will detect such difference and just discard that cache.
>>>
>>> However we must be more careful if free space cache has more free space
>>> cache, and if that happens, paried with above race one invalid free
>>> space cache can be loaded into kernel.
>>>
>>> So if we find any free space cache who has more free space then block
>>> group item, we report it as an error other than ignoring it.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>> v2:
>>>   Fix the timming of free space output.
>>> ---
>>>  check/main.c       |  4 +++-
>>>  free-space-cache.c | 32 ++++++++++++++++++++++++--------
>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index 97baae583f04..bc31f7e32061 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -5339,7 +5339,9 @@ static int check_space_cache(struct btrfs_root *root)
>>>  			error += ret;
>>>  		} else {
>>>  			ret = load_free_space_cache(root->fs_info, cache);
>>> -			if (!ret)
>>> +			if (ret < 0)
>>> +				error++;
>>> +			if (ret <= 0)
>>>  				continue;
>>>  		}
>>>  
>>> diff --git a/free-space-cache.c b/free-space-cache.c
>>> index f933f9f1cf3f..9b83a71ca59a 100644
>>> --- a/free-space-cache.c
>>> +++ b/free-space-cache.c
>>> @@ -438,7 +438,8 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,
>>>  	struct btrfs_path *path;
>>>  	u64 used = btrfs_block_group_used(&block_group->item);
>>>  	int ret = 0;
>>> -	int matched;
>>> +	u64 bg_free;
>>> +	s64 diff;
>>>  
>>>  	path = btrfs_alloc_path();
>>>  	if (!path)
>>> @@ -448,18 +449,33 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,
>>>  				      block_group->key.objectid);
>>>  	btrfs_free_path(path);
>>>  
>>> -	matched = (ctl->free_space == (block_group->key.offset - used -
>>> -				       block_group->bytes_super));
>>> -	if (ret == 1 && !matched) {
>>> -		__btrfs_remove_free_space_cache(ctl);
>>> +	bg_free = block_group->key.offset - used - block_group->bytes_super;
>>> +	diff = ctl->free_space - bg_free;
>>> +	if (ret == 1 && diff) {
>>>  		fprintf(stderr,
>>> -		       "block group %llu has wrong amount of free space\n",
>>> -		       block_group->key.objectid);
>>> +		       "block group %llu has wrong amount of free space, free space cache has %llu block group has %llu\n",nit: Always put units when printing numbers. In this case we are talking
>> about bytes.
>>> +		       block_group->key.objectid, ctl->free_space, bg_free);
>>> +		__btrfs_remove_free_space_cache(ctl);
>>> +		/*
>>> +		 * Due to btrfs_reserve_extent() can happen out of > +		 * transaction, but all btrfs_release_extent() happens inside
>>> +		 * a transaction, so under heavy race it's possible that free
>>> +		 * space cache has less free space, and both kernel just discard
>>> +		 * such cache. But if we find some case where free space cache
>>> +		 * has more free space, this means under certain case such
>>> +		 * cache can be loaded and cause double allocate.
>>> +		 *
>>> +		 * Detect such possibility here.
>>> +		 */
>>> +		if (diff > 0)
>>> +			error(
>>> +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers");
>>
>> I'm not entirely happy with this message. So they will post to the
>> mailing list saying something along the lines of "I got this message
>> what do I do no, please help".  Better to output actionable data so that
>> the user can post it immediately.
> 
> Unfortunately, this is already the situation we don't expect to see.
> 
> What we really need is to know this could happen, and if possible some
> info about the situation.
> There is not much actionable data here.

Fair enough, at the very least I think we should put information how to
contact btrfs developers. So put the address of the mailing list, I
don't think it's safe to assume people will be aware of it .

> 
> Thanks,
> Qu
> 
>>
>>>  		ret = -1;
>>>  	}
>>>  
>>>  	if (ret < 0) {
>>> -		ret = 0;
>>> +		if (diff <= 0)
>>> +			ret = 0;
>>>  
>>>  		fprintf(stderr,
>>>  		       "failed to load free space cache for block group %llu\n",
>>>
>