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
