On Wed, Apr 08, 2020 at 08:36:27PM +0800, Qu Wenruo wrote:
> Forgot to mention, although this doesn't cause any data corruption, it
> breaks snapper, which has some kind of "space aware cleaner algorithm",
> which put all newly created snapshots into 1/0, but not the current root
> subvolume.
>
> And since snapper uses snapshot ioctl to assign qgroup relationship
> directly, without using qgrou assign ioctl, it has no way to detect such
> problem.
>
> Hopes we can get this patch into current release cycle.
It's still time to get it to 5.8, with CC: stable it could get
propagated to other versions. For 5.7 it's not clear at this point as
the bug does not seem to be urgent and as far as I understand it,
there's a workaround.
Also with an application (snapper) using some semantics of the ioctls,
we need to actually test it with patched and unpatched kernel, or maybe
snapper needs some fixup first.
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -2622,6 +2622,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> > struct btrfs_root *quota_root;
> > struct btrfs_qgroup *srcgroup;
> > struct btrfs_qgroup *dstgroup;
> > + bool need_rescan = false;
> > u32 level_size = 0;
> > u64 nums;
> >
> > @@ -2765,6 +2766,13 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> > goto unlock;
> > }
> > ++i_qgroups;
> > +
> > + /*
> > + * If we're doing a snapshot, and adding the snapshot to a new
> > + * qgroup, the numbers are guaranteed to be incorrect.
> > + */
> > + if (srcid)
> > + need_rescan = true;
> > }
> >
> > for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) {
> > @@ -2784,6 +2792,9 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >
> > dst->rfer = src->rfer - level_size;
> > dst->rfer_cmpr = src->rfer_cmpr - level_size;
> > +
> > + /* Manually tweaking numbers? No way to keep qgroup sane */
> > + need_rescan = true;
> > }
> > for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) {
> > struct btrfs_qgroup *src;
> > @@ -2802,6 +2813,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> >
> > dst->excl = src->excl + level_size;
> > dst->excl_cmpr = src->excl_cmpr + level_size;
> > + need_rescan = true;
> > }
> >
> > unlock:
> > @@ -2809,6 +2821,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> > out:
> > if (!committing)
> > mutex_unlock(&fs_info->qgroup_ioctl_lock);
> > + if (need_rescan)
> > + fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
This got me curious, a non-atomic change to qgroup_flags and without
any protection. The function is not running in a safe context (like
quota enable or disable) so lack of synchronization seems suspicious. I
grepped for other changes to the qgroup_flags and it's very
inconsistent. Sometimes it's the fs_info::qgroup_lock, no lokcing at all
or no obvious lock but likely fs_info::qgroup_ioctl_lock or
qgroup_rescan_lock.
I was considering using atomic bit updates but that would be another
patch.
