Re: [PATCH 2/2] btrfs: Don't set SHAREABLE flag for data reloc tree

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

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



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.
> 


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