On 2020/5/13 下午10:01, David Sterba wrote:
> On Wed, May 13, 2020 at 02:16:11PM +0800, Qu Wenruo wrote:
>> -	if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID) {
>> +	if (root->root_key.objectid != BTRFS_TREE_LOG_OBJECTID &&
>> +	    root->root_key.objectid != BTRFS_DATA_RELOC_TREE_OBJECTID) {
> 
>>  	if (test_bit(BTRFS_ROOT_SHAREABLE, &root->state) ||
>> -	    root == fs_info->tree_root)
>> +	    root == fs_info->tree_root || root == fs_info->dreloc_root)
> 
>>  		if (found_extent &&
>>  		    (test_bit(BTRFS_ROOT_SHAREABLE, &root->state) ||
>> -		     root == fs_info->tree_root)) {
>> +		     root == fs_info->tree_root ||
>> +		     root == fs_info->dreloc_root)) {
> 
> Lots of repeated conditions, though not all of them exactly the same. I
> was thinking about adding some helpers but don't have a good suggestion.

The good news is, only inode truncating cares that much.
All other locations won't bother at all.

The concern here is, INODE_ITEM can exist in trees without SHAREABLE
(REF_COWS).
The original exception is root tree (v1 space cache uses INODE_ITEM).

As we don't set SHAREABLE flag for data reloc tree now, it's adding the
exception.

I could find a way to make it more readable, by separating the regular
SHAREABLE check and two exceptions and wrap it into a function.

Thanks,
Qu

> 
> The concern is about too much special casing, it's eg tree_root +
> data_reloc_tree, or SHAREABLE + tree_reloc + data_reloc, etc. The
> helpers should capture the common semantics of the condition and also
> reduce the surface for future errors.
>