Re: [PATCH 0/3] btrfs: fixes for relocation to avoid KASAN reports

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4417 bytes --]



On 2020/1/4 下午5:37, Qu Wenruo wrote:
> 
> 
> On 2020/1/4 上午12:15, David Sterba wrote:
>> On Fri, Jan 03, 2020 at 04:52:59PM +0100, David Sterba wrote:
>>> So it's one bit vs refcount and a lock. For the backports I'd go with
>>> the bit, but this needs the barriers as mentioned in my previous reply.
>>> Can you please update the patches?
>>
>> The idea is in the diff below (compile tested only). I found one more
>> case that was not addressed by your patches, it's in
>> btrfs_update_reloc_root.
> 
> But fix in btrfs_update_reloc_root() is already included in commit
> d2311e698578 ("btrfs: relocation: Delay reloc tree deletion after
> merge_reloc_roots").
> 
> Or would you mind to share more details about the missing check?
>>
>> Given that the type of the fix is the same, I'd rather do that in one
>> patch. The reported stack traces are more or less the same.
> 
> To merge them into patch set is no problem, and should make backports a
> little easier.
> 
> But I still didn't understand the barrier part.
> If we're relying on that bit operation before accessing reloc_root, it
> should be safe enough, even without memory barrier.

My bad, set_bit() and test_bit() themselves doesn't imply memory
barrier. (I always thought the opposite)

I'll put memory barriers in next version.

Thanks,
Qu

> 
> Would you please explain a little more?
> 
> Thanks,
> Qu
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index af4dd49a71c7..aeba3a7506e1 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -517,6 +517,15 @@ static int update_backref_cache(struct btrfs_trans_handle *trans,
>>  	return 1;
>>  }
>>  
>> +static bool have_reloc_root(struct btrfs_root *root)
>> +{
>> +	smp_mb__before_atomic();
>> +	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>> +		return false;
>> +	if (!root->reloc_root)
>> +		return false;
>> +	return true;
>> +}
>>  
>>  static int should_ignore_root(struct btrfs_root *root)
>>  {
>> @@ -525,9 +534,9 @@ static int should_ignore_root(struct btrfs_root *root)
>>  	if (!test_bit(BTRFS_ROOT_REF_COWS, &root->state))
>>  		return 0;
>>  
>> -	reloc_root = root->reloc_root;
>> -	if (!reloc_root)
>> +	if (!have_reloc_root(root))
>>  		return 0;
>> +	reloc_root = root->reloc_root;
>>  
>>  	if (btrfs_root_last_snapshot(&reloc_root->root_item) ==
>>  	    root->fs_info->running_transaction->transid - 1)
>> @@ -1439,6 +1448,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>>  	 * The subvolume has reloc tree but the swap is finished, no need to
>>  	 * create/update the dead reloc tree
>>  	 */
>> +	smp_mb__before_atomic();
>>  	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>  		return 0;
>>  
>> @@ -1478,8 +1488,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>>  	struct btrfs_root_item *root_item;
>>  	int ret;
>>  
>> -	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state) ||
>> -	    !root->reloc_root)
>> +	if (!have_reloc_root(root))
>>  		goto out;
>>  
>>  	reloc_root = root->reloc_root;
>> @@ -1489,6 +1498,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>>  	if (fs_info->reloc_ctl->merge_reloc_tree &&
>>  	    btrfs_root_refs(root_item) == 0) {
>>  		set_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>> +		smp_mb__after_atomic();
>>  		__del_reloc_root(reloc_root);
>>  	}
>>  
>> @@ -2201,6 +2211,7 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>>  				if (ret2 < 0 && !ret)
>>  					ret = ret2;
>>  			}
>> +			smp_mb__before_atomic();
>>  			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>>  			btrfs_put_fs_root(root);
>>  		} else {
>> @@ -4730,7 +4741,7 @@ void btrfs_reloc_pre_snapshot(struct btrfs_pending_snapshot *pending,
>>  	struct btrfs_root *root = pending->root;
>>  	struct reloc_control *rc = root->fs_info->reloc_ctl;
>>  
>> -	if (!root->reloc_root || !rc)
>> +	if (!rc || !have_reloc_root(root))
>>  		return;
>>  
>>  	if (!rc->merge_reloc_tree)
>> @@ -4764,7 +4775,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>>  	struct reloc_control *rc = root->fs_info->reloc_ctl;
>>  	int ret;
>>  
>> -	if (!root->reloc_root || !rc)
>> +	if (!rc || !have_reloc_root(root))
>>  		return 0;
>>  
>>  	rc = root->fs_info->reloc_ctl;
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]