[PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[Qu Wenruo]

While the reason why qgroup reserved space may underflow is still under
investigation, such WARN_ON will help us to expose the bug more easily,
and for end-user we can detect and avoid underflow.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8db2e29..8532587 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0)
-		qgroup->reserved -= num_bytes;
+	if (sign > 0) {
+		if (WARN_ON(qgroup->reserved < num_bytes))
+			qgroup->reserved = 0;
+		else
+			qgroup->reserved -= num_bytes;
+	}
 
 	qgroup_dirty(fs_info, qgroup);
 
@@ -1082,8 +1086,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0)
-			qgroup->reserved -= num_bytes;
+		if (sign > 0) {
+			if (WARN_ON(qgroup->reserved < num_bytes))
+				qgroup->reserved = 0;
+			else
+				qgroup->reserved -= num_bytes;
+		}
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 
@@ -2201,7 +2209,10 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 
 		qg = u64_to_ptr(unode->aux);
 
-		qg->reserved -= num_bytes;
+		if (WARN_ON(qgroup->reserved < num_bytes))
+			qgroup->reserved = 0;
+		else
+			qgroup->reserved -= num_bytes;
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,
-- 
2.10.0

[PATCH 2/2] btrfs: Add trace point for qgroup reserved space

[Qu Wenruo]

Introduce the following trace points:
qgroup_update_reserve
qgroup_meta_reserve

And modify the timing of btrfs_qgroup_free_delayed_ref() and
btrfs_qgroup_release_data() events, to work with qgroup_update_reserve()
event.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c            | 21 ++++++++++++++++++---
 fs/btrfs/qgroup.h            |  2 +-
 include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8532587..593a519 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1062,6 +1062,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
 	if (sign > 0) {
+		trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
+					    qgroup->reserved, (s64)-num_bytes);
 		if (WARN_ON(qgroup->reserved < num_bytes))
 			qgroup->reserved = 0;
 		else
@@ -1087,6 +1089,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
 		if (sign > 0) {
+			trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
+						    qgroup->reserved,
+						    (s64)-num_bytes);
 			if (WARN_ON(qgroup->reserved < num_bytes))
 				qgroup->reserved = 0;
 			else
@@ -2164,6 +2169,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes)
 
 		qg = u64_to_ptr(unode->aux);
 
+		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
+					    qg->reserved, num_bytes);
 		qg->reserved += num_bytes;
 	}
 
@@ -2209,6 +2216,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 
 		qg = u64_to_ptr(unode->aux);
 
+		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
+					    qg->reserved, (s64)-num_bytes);
 		if (WARN_ON(qgroup->reserved < num_bytes))
 			qgroup->reserved = 0;
 		else
@@ -2637,12 +2646,12 @@ static int __btrfs_qgroup_release_data(struct inode *inode, u64 start, u64 len,
 	if (ret < 0)
 		goto out;
 
-	if (free) {
-		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
+	if (free)
 		trace_op = QGROUP_FREE;
-	}
 	trace_btrfs_qgroup_release_data(inode, start, len,
 					changeset.bytes_changed, trace_op);
+	if (free)
+		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
 out:
 	ulist_free(changeset.range_changed);
 	return ret;
@@ -2692,6 +2701,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes)
 		return 0;
 
 	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)num_bytes);
 	ret = qgroup_reserve(root, num_bytes);
 	if (ret < 0)
 		return ret;
@@ -2709,6 +2720,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	reserved = atomic_xchg(&root->qgroup_meta_rsv, 0);
 	if (reserved == 0)
 		return;
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)-reserved);
 	qgroup_free(root, reserved);
 }
 
@@ -2720,6 +2733,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
 	WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic_sub(num_bytes, &root->qgroup_meta_rsv);
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)-num_bytes);
 	qgroup_free(root, num_bytes);
 }
 
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 1bc64c8..6b6756c 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -114,8 +114,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
 }
 void assert_qgroups_uptodate(struct btrfs_trans_handle *trans);
 
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index e030d6f..fb3cb6c 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -1468,6 +1468,49 @@ TRACE_EVENT(qgroup_update_counters,
 		  __entry->cur_new_count)
 );
 
+TRACE_EVENT(qgroup_update_reserve,
+
+	TP_PROTO(struct btrfs_fs_info *fs_info, u64 qgid, u64 cur_reserved,
+		 s64 diff),
+
+	TP_ARGS(fs_info, qgid, cur_reserved, diff),
+
+	TP_STRUCT__entry_btrfs(
+		__field(	u64,	qgid			)
+		__field(	u64,	cur_reserved		)
+		__field(	s64,	diff			)
+	),
+
+	TP_fast_assign_btrfs(fs_info,
+		__entry->qgid		= qgid;
+		__entry->cur_reserved	= cur_reserved;
+		__entry->diff		= diff;
+	),
+
+	TP_printk_btrfs("qgid = %llu, cur_reserved = %llu, diff = %lld",
+		__entry->qgid, __entry->cur_reserved, __entry->diff)
+);
+
+TRACE_EVENT(qgroup_meta_reserve,
+
+	TP_PROTO(struct btrfs_fs_info *fs_info, u64 refroot, s64 diff),
+
+	TP_ARGS(fs_info, refroot, diff),
+
+	TP_STRUCT__entry_btrfs(
+		__field(	u64,	refroot			)
+		__field(	u64,	diff			)
+	),
+
+	TP_fast_assign_btrfs(fs_info,
+		__entry->refroot	= refroot;
+		__entry->diff		= diff;
+	),
+
+	TP_printk_btrfs("refroot = %llu, diff = %lld",
+		__entry->refroot, __entry->diff)
+);
+
 #endif /* _TRACE_BTRFS_H */
 
 /* This part must be outside protection */
-- 
2.10.0

Re: [PATCH 2/2] btrfs: Add trace point for qgroup reserved space

[Goldwyn Rodrigues]

On 09/29/2016 08:15 PM, Qu Wenruo wrote:
> Introduce the following trace points:
> qgroup_update_reserve
> qgroup_meta_reserve
> 
> And modify the timing of btrfs_qgroup_free_delayed_ref() and
> btrfs_qgroup_release_data() events, to work with qgroup_update_reserve()
> event.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>


> ---
>  fs/btrfs/qgroup.c            | 21 ++++++++++++++++++---
>  fs/btrfs/qgroup.h            |  2 +-
>  include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8532587..593a519 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1062,6 +1062,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	qgroup->excl += sign * num_bytes;
>  	qgroup->excl_cmpr += sign * num_bytes;
>  	if (sign > 0) {
> +		trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
> +					    qgroup->reserved, (s64)-num_bytes);
>  		if (WARN_ON(qgroup->reserved < num_bytes))
>  			qgroup->reserved = 0;
>  		else
> @@ -1087,6 +1089,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  		qgroup->excl += sign * num_bytes;
>  		if (sign > 0) {
> +			trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
> +						    qgroup->reserved,
> +						    (s64)-num_bytes);
>  			if (WARN_ON(qgroup->reserved < num_bytes))
>  				qgroup->reserved = 0;
>  			else
> @@ -2164,6 +2169,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes)
>  
>  		qg = u64_to_ptr(unode->aux);
>  
> +		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
> +					    qg->reserved, num_bytes);
>  		qg->reserved += num_bytes;
>  	}
>  
> @@ -2209,6 +2216,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>  
>  		qg = u64_to_ptr(unode->aux);
>  
> +		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
> +					    qg->reserved, (s64)-num_bytes);
>  		if (WARN_ON(qgroup->reserved < num_bytes))
>  			qgroup->reserved = 0;
>  		else
> @@ -2637,12 +2646,12 @@ static int __btrfs_qgroup_release_data(struct inode *inode, u64 start, u64 len,
>  	if (ret < 0)
>  		goto out;
>  
> -	if (free) {
> -		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
> +	if (free)
>  		trace_op = QGROUP_FREE;
> -	}
>  	trace_btrfs_qgroup_release_data(inode, start, len,
>  					changeset.bytes_changed, trace_op);
> +	if (free)
> +		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
>  out:
>  	ulist_free(changeset.range_changed);
>  	return ret;
> @@ -2692,6 +2701,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes)
>  		return 0;
>  
>  	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
> +	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
> +				  (s64)num_bytes);
>  	ret = qgroup_reserve(root, num_bytes);
>  	if (ret < 0)
>  		return ret;
> @@ -2709,6 +2720,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>  	reserved = atomic_xchg(&root->qgroup_meta_rsv, 0);
>  	if (reserved == 0)
>  		return;
> +	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
> +				  (s64)-reserved);
>  	qgroup_free(root, reserved);
>  }
>  
> @@ -2720,6 +2733,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>  	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
>  	WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes);
>  	atomic_sub(num_bytes, &root->qgroup_meta_rsv);
> +	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
> +				  (s64)-num_bytes);
>  	qgroup_free(root, num_bytes);
>  }
>  
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index 1bc64c8..6b6756c 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -114,8 +114,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>  						 u64 ref_root, u64 num_bytes)
>  {
> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>  }
>  void assert_qgroups_uptodate(struct btrfs_trans_handle *trans);
>  
> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
> index e030d6f..fb3cb6c 100644
> --- a/include/trace/events/btrfs.h
> +++ b/include/trace/events/btrfs.h
> @@ -1468,6 +1468,49 @@ TRACE_EVENT(qgroup_update_counters,
>  		  __entry->cur_new_count)
>  );
>  
> +TRACE_EVENT(qgroup_update_reserve,
> +
> +	TP_PROTO(struct btrfs_fs_info *fs_info, u64 qgid, u64 cur_reserved,
> +		 s64 diff),
> +
> +	TP_ARGS(fs_info, qgid, cur_reserved, diff),
> +
> +	TP_STRUCT__entry_btrfs(
> +		__field(	u64,	qgid			)
> +		__field(	u64,	cur_reserved		)
> +		__field(	s64,	diff			)
> +	),
> +
> +	TP_fast_assign_btrfs(fs_info,
> +		__entry->qgid		= qgid;
> +		__entry->cur_reserved	= cur_reserved;
> +		__entry->diff		= diff;
> +	),
> +
> +	TP_printk_btrfs("qgid = %llu, cur_reserved = %llu, diff = %lld",
> +		__entry->qgid, __entry->cur_reserved, __entry->diff)
> +);
> +
> +TRACE_EVENT(qgroup_meta_reserve,
> +
> +	TP_PROTO(struct btrfs_fs_info *fs_info, u64 refroot, s64 diff),
> +
> +	TP_ARGS(fs_info, refroot, diff),
> +
> +	TP_STRUCT__entry_btrfs(
> +		__field(	u64,	refroot			)
> +		__field(	u64,	diff			)
> +	),
> +
> +	TP_fast_assign_btrfs(fs_info,
> +		__entry->refroot	= refroot;
> +		__entry->diff		= diff;
> +	),
> +
> +	TP_printk_btrfs("refroot = %llu, diff = %lld",
> +		__entry->refroot, __entry->diff)
> +);
> +
>  #endif /* _TRACE_BTRFS_H */
>  
>  /* This part must be outside protection */
> 

-- 
Goldwyn

Re: [PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[Goldwyn Rodrigues]

On 09/29/2016 08:15 PM, Qu Wenruo wrote:
> While the reason why qgroup reserved space may underflow is still under
> investigation, such WARN_ON will help us to expose the bug more easily,
> and for end-user we can detect and avoid underflow.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Looks good.

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

> ---
>  fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8db2e29..8532587 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  	qgroup->excl += sign * num_bytes;
>  	qgroup->excl_cmpr += sign * num_bytes;
> -	if (sign > 0)
> -		qgroup->reserved -= num_bytes;
> +	if (sign > 0) {
> +		if (WARN_ON(qgroup->reserved < num_bytes))
> +			qgroup->reserved = 0;
> +		else
> +			qgroup->reserved -= num_bytes;
> +	}
>  
>  	qgroup_dirty(fs_info, qgroup);
>  
> @@ -1082,8 +1086,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  		qgroup->rfer_cmpr += sign * num_bytes;
>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  		qgroup->excl += sign * num_bytes;
> -		if (sign > 0)
> -			qgroup->reserved -= num_bytes;
> +		if (sign > 0) {
> +			if (WARN_ON(qgroup->reserved < num_bytes))
> +				qgroup->reserved = 0;
> +			else
> +				qgroup->reserved -= num_bytes;
> +		}
>  		qgroup->excl_cmpr += sign * num_bytes;
>  		qgroup_dirty(fs_info, qgroup);
>  
> @@ -2201,7 +2209,10 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>  
>  		qg = u64_to_ptr(unode->aux);
>  
> -		qg->reserved -= num_bytes;
> +		if (WARN_ON(qgroup->reserved < num_bytes))
> +			qgroup->reserved = 0;
> +		else
> +			qgroup->reserved -= num_bytes;
>  
>  		list_for_each_entry(glist, &qg->groups, next_group) {
>  			ret = ulist_add(fs_info->qgroup_ulist,
> 

-- 
Goldwyn

Re: [PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[David Sterba]

On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote:
> While the reason why qgroup reserved space may underflow is still under
> investigation, such WARN_ON will help us to expose the bug more easily,
> and for end-user we can detect and avoid underflow.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8db2e29..8532587 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  	qgroup->excl += sign * num_bytes;
>  	qgroup->excl_cmpr += sign * num_bytes;
> -	if (sign > 0)
> -		qgroup->reserved -= num_bytes;
> +	if (sign > 0) {
> +		if (WARN_ON(qgroup->reserved < num_bytes))

That's only partially helpful, you should also print the numbers,
ref_root and/or qgroup id.

Re: [PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[Qu Wenruo]

At 10/19/2016 10:31 PM, David Sterba wrote:
> On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote:
>> While the reason why qgroup reserved space may underflow is still under
>> investigation, such WARN_ON will help us to expose the bug more easily,
>> and for end-user we can detect and avoid underflow.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 8db2e29..8532587 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>  	qgroup->excl += sign * num_bytes;
>>  	qgroup->excl_cmpr += sign * num_bytes;
>> -	if (sign > 0)
>> -		qgroup->reserved -= num_bytes;
>> +	if (sign > 0) {
>> +		if (WARN_ON(qgroup->reserved < num_bytes))
>
> That's only partially helpful, you should also print the numbers,
> ref_root and/or qgroup id.
>

I'm OK to add more output.
But normally it's not really help though.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[David Sterba]

On Thu, Oct 20, 2016 at 09:05:34AM +0800, Qu Wenruo wrote:
> 
> 
> At 10/19/2016 10:31 PM, David Sterba wrote:
> > On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote:
> >> While the reason why qgroup reserved space may underflow is still under
> >> investigation, such WARN_ON will help us to expose the bug more easily,
> >> and for end-user we can detect and avoid underflow.
> >>
> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> >> ---
> >>  fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
> >>  1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> >> index 8db2e29..8532587 100644
> >> --- a/fs/btrfs/qgroup.c
> >> +++ b/fs/btrfs/qgroup.c
> >> @@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
> >>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
> >>  	qgroup->excl += sign * num_bytes;
> >>  	qgroup->excl_cmpr += sign * num_bytes;
> >> -	if (sign > 0)
> >> -		qgroup->reserved -= num_bytes;
> >> +	if (sign > 0) {
> >> +		if (WARN_ON(qgroup->reserved < num_bytes))
> >
> > That's only partially helpful, you should also print the numbers,
> > ref_root and/or qgroup id.
> >
> 
> I'm OK to add more output.
> But normally it's not really help though.

So do you need the warning at all? The next patch adds tracepoints, I
assume you turn them on and look for the issues you're debugging. The
warning would otherwise appear for any user. Without the message it's
IME more scary, it's not obvious what happened without looking into the
code.

Re: [PATCH 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[Qu Wenruo]

At 10/20/2016 05:20 PM, David Sterba wrote:
> On Thu, Oct 20, 2016 at 09:05:34AM +0800, Qu Wenruo wrote:
>>
>>
>> At 10/19/2016 10:31 PM, David Sterba wrote:
>>> On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote:
>>>> While the reason why qgroup reserved space may underflow is still under
>>>> investigation, such WARN_ON will help us to expose the bug more easily,
>>>> and for end-user we can detect and avoid underflow.
>>>>
>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>> ---
>>>>  fs/btrfs/qgroup.c | 21 ++++++++++++++++-----
>>>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index 8db2e29..8532587 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -1061,8 +1061,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>>>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>>>  	qgroup->excl += sign * num_bytes;
>>>>  	qgroup->excl_cmpr += sign * num_bytes;
>>>> -	if (sign > 0)
>>>> -		qgroup->reserved -= num_bytes;
>>>> +	if (sign > 0) {
>>>> +		if (WARN_ON(qgroup->reserved < num_bytes))
>>>
>>> That's only partially helpful, you should also print the numbers,
>>> ref_root and/or qgroup id.
>>>
>>
>> I'm OK to add more output.
>> But normally it's not really help though.
>
> So do you need the warning at all? The next patch adds tracepoints, I
> assume you turn them on and look for the issues you're debugging. The
> warning would otherwise appear for any user. Without the message it's
> IME more scary, it's not obvious what happened without looking into the
> code.
>

The WARN_ON() is needed to let end-user know there is something wrong 
and encourage them to report the bug.

And, other than trace events, normally I just check if there is any 
warning in kernel dmesg to do a quick check if the fix is OK or not.
(trace events is never simpler than plain dmesg)

So personally speaking, WARN_ON() is always needed and obvious enough 
for both developer and end-user.

But that's just my personal preference.
More data on fsid and qgroupid after scary kernel WARN_ON() won't hurt 
anyway.
Especially fsid/qgroupid is useful for multi-btrfs/subvolume system.

Thanks,
Qu

[PATCH v2 1/2] btrfs: Add WARN_ON for qgroup reserved underflow

[Qu Wenruo]

Goldwyn Rodrigues has exposed and fixed a bug which underflows btrfs
qgroup reserved space, and leads to non-writable fs.

This reminds us that we don't have enough underflow check for qgroup
reserved space.

For underflow case, we should not really underflow the numbers but warn
and keeps qgroup still work.

So add more check on qgroup reserved space and add WARN_ON() and
btrfs_warn() for any underflow case.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
Changelog:
v2:
  Add btrfs_warn() output for fsid, qgroupid, original reserved space and
  num_bytes to reduce, for end-user to locate the subvolume causing the
  problem. Suggested by David.
---
 fs/btrfs/qgroup.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 11f4fff..fc0c64e 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1031,6 +1031,15 @@ static void qgroup_dirty(struct btrfs_fs_info *fs_info,
 		list_add(&qgroup->dirty, &fs_info->dirty_qgroups);
 }
 
+static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
+				      struct btrfs_qgroup *qgroup,
+				      u64 num_bytes)
+{
+	btrfs_warn(fs_info,
+		"qgroup %llu reserved space underflow, have: %llu, to free: %llu",
+		qgroup->qgroupid, qgroup->reserved, num_bytes);
+	qgroup->reserved = 0;
+}
 /*
  * The easy accounting, if we are adding/removing the only ref for an extent
  * then this qgroup and all of the parent qgroups get their reference and
@@ -1058,8 +1067,13 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0)
-		qgroup->reserved -= num_bytes;
+	if (sign > 0) {
+		if (WARN_ON(qgroup->reserved < num_bytes))
+			report_reserved_underflow(fs_info, qgroup,
+						  num_bytes);
+		else
+			qgroup->reserved -= num_bytes;
+	}
 
 	qgroup_dirty(fs_info, qgroup);
 
@@ -1079,8 +1093,13 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0)
-			qgroup->reserved -= num_bytes;
+		if (sign > 0) {
+			if (WARN_ON(qgroup->reserved < num_bytes))
+				report_reserved_underflow(fs_info, qgroup,
+							  num_bytes);
+			else
+				qgroup->reserved -= num_bytes;
+		}
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 
@@ -2204,7 +2223,10 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 
 		qg = u64_to_ptr(unode->aux);
 
-		qg->reserved -= num_bytes;
+		if (WARN_ON(qgroup->reserved < num_bytes))
+			report_reserved_underflow(fs_info, qgroup, num_bytes);
+		else
+			qgroup->reserved -= num_bytes;
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,
-- 
2.10.0

[PATCH 2/2] btrfs: Add trace point for qgroup reserved space

[Qu Wenruo]

Introduce the following trace points:
qgroup_update_reserve
qgroup_meta_reserve

And modify the timing of btrfs_qgroup_free_delayed_ref() and
btrfs_qgroup_release_data() events, to work with qgroup_update_reserve()
event.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c            | 21 ++++++++++++++++++---
 fs/btrfs/qgroup.h            |  2 +-
 include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index fc0c64e..aad34314 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1068,6 +1068,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
 	if (sign > 0) {
+		trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
+					    qgroup->reserved, (s64)-num_bytes);
 		if (WARN_ON(qgroup->reserved < num_bytes))
 			report_reserved_underflow(fs_info, qgroup,
 						  num_bytes);
@@ -1094,6 +1096,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
 		if (sign > 0) {
+			trace_qgroup_update_reserve(fs_info, qgroup->qgroupid,
+						    qgroup->reserved,
+						    (s64)-num_bytes);
 			if (WARN_ON(qgroup->reserved < num_bytes))
 				report_reserved_underflow(fs_info, qgroup,
 							  num_bytes);
@@ -2178,6 +2183,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes)
 
 		qg = u64_to_ptr(unode->aux);
 
+		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
+					    qg->reserved, num_bytes);
 		qg->reserved += num_bytes;
 	}
 
@@ -2223,6 +2230,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 
 		qg = u64_to_ptr(unode->aux);
 
+		trace_qgroup_update_reserve(fs_info, qg->qgroupid,
+					    qg->reserved, (s64)-num_bytes);
 		if (WARN_ON(qgroup->reserved < num_bytes))
 			report_reserved_underflow(fs_info, qgroup, num_bytes);
 		else
@@ -2651,12 +2660,12 @@ static int __btrfs_qgroup_release_data(struct inode *inode, u64 start, u64 len,
 	if (ret < 0)
 		goto out;
 
-	if (free) {
-		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
+	if (free)
 		trace_op = QGROUP_FREE;
-	}
 	trace_btrfs_qgroup_release_data(inode, start, len,
 					changeset.bytes_changed, trace_op);
+	if (free)
+		qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed);
 out:
 	ulist_free(changeset.range_changed);
 	return ret;
@@ -2706,6 +2715,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes)
 		return 0;
 
 	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)num_bytes);
 	ret = qgroup_reserve(root, num_bytes);
 	if (ret < 0)
 		return ret;
@@ -2724,6 +2735,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	reserved = atomic_xchg(&root->qgroup_meta_rsv, 0);
 	if (reserved == 0)
 		return;
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)-reserved);
 	qgroup_free(root, reserved);
 }
 
@@ -2736,6 +2749,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	BUG_ON(num_bytes != round_down(num_bytes, root->nodesize));
 	WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic_sub(num_bytes, &root->qgroup_meta_rsv);
+	trace_qgroup_meta_reserve(root->fs_info, root->objectid,
+				  (s64)-num_bytes);
 	qgroup_free(root, num_bytes);
 }
 
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 1bc64c8..6b6756c 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -114,8 +114,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
 }
 void assert_qgroups_uptodate(struct btrfs_trans_handle *trans);
 
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index e030d6f..fb3cb6c 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -1468,6 +1468,49 @@ TRACE_EVENT(qgroup_update_counters,
 		  __entry->cur_new_count)
 );
 
+TRACE_EVENT(qgroup_update_reserve,
+
+	TP_PROTO(struct btrfs_fs_info *fs_info, u64 qgid, u64 cur_reserved,
+		 s64 diff),
+
+	TP_ARGS(fs_info, qgid, cur_reserved, diff),
+
+	TP_STRUCT__entry_btrfs(
+		__field(	u64,	qgid			)
+		__field(	u64,	cur_reserved		)
+		__field(	s64,	diff			)
+	),
+
+	TP_fast_assign_btrfs(fs_info,
+		__entry->qgid		= qgid;
+		__entry->cur_reserved	= cur_reserved;
+		__entry->diff		= diff;
+	),
+
+	TP_printk_btrfs("qgid = %llu, cur_reserved = %llu, diff = %lld",
+		__entry->qgid, __entry->cur_reserved, __entry->diff)
+);
+
+TRACE_EVENT(qgroup_meta_reserve,
+
+	TP_PROTO(struct btrfs_fs_info *fs_info, u64 refroot, s64 diff),
+
+	TP_ARGS(fs_info, refroot, diff),
+
+	TP_STRUCT__entry_btrfs(
+		__field(	u64,	refroot			)
+		__field(	u64,	diff			)
+	),
+
+	TP_fast_assign_btrfs(fs_info,
+		__entry->refroot	= refroot;
+		__entry->diff		= diff;
+	),
+
+	TP_printk_btrfs("refroot = %llu, diff = %lld",
+		__entry->refroot, __entry->diff)
+);
+
 #endif /* _TRACE_BTRFS_H */
 
 /* This part must be outside protection */
-- 
2.10.0

Re: [PATCH 2/2] btrfs: Add trace point for qgroup reserved space

[David Sterba]

On Thu, Oct 20, 2016 at 10:28:42AM +0800, Qu Wenruo wrote:
> Introduce the following trace points:
> qgroup_update_reserve
> qgroup_meta_reserve
> 
> And modify the timing of btrfs_qgroup_free_delayed_ref() and
> btrfs_qgroup_release_data() events, to work with qgroup_update_reserve()
> event.

I think this should go to a separate patch, as it's a functional change
(and not obvious to me) unlike adding the tracepoints.