On Tue, Nov 20, 2018 at 08:32:42AM +0800, Qu Wenruo wrote: > >>> @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info > >>> *fs_info) > >>> btrfs_abort_transaction(trans, ret); > >>> goto out_free_path; > >>> } > >>> - spin_lock(&fs_info->qgroup_lock); > >>> - fs_info->quota_root = quota_root; > >>> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags); > >>> - spin_unlock(&fs_info->qgroup_lock); > >>> > >>> ret = btrfs_commit_transaction(trans); > >>> trans = NULL; > >>> if (ret) > >>> goto out_free_path; > >> > >> The main concern here is, if we don't set qgroup enabled bit before we > >> commit transaction, there will be a window where new tree modification > >> could happen before QGROUP_ENABLED bit set. > > > > That doesn't seem to make much sense to me, if I understood correctly. > > Essentially you're saying stuff done to any tree in the the > > transaction we use to > > enable quotas must be accounted for. In that case the quota enabled bit > > should > > be done as soon as the transaction is started, because before we set > > it and after > > we started (or joined) a transaction, a lot could of modifications may > > have happened. > > Nevertheless I don't think it's unexpected for anyone to have the > > accounting happening > > only after the quota enable ioctl returns success. > > The problem is not accounting, the qgroup number won't cause problem. > > It's the reserved space. Like some dirty pages are dirtied before quota > enabled, but at run_dealloc() time quota is enabled. > > For such case we have io_tree based method to avoid underflow so it > should be OK. > > So v2 patch looks OK.
Does that mean reviewed-by? In case there's a evolved discussion under a patch, a clear yes/no is appreciated and an explicit Reviewed-by even more. I'm about to add this patch to rc4 pull, thre's still some time to add the tag. Thanks.
