On 2020/6/8 下午2:58, Nikolay Borisov wrote:
>
>
> On 7.06.20 г. 10:25 ч., Qu Wenruo wrote:
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> fs/btrfs/disk-io.c | 6 ++++++
>> fs/btrfs/qgroup.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> fs/btrfs/qgroup.h | 2 +-
>> 3 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index f8ec2d8606fd..48d047e64461 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -4058,6 +4058,12 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
>> ASSERT(list_empty(&fs_info->delayed_iputs));
>> set_bit(BTRFS_FS_CLOSING_DONE, &fs_info->flags);
>>
>> + if (btrfs_qgroup_has_leak(fs_info)) {
>> + WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
>> + KERN_ERR "BTRFS: qgroup reserved space leaked\n");
>> + btrfs_err(fs_info, "qgroup reserved space leaked\n");
> I don't think the message from the WARN() brings any value, it's simply
> duplicated by the btrfs_err. IMO it's more concise to do:
>
> WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> btrfs_err(qgroup reserved space leaked);
>
> without losing any information whatsoever.
Makes sense, also another cleanup item for existing similar cases.
>
>> + }
>
> Is it safe calling this code here? workqueues are being destroyed after
> it in btrfs_stop_all_workers so it's possible that they have some
> lingering work which in turn might cause false positive in this check ?
The safety here is as safe as other calls in close_ctree(), we expect no
new trans started nor existing running trans (finish ordered io call),
and no dirty pages (invalidate/release page call)
So at this timing there should be nothing to modify qgroup and we're safe.
Or did I miss something for the close_ctree() context?
>> +
>> btrfs_free_qgroup_config(fs_info);
>> ASSERT(list_empty(&fs_info->delalloc_roots));
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 5bd4089ad0e1..3fccf2ffdcf1 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -505,6 +505,49 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
>> return ret < 0 ? ret : 0;
>> }
>>
>> +static u64 btrfs_qgroup_subvolid(u64 qgroupid)
>> +{
>> + return (qgroupid & ((1ULL << BTRFS_QGROUP_LEVEL_SHIFT) - 1));
>> +}
>> +/*
>> + * Get called for close_ctree() when quota is still enabled.
>> + * This verifies we don't leak some reserved space.
>> + *
>> + * Return false if no reserved space is left.
>> + * Return true if some reserved space is leaked.
>> + */
>> +bool btrfs_qgroup_has_leak(struct btrfs_fs_info *fs_info)
>> +{
>> + struct btrfs_qgroup *qgroup;
>
> nit:This variable is used only in the loop below just define it there to
> reduce its scope.
Sure.
Thanks,
Qu
>
>> + struct rb_node *node;
>> + bool ret = false;
>> +
>> + if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
>> + return ret;
>> + /*
>> + * Since we're unmounting, there is no race and no need to grab
>> + * qgroup lock.
>> + * And here we don't go post order to provide a more user friendly
>> + * sorted result.
>> + */
>> + for (node = rb_first(&fs_info->qgroup_tree); node; node = rb_next(node)) {
>> + int i;
>> +
>> + qgroup = rb_entry(node, struct btrfs_qgroup, node);
>> + for (i = 0; i < BTRFS_QGROUP_RSV_LAST; i++) {
>> + if (qgroup->rsv.values[i]) {
>> + ret = true;
>> + btrfs_warn(fs_info,
>> + "qgroup %llu/%llu has unreleased space, type=%d rsv=%llu",
>> + btrfs_qgroup_level(qgroup->qgroupid),
>> + btrfs_qgroup_subvolid(qgroup->qgroupid),
>> + i, qgroup->rsv.values[i]);
>> + }
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> /*
>> * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
>> * first two are in single-threaded paths.And for the third one, we have set
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index 1bc654459469..e3e9f9df8320 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -415,5 +415,5 @@ int btrfs_qgroup_add_swapped_blocks(struct btrfs_trans_handle *trans,
>> int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
>> struct btrfs_root *root, struct extent_buffer *eb);
>> void btrfs_qgroup_destroy_extent_records(struct btrfs_transaction *trans);
>> -
>> +bool btrfs_qgroup_has_leak(struct btrfs_fs_info *fs_info);
>> #endif
>>
