Re: [PATCH] test 305230142ae0

[Qu Wenruo]

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