Re: [PATCH] test 305230142ae0

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

On 2023/11/11 18:43, Edward Adam Davis wrote:
> On Sat, 11 Nov 2023 06:20:59 +0000, Matthew Wilcox wrote:
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
>>>   		goto out;
>>>   	}
>>>
>>> -	*objectid = root->free_objectid++;
>>> +	while (find_qgroup_rb(root->fs_info, root->free_objectid++));
>>> +	*objectid = root->free_objectid;
>>
>> This looks buggy to me.  Let's say that free_objectid is currently 3.
>>
>> Before, it would assign 3 to *objectid, and increment free_objectid to
>> 4.  After (assuming the loop terminates on first iteration), it will
>> increment free_objectid to 4, then assign 4 to *objectid.
>>
>> I think you meant to write:
>>
>> 	while (find_qgroup_rb(root->fs_info, root->free_objectid))
>> 		root->free_objectid++;
>> 	*objectid = root->free_objectid++;
> Yes, your guess is correct.
>>
>> And the lesson here is that more compact code is not necessarily more
>> correct code.
>>
>> (I'm not making any judgement about whether this is the correct fix;
>> I don't understand btrfs well enough to have an opinion.  Just that
>> this is not an equivalent transformation)
> I don't have much knowledge about btrfs too, but one thing is clear: the qgroupid
> taken by create_snapshot() is calculated from btrfs_get_free_ojectid().
> At the same time, when calculating the new value in btrfs_get_free_ojectid(),
> it is clearly unreasonable to not determine whether the new value exists in the
> qgroup_tree tree.

Nope, it's totally wrong.

Qgroupid is bound to subvolumeid, thus getting a different id for
qgroupid is going to screw the whole thing up.

> Perhaps there are other methods to obtain a new qgroupid, but before obtaining
> a new value, it is necessary to perform a duplicate value judgment on qgroup_tree,
> otherwise similar problems may still occur.

If you don't really understand the context, the fix is never going to be
correct.

Thanks,
Qu

>
> edward
>
>