On 2018/07/27 10:19, Qu Wenruo wrote: > > > On 2018年07月27日 09:10, Misono Tomohiro wrote: >> On 2018/07/26 18:15, Qu Wenruo wrote: >>> Between btrfs_quota_enable() finished and rescan kicked in, there is a >>> small window that quota status has (ON | INCONSISTENT) bits set but >>> without RESCAN bits set. >>> >>> And transaction is committed inside the window and then power loss >>> happens, we will have a quota tree with all qgroup numbers set to 0, and >>> not RESCAN bit set. >>> >>> At next mount time, qgroup rescan will not kick in due to the missing of >>> RESCAN bit, user needs to kick in rescan manually. >>> >>> This patch will fix it by setting RESCAN bit at btrfs_quota_enable(), >>> so even after power loss we will still kick in rescan automatically. >>> >>> Suggested-by: Misono Tomohiro <misono.tomoh...@jp.fujitsu.com> >>> Signed-off-by: Qu Wenruo <w...@suse.com> >>> --- >>> fs/btrfs/qgroup.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >>> index c25dc47210a3..13c1c7dd278d 100644 >>> --- a/fs/btrfs/qgroup.c >>> +++ b/fs/btrfs/qgroup.c >>> @@ -930,7 +930,8 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans, >>> btrfs_set_qgroup_status_generation(leaf, ptr, trans->transid); >>> btrfs_set_qgroup_status_version(leaf, ptr, BTRFS_QGROUP_STATUS_VERSION); >>> fs_info->qgroup_flags = BTRFS_QGROUP_STATUS_FLAG_ON | >>> - BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT; >>> + BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT | >>> + BTRFS_QGROUP_STATUS_FLAG_RESCAN; >>> btrfs_set_qgroup_status_flags(leaf, ptr, fs_info->qgroup_flags); >>> btrfs_set_qgroup_status_rescan(leaf, ptr, 0); >>> >>> @@ -987,7 +988,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans, >>> fs_info->quota_root = quota_root; >>> set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags); >>> spin_unlock(&fs_info->qgroup_lock); >>> - ret = qgroup_rescan_init(fs_info, 0, 1); >>> + ret = qgroup_rescan_init(fs_info, 0, 0); >>> if (!ret) { >>> qgroup_rescan_zero_tracking(fs_info); >>> btrfs_queue_work(fs_info->qgroup_rescan_workers, >>> >> >> This is what I think at first, but is it ok not holding >> fs_info->qgroup_ioctl_lock >> in brfs_qgroup_rescan() as you concerned in previous thread? > > I think it's OK, since we have larger mutex (subvol_sem) for > quota_enable/disable() so there will be no concurrency modifying flags. > And we're holding trans handler from btrfs_ioctl_quota_ctl(), > transaction won't be committed in btrfs_quota_enable().
Ok, but nikolay's patch in misc-next moves transaction commit in btrfs_quota_enable(): https://patchwork.kernel.org/patch/10508819/ ("btrfs: qgroups: Move transaction management inside btrfs_quota_enable/disable") This is related to https://marc.info/?l=linux-btrfs&m=152999289017582. However, it seems that other people does not see the problem, so I'm not sure how the above patch ends up... Thanks, Tomohiro Misono > > So I think it's OK. > > Thanks, > Qu > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html