diff mbox series

[2/2] btrfs: qgroup: catch reserved space leakage at unmount time

Message ID 20200607072512.31721-3-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: qgroup: detect and fix leaked data reserved space | expand

Commit Message

Qu Wenruo June 7, 2020, 7:25 a.m. UTC
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c |  6 ++++++
 fs/btrfs/qgroup.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/qgroup.h  |  2 +-
 3 files changed, 50 insertions(+), 1 deletion(-)

Comments

Nikolay Borisov June 8, 2020, 6:58 a.m. UTC | #1
On 7.06.20 г. 10:25 ч., Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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.

> +	}

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 ?
> +
>  	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.

> +	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
>
Michał Mirosław June 8, 2020, 7:20 a.m. UTC | #2
On Sun, Jun 07, 2020 at 03:25:12PM +0800, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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");
> +	}

This looks like debugging aid, so:

if (IS_ENABLED(CONFIG_BTRFS_DEBUG))
	btrfs_check_qgroup_leak(fs_info);

would be more readable (WARN() pushed to the function).

Best Regards,
Michał Mirosław
Qu Wenruo June 8, 2020, 7:22 a.m. UTC | #3
On 2020/6/8 下午2:58, Nikolay Borisov wrote:
>
>
> On 7.06.20 г. 10:25 ч., Qu Wenruo wrote:
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  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
>>
Qu Wenruo June 8, 2020, 7:24 a.m. UTC | #4
On 2020/6/8 下午3:20, Michał Mirosław wrote:
> On Sun, Jun 07, 2020 at 03:25:12PM +0800, Qu Wenruo wrote:
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  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");
>> +	}
> 
> This looks like debugging aid, so:
> 
> if (IS_ENABLED(CONFIG_BTRFS_DEBUG))
> 	btrfs_check_qgroup_leak(fs_info);
> 
> would be more readable (WARN() pushed to the function).

We want to check to be executed even on production system, but just less
noisy (no kernel backtrace dump).
Just like tree-checker and EXTENT_QUOTA_RESERVED check.

Thanks,
Qu

> 
> Best Regards,
> Michał Mirosław
>
Michał Mirosław June 8, 2020, 7:44 a.m. UTC | #5
On Mon, Jun 08, 2020 at 03:24:10PM +0800, Qu Wenruo wrote:
> On 2020/6/8 下午3:20, Michał Mirosław wrote:
> > On Sun, Jun 07, 2020 at 03:25:12PM +0800, Qu Wenruo wrote:
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  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");
> >> +	}
> > 
> > This looks like debugging aid, so:
> > 
> > if (IS_ENABLED(CONFIG_BTRFS_DEBUG))
> > 	btrfs_check_qgroup_leak(fs_info);
> > 
> > would be more readable (WARN() pushed to the function).
> 
> We want to check to be executed even on production system, but just less
> noisy (no kernel backtrace dump).
> Just like tree-checker and EXTENT_QUOTA_RESERVED check.

In that case I suggest:

btrfs_err(...);
WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));

as I expect people look for messages before the Oops for more information.

Best Regards,
Michał Mirosław
Qu Wenruo June 8, 2020, 9:37 a.m. UTC | #6
On 2020/6/8 下午3:44, Michał Mirosław wrote:
> On Mon, Jun 08, 2020 at 03:24:10PM +0800, Qu Wenruo wrote:
>> On 2020/6/8 下午3:20, Michał Mirosław wrote:
>>> On Sun, Jun 07, 2020 at 03:25:12PM +0800, Qu Wenruo wrote:
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  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");
>>>> +	}
>>>
>>> This looks like debugging aid, so:
>>>
>>> if (IS_ENABLED(CONFIG_BTRFS_DEBUG))
>>> 	btrfs_check_qgroup_leak(fs_info);
>>>
>>> would be more readable (WARN() pushed to the function).
>>
>> We want to check to be executed even on production system, but just less
>> noisy (no kernel backtrace dump).
>> Just like tree-checker and EXTENT_QUOTA_RESERVED check.
> 
> In that case I suggest:
> 
> btrfs_err(...);
> WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> 
> as I expect people look for messages before the Oops for more information.

Yep, exactly what Nik suggested and what I would do in next version.

Thanks,
Qu
> 
> Best Regards,
> Michał Mirosław
>
David Sterba June 9, 2020, 6:46 p.m. UTC | #7
On Sun, Jun 07, 2020 at 03:25:12PM +0800, Qu Wenruo wrote:

Please write some changelog.

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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");

No newline in the btrfs_err strings.

> +	}
> +
>  	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));
> +}

Missing newline.

> +/*
> + * 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)

I think we've been naming such functions with 'check' eg.
btrfs_check_quota_leak, and return true/false.

> +{
> +	struct btrfs_qgroup *qgroup;
> +	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",

If this could be potentially noisy, the ratelimited version would be
more suitable.

The message wording sounds ok, as it points to the qgroups and there's
one global message printed from close_ctree that says it's a 'leak'.

> +				   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);

So when I'm pointing out newlines, please keep one between the text and
the last #endif.

Thanks.

>  #endif
> -- 
> 2.26.2
diff mbox series

Patch

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");
+	}
+
 	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;
+	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