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