On 2018/11/22 下午9:12, David Sterba wrote: > 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. >
I'd like to add reviewed-by tab, but I'm still not 100% if this will cause extra qgroup reserved space related problem. At least from my side, I can't directly see a case where it will cause problem. Does such case mean a reviewed-by tag? Or something LGTM-but-uncertain? Thanks, Qu
signature.asc
Description: OpenPGP digital signature
