Btrfs devel - Mar 2013 - [PATCH] Btrfs-progs: fix overflow when printing qgroup info

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Since btrfs quota rescan has not been implemented yet,
a user complains that "btrfs qgroup show" lists qgroup
referenced/exclusive be negative. However, this should
not happen even if overflow happens,because the type for
qgroup referenced/exclusive is u64,fix it.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reported-by: Koen De Wit <koen.de.wit@oracle.com>
---
 cmds-qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 60ca33d..fc4cb13 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -105,7 +105,7 @@ static int qgroup_create(int create, int argc, char **argv)
 
 void print_qgroup_info(u64 objectid, struct btrfs_qgroup_info_item *info)
 {
-	printf("%llu/%llu %lld %lld\n", objectid >> 48,
+	printf("%llu/%llu %llu %llu\n", objectid >> 48,
 		objectid & ((1ll << 48) - 1),
 		btrfs_stack_qgroup_info_referenced(info),
 		btrfs_stack_qgroup_info_exclusive(info));
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, March 22, 2013 at 12:53 (+0100), Wang Shilong
wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> 
> Since btrfs quota rescan has not been implemented yet,
> a user complains that "btrfs qgroup show" lists qgroup
> referenced/exclusive be negative. However, this should
> not happen even if overflow happens,because the type for
> qgroup referenced/exclusive is u64,fix it.
> 
> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> Reported-by: Koen De Wit <koen.de.wit@oracle.com>
> ---
>  cmds-qgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index 60ca33d..fc4cb13 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -105,7 +105,7 @@ static int qgroup_create(int create, int argc, char
**argv)
>  
>  void print_qgroup_info(u64 objectid, struct btrfs_qgroup_info_item *info)
>  {
> -	printf("%llu/%llu %lld %lld\n", objectid >> 48,
> +	printf("%llu/%llu %llu %llu\n", objectid >> 48,
>  		objectid & ((1ll << 48) - 1),
>  		btrfs_stack_qgroup_info_referenced(info),
>  		btrfs_stack_qgroup_info_exclusive(info));
> 
I don''t like that change. Seeing negative numbers is what you should
expect in
the current situation.

Once anyone come across negative numbers with a volume holding more data than
what can be tracked with 63 bit, I may come to agree to your change. For now, it
will confuse more than help.

-Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,
> 
> On Fri, March 22, 2013 at 12:53 (+0100), Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> 
>> Since btrfs quota rescan has not been implemented yet,
>> a user complains that "btrfs qgroup show" lists qgroup
>> referenced/exclusive be negative. However, this should
>> not happen even if overflow happens,because the type for
>> qgroup referenced/exclusive is u64,fix it.
>> 
>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> Reported-by: Koen De Wit <koen.de.wit@oracle.com>
>> ---
>> cmds-qgroup.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index 60ca33d..fc4cb13 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -105,7 +105,7 @@ static int qgroup_create(int create, int argc, char
**argv)
>> 
>> void print_qgroup_info(u64 objectid, struct btrfs_qgroup_info_item
*info)
>> {
>> -	printf("%llu/%llu %lld %lld\n", objectid >> 48,
>> +	printf("%llu/%llu %llu %llu\n", objectid >> 48,
>> 		objectid & ((1ll << 48) - 1),
>> 		btrfs_stack_qgroup_info_referenced(info),
>> 		btrfs_stack_qgroup_info_exclusive(info));
>> 
> 
> I don''t like that change. Seeing negative numbers is what you
should expect in
> the current situation.
> 
> Once anyone come across negative numbers with a volume holding more data
than
> what can be tracked with 63 bit, I may come to agree to your change. For
now, it
> will confuse more than help.
Maybe, you are right.

But the type for referenced/exclusive is u64. Considering the following case:

overflow happens, referenced/exclusive changes into a big positive integer, so
next time,
when we doing accounting, it may return edquot. So i think the check in the
kernel is necessary.

Or am i missing something ?

Thanks,
Wang

> 
> -Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,
> Hello,
> 
>> 
>> On Fri, March 22, 2013 at 12:53 (+0100), Wang Shilong wrote:
>>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>> 
>>> Since btrfs quota rescan has not been implemented yet,
>>> a user complains that "btrfs qgroup show" lists qgroup
>>> referenced/exclusive be negative. However, this should
>>> not happen even if overflow happens,because the type for
>>> qgroup referenced/exclusive is u64,fix it.
>>> 
>>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>> Reported-by: Koen De Wit <koen.de.wit@oracle.com>
>>> ---
>>> cmds-qgroup.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>>> index 60ca33d..fc4cb13 100644
>>> --- a/cmds-qgroup.c
>>> +++ b/cmds-qgroup.c
>>> @@ -105,7 +105,7 @@ static int qgroup_create(int create, int argc,
char **argv)
>>> 
>>> void print_qgroup_info(u64 objectid, struct btrfs_qgroup_info_item
*info)
>>> {
>>> -	printf("%llu/%llu %lld %lld\n", objectid >> 48,
>>> +	printf("%llu/%llu %llu %llu\n", objectid >> 48,
>>> 		objectid & ((1ll << 48) - 1),
>>> 		btrfs_stack_qgroup_info_referenced(info),
>>> 		btrfs_stack_qgroup_info_exclusive(info));
>>> 
>> 
>> I don''t like that change. Seeing negative numbers is what you
should expect in
>> the current situation.
>> 
>> Once anyone come across negative numbers with a volume holding more
data than
>> what can be tracked with 63 bit, I may come to agree to your change.
For now, it
>> will confuse more than help.
> 
> Maybe, you are right.
> 
> But the type for referenced/exclusive is u64. Considering the following
case:
> 
> overflow happens, referenced/exclusive changes into a big positive integer,
so next time,
> when we doing accounting, it may return edquot. So i think the check in the
kernel is necessary.
For the above case, From the user view, the referenced/exclusive is negative, 
but user can''t continue to write data. How strange it is!

So, i think to have a check in the kernel and making the referenced/exclusive to
be 0
and give a warning in the kernel is better than current situation.

If you agree with my approach, i will make the patch and send it next week. ~_~

Thanks,
Wang
> 
> Or am i missing something ?
> 
> Thanks,
> Wang
> 
> 
>> 
>> -Jan
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html