thr3ads.net - Btrfs devel - [PATCH 1/2] Btrfs: cleanup duplicated division functions [Sep 2012]

If this information is useful, please help other people find it:
Share via:

Miao Xie

2012-Sep-13 10:51 UTC

[PATCH 1/2] Btrfs: cleanup duplicated division functions

div_factor{_fine} has been implemented for two times, cleanup it.
And I move them into a independent file named math.h because they are
common math functions.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c |   19 +------------------
 fs/btrfs/math.h        |   44 ++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c     |   23 +----------------------
 3 files changed, 46 insertions(+), 40 deletions(-)
 create mode 100644 fs/btrfs/math.h

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..5f6bae1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
 #include "volumes.h"
 #include "locking.h"
 #include "free-space-cache.h"
+#include "math.h"
 
 #undef SCRAMBLE_DELAYED_REFS
 
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
 	rcu_read_unlock();
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor == 100)
-		return num;
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 u64 btrfs_find_block_group(struct btrfs_root *root,
 			   u64 search_start, u64 search_hint, int owner)
 {
diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
new file mode 100644
index 0000000..b7816ce
--- /dev/null
+++ b/fs/btrfs/math.h
@@ -0,0 +1,44 @@
+
+/*
+ * Copyright (C) 2012 Fujitsu.  All rights reserved.
+ * Written by Miao Xie <miaox@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef __BTRFS_MATH_H
+#define __BTRFS_MATH_H
+
+#include <asm/div64.h>
+
+static inline u64 div_factor(u64 num, int factor)
+{
+	if (factor == 10)
+		return num;
+	num *= factor;
+	do_div(num, 10);
+	return num;
+}
+
+static inline u64 div_factor_fine(u64 num, int factor)
+{
+	if (factor == 100)
+		return num;
+	num *= factor;
+	do_div(num, 100);
+	return num;
+}
+
+#endif
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f4e70e..2558fc0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
-#include <asm/div64.h>
 #include "compat.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -36,6 +35,7 @@
 #include "async-thread.h"
 #include "check-integrity.h"
 #include "rcu-string.h"
+#include "math.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
 	return 1;
 }
 
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor <= 0)
-		return 0;
-	if (factor >= 100)
-		return num;
-
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
 			      struct btrfs_balance_args *bargs)
 {
@@ -2501,15 +2489,6 @@ static int should_balance_chunk(struct btrfs_root *root,
 	return 1;
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
 static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
-- 
1.7.6.5
--
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

David Sterba

2012-Sep-14 13:54 UTC

head link

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie
wrote:> div_factor{_fine} has been implemented for two times, cleanup it.
> And I move them into a independent file named math.h because they are
> common math functions.
You removed the sanity checks:

-       if (factor <= 0)
-               return 0;
-       if (factor >= 100)
-               return num;

in new version. And I don''t think it''s necessary to add an
extra include
with a rather generic name and trivial code. A separate .h/.c with
non-filesystem related support code like this looks more suitable.

Do you intend to use the functions out of extent-tree.c ?

david
--
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

Miao Xie

2012-Sep-17 02:21 UTC

head link

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

On fri, 14 Sep 2012 15:54:18 +0200, David Sterba wrote:> On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie wrote:
>> div_factor{_fine} has been implemented for two times, cleanup it.
>> And I move them into a independent file named math.h because they are
>> common math functions.
> 
> You removed the sanity checks:
> 
> -       if (factor <= 0)
> -               return 0;
> -       if (factor >= 100)
> -               return num;
As inline functions, they should not contain complex checks, the caller should
make sure the parameters are right. I think.
> in new version. And I don''t think it''s necessary to add
an extra include
> with a rather generic name and trivial code. A separate .h/.c with
> non-filesystem related support code like this looks more suitable.
> 
> Do you intend to use the functions out of extent-tree.c ?
They are used in both extent-tree.c and volumes.c from the outset, but they
were implemented in these two files severally.
--
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

Ilya Dryomov

2012-Sep-17 12:07 UTC

head link

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

On Mon, Sep 17, 2012 at 10:21:00AM +0800, Miao Xie
wrote:> On fri, 14 Sep 2012 15:54:18 +0200, David Sterba wrote:
> > On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie wrote:
> >> div_factor{_fine} has been implemented for two times, cleanup it.
> >> And I move them into a independent file named math.h because they
are
> >> common math functions.
> > 
> > You removed the sanity checks:
> > 
> > -       if (factor <= 0)
> > -               return 0;
> > -       if (factor >= 100)
> > -               return num;
> 
> As inline functions, they should not contain complex checks, the caller
should
> make sure the parameters are right. I think.
div_factor_fine() in volumes.c is not inline, and is called from
chunk_usage_filter() on unvalidated user input.  If you think the caller
should do those checks, you should move them to the caller as part of
your patch.

Thanks,

		Ilya
--
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

David Sterba

2012-Sep-17 16:31 UTC

head link

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

On Mon, Sep 17, 2012 at 10:21:00AM +0800, Miao Xie
wrote:> On fri, 14 Sep 2012 15:54:18 +0200, David Sterba wrote:
> > On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie wrote:
> >> div_factor{_fine} has been implemented for two times, cleanup it.
> >> And I move them into a independent file named math.h because they
are
> >> common math functions.
> > 
> > You removed the sanity checks:
> > 
> > -       if (factor <= 0)
> > -               return 0;
> > -       if (factor >= 100)
> > -               return num;
> 
> As inline functions, they should not contain complex checks, the caller
should
> make sure the parameters are right. I think.
It''s compiler''s job to decide whether a function should be
inlined or
not. The keyword/function attribute ''inline'' is only a hint,
unless
always_inline is used and the author should be sure that it really has
the expected outcome and that compiler is wrong here.

I don''t agree that each caller should do the checks, it only makes code
harder to read and forces the authors to check for conditions that may
not be apparent or are just ommitted.

If we need a function that does not check the boundaries, then of course
go for it, but I don''t see such case yet.
> > in new version. And I don''t think it''s necessary to
add an extra include
> > with a rather generic name and trivial code. A separate .h/.c with
> > non-filesystem related support code like this looks more suitable.
> > 
> > Do you intend to use the functions out of extent-tree.c ?
> 
> They are used in both extent-tree.c and volumes.c from the outset, but they
> were implemented in these two files severally.
Ah, I see.

david
--
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

Miao Xie

2012-Sep-18 03:53 UTC

head link

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

On Mon, 17 Sep 2012 18:31:13 +0200, David Sterba wrote:> On Mon, Sep 17, 2012 at 10:21:00AM +0800, Miao Xie wrote:
>> On fri, 14 Sep 2012 15:54:18 +0200, David Sterba wrote:
>>> On Thu, Sep 13, 2012 at 06:51:36PM +0800, Miao Xie wrote:
>>>> div_factor{_fine} has been implemented for two times, cleanup
it.
>>>> And I move them into a independent file named math.h because
they are
>>>> common math functions.
>>>
>>> You removed the sanity checks:
>>>
>>> -       if (factor <= 0)
>>> -               return 0;
>>> -       if (factor >= 100)
>>> -               return num;
>>
>> As inline functions, they should not contain complex checks, the caller
should
>> make sure the parameters are right. I think.
> 
> It''s compiler''s job to decide whether a function should
be inlined or
> not. The keyword/function attribute ''inline'' is only a
hint, unless
> always_inline is used and the author should be sure that it really has
> the expected outcome and that compiler is wrong here.
Right, but I think we should make the functions as simple as possible since
they are marked as inline, because the simple function is more likely to be
inlined
than the complex one.
> I don''t agree that each caller should do the checks, it only makes
code
> harder to read and forces the authors to check for conditions that may
> not be apparent or are just ommitted.
Right. But for these functions, we are sure the value of the parameters is
in the right range in the most place, and all the place that we are sure the
value is right is in the hot path. The only place that we need check the
parameters is in slow path, this is also the reason why we make them inline.
so doing those checks just wastes time. We just need modify the caller.

Thanks
Miao
> If we need a function that does not check the boundaries, then of course
> go for it, but I don''t see such case yet.
> 
>>> in new version. And I don''t think it''s necessary
to add an extra include
>>> with a rather generic name and trivial code. A separate .h/.c with
>>> non-filesystem related support code like this looks more suitable.
>>>
>>> Do you intend to use the functions out of extent-tree.c ?
>>
>> They are used in both extent-tree.c and volumes.c from the outset, but
they
>> were implemented in these two files severally.
> 
> Ah, I see.
> 
> david
> 

--
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

Miao Xie

2012-Sep-20 02:57 UTC

head link

[PATCH V2 1/2] Btrfs: cleanup duplicated division functions

div_factor{_fine} has been implemented for two times, cleanup it.
And I move them into a independent file named math.h because they are
common math functions.

Because those functions are mostly used on the hot path, and we are sure
the parameters are right in the most cases, we don''t add complex checks
for the parameters. But in the other place, we must check and make sure
the parameters are right. So besides the code cleanup, this patch also
add a check for the usage of the space balance, it is the only place that
we need add check to make sure the parameters of div_factor{_fine} are
right till now.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v1 -> v2:
- add missing check
---
 fs/btrfs/extent-tree.c |   19 +------------------
 fs/btrfs/ioctl.c       |   18 ++++++++++++++++++
 fs/btrfs/math.h        |   44 ++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c     |   23 +----------------------
 4 files changed, 64 insertions(+), 40 deletions(-)
 create mode 100644 fs/btrfs/math.h

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..5f6bae1 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
 #include "volumes.h"
 #include "locking.h"
 #include "free-space-cache.h"
+#include "math.h"
 
 #undef SCRAMBLE_DELAYED_REFS
 
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
 	rcu_read_unlock();
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor == 100)
-		return num;
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 u64 btrfs_find_block_group(struct btrfs_root *root,
 			   u64 search_start, u64 search_hint, int owner)
 {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9384a2a..d8d53f7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file, void
__user *arg)
 
 			goto do_balance;
 		}
+
+		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->data.usage < 0 || bargs->data.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
+
+		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->meta.usage < 0 || bargs->meta.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
+
+		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
 	} else {
 		bargs = NULL;
 	}
diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
new file mode 100644
index 0000000..b7816ce
--- /dev/null
+++ b/fs/btrfs/math.h
@@ -0,0 +1,44 @@
+
+/*
+ * Copyright (C) 2012 Fujitsu.  All rights reserved.
+ * Written by Miao Xie <miaox@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef __BTRFS_MATH_H
+#define __BTRFS_MATH_H
+
+#include <asm/div64.h>
+
+static inline u64 div_factor(u64 num, int factor)
+{
+	if (factor == 10)
+		return num;
+	num *= factor;
+	do_div(num, 10);
+	return num;
+}
+
+static inline u64 div_factor_fine(u64 num, int factor)
+{
+	if (factor == 100)
+		return num;
+	num *= factor;
+	do_div(num, 100);
+	return num;
+}
+
+#endif
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f4e70e..2558fc0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
-#include <asm/div64.h>
 #include "compat.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -36,6 +35,7 @@
 #include "async-thread.h"
 #include "check-integrity.h"
 #include "rcu-string.h"
+#include "math.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
 	return 1;
 }
 
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor <= 0)
-		return 0;
-	if (factor >= 100)
-		return num;
-
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
 			      struct btrfs_balance_args *bargs)
 {
@@ -2501,15 +2489,6 @@ static int should_balance_chunk(struct btrfs_root *root,
 	return 1;
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
 static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
-- 
1.7.6.5

--
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

David Sterba

2012-Sep-20 13:28 UTC

head link

Re: [PATCH V2 1/2] Btrfs: cleanup duplicated division functions

On Thu, Sep 20, 2012 at 10:57:54AM +0800, Miao Xie
wrote:> Because those functions are mostly used on the hot path, and we are sure
> the parameters are right in the most cases, we don''t add complex
checks
> for the parameters. But in the other place, we must check and make sure
> the parameters are right. So besides the code cleanup, this patch also
> add a check for the usage of the space balance, it is the only place that
> we need add check to make sure the parameters of div_factor{_fine} are
> right till now.
I''ve reviewed the hotpaths, makes sense to optimize for speed. Adding
the boundary checks to balance ioctl is ok.

A few suggestions:

* drop the version that does the /10 version and keep only /100 (naming
  it div_factor)
* drop the

+	if (factor == 10)
+		return num;

check, there''s only one instance where we pass the maximum value (and
it''s not a frequent case), so there''s the if() penalty, makes
the
function smaller and even more suitable for inlining.
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9384a2a..d8d53f7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
>  
>  			goto do_balance;
>  		}
> +
> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->data.usage < 0 || bargs->data.usage > 100)) {
data.usage <= 0

otherwise you''d divide by 0 in chunk_usage_filter()

(in current version the check is done in the _fine version, so -dusage=0
does not crash kernel)
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->meta.usage < 0 || bargs->meta.usage > 100)) {
same here
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
same here
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
>  	} else {
>  		bargs = NULL;
>  	}
> diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
> new file mode 100644
> index 0000000..b7816ce
> --- /dev/null
> +++ b/fs/btrfs/math.h
I think we don''t need single file to hold one trivial function then :)
> @@ -0,0 +1,44 @@
> +#include <asm/div64.h>
> +
> +static inline u64 div_factor(u64 num, int factor)
> +{
> +	num *= factor;
> +	do_div(num, 100);
> +	return num;
> +}
david
--
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

Miao Xie

2012-Sep-21 08:26 UTC

head link

Re: [PATCH V2 1/2] Btrfs: cleanup duplicated division functions

On Thu, 20 Sep 2012 15:28:03 +0200, David Sterba wrote:> On Thu, Sep 20, 2012 at 10:57:54AM +0800, Miao Xie wrote:
>> Because those functions are mostly used on the hot path, and we are
sure
>> the parameters are right in the most cases, we don''t add
complex checks
>> for the parameters. But in the other place, we must check and make sure
>> the parameters are right. So besides the code cleanup, this patch also
>> add a check for the usage of the space balance, it is the only place
that
>> we need add check to make sure the parameters of div_factor{_fine} are
>> right till now.
> 
> I''ve reviewed the hotpaths, makes sense to optimize for speed.
Adding
> the boundary checks to balance ioctl is ok.
> 
> A few suggestions:
> 
> * drop the version that does the /10 version and keep only /100 (naming
>   it div_factor)
> * drop the
> 
> +	if (factor == 10)
> +		return num;
> 
> check, there''s only one instance where we pass the maximum value
(and
> it''s not a frequent case), so there''s the if() penalty,
makes the
> function smaller and even more suitable for inlining.
OK.
> 
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 9384a2a..d8d53f7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file
*file, void __user *arg)
>>  
>>  			goto do_balance;
>>  		}
>> +
>> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->data.usage < 0 || bargs->data.usage > 100))
{
> 
> data.usage <= 0
> 
> otherwise you''d divide by 0 in chunk_usage_filter()
The divisor always is 100 or 10, so...
>> diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
>> new file mode 100644
>> index 0000000..b7816ce
>> --- /dev/null
>> +++ b/fs/btrfs/math.h
> 
> I think we don''t need single file to hold one trivial function
then :)
> 
>> @@ -0,0 +1,44 @@
>> +#include <asm/div64.h>
>> +
>> +static inline u64 div_factor(u64 num, int factor)
>> +{
>> +	num *= factor;
>> +	do_div(num, 100);
>> +	return num;
>> +}
I don''t find a suitable file to put it down. Maybe we can stuff it
into ctree.h, but I prefer a single file to a unrelated file.

Thanks
Miao
--
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

Miao Xie

2012-Sep-21 09:07 UTC

head link

[PATCH V3 1/2] Btrfs: cleanup duplicated division functions

div_factor{_fine} has been implemented for two times, and these two functions
are very similar, so cleanup the reduplicate implement and drop the original
div_factor(), and then rename div_factor_fine() to div_factor(). So the divisor
of the new div_factor() is 100, not 10.

And I move div_factor into a independent file named math.h because it is a
common math function, may be used by every composition of btrfs.

Because these functions are mostly used on the hot path, and we are sure
the parameters are right in the most cases, we don''t add complex checks
for the parameters. But in the other place, we must check and make sure
the parameters are right. So besides the code cleanup, this patch also
add a check for the usage of the space balance, it is the only place that
we need add check to make sure the parameters of div_factor are right till now.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v2 -> v3:
- drop the original div_factor and rename div_factor_fine to div_factor
- drop the check of the factor

Changelog v1 -> v2:
- add missing check
---
 fs/btrfs/extent-tree.c |   29 ++++++-----------------------
 fs/btrfs/ioctl.c       |   18 ++++++++++++++++++
 fs/btrfs/math.h        |   33 +++++++++++++++++++++++++++++++++
 fs/btrfs/relocation.c  |    2 +-
 fs/btrfs/transaction.c |    2 +-
 fs/btrfs/volumes.c     |   30 +++++-------------------------
 6 files changed, 64 insertions(+), 50 deletions(-)
 create mode 100644 fs/btrfs/math.h

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..bcb9ced 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
 #include "volumes.h"
 #include "locking.h"
 #include "free-space-cache.h"
+#include "math.h"
 
 #undef SCRAMBLE_DELAYED_REFS
 
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
 	rcu_read_unlock();
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor == 100)
-		return num;
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 u64 btrfs_find_block_group(struct btrfs_root *root,
 			   u64 search_start, u64 search_hint, int owner)
 {
@@ -674,7 +657,7 @@ u64 btrfs_find_block_group(struct btrfs_root *root,
 	u64 last = max(search_hint, search_start);
 	u64 group_start = 0;
 	int full_search = 0;
-	int factor = 9;
+	int factor = 90;
 	int wrapped = 0;
 again:
 	while (1) {
@@ -708,7 +691,7 @@ again:
 	if (!full_search && factor < 10) {
 		last = search_start;
 		full_search = 1;
-		factor = 10;
+		factor = 100;
 		goto again;
 	}
 found:
@@ -3513,7 +3496,7 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	if (force == CHUNK_ALLOC_LIMITED) {
 		thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 		thresh = max_t(u64, 64 * 1024 * 1024,
-			       div_factor_fine(thresh, 1));
+			       div_factor(thresh, 1));
 
 		if (num_bytes - num_allocated < thresh)
 			return 1;
@@ -3521,12 +3504,12 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 
 	/* 256MB or 2% of the FS */
-	thresh = max_t(u64, 256 * 1024 * 1024, div_factor_fine(thresh, 2));
+	thresh = max_t(u64, 256 * 1024 * 1024, div_factor(thresh, 2));
 	/* system chunks need a much small threshold */
 	if (sinfo->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		thresh = 32 * 1024 * 1024;
 
-	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 8))
+	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 80))
 		return 0;
 	return 1;
 }
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9384a2a..d8d53f7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file, void
__user *arg)
 
 			goto do_balance;
 		}
+
+		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->data.usage < 0 || bargs->data.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
+
+		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->meta.usage < 0 || bargs->meta.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
+
+		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
+			ret = -EINVAL;
+			goto out_bargs;
+		}
 	} else {
 		bargs = NULL;
 	}
diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
new file mode 100644
index 0000000..a157665
--- /dev/null
+++ b/fs/btrfs/math.h
@@ -0,0 +1,33 @@
+
+/*
+ * Copyright (C) 2012 Fujitsu.  All rights reserved.
+ * Written by Miao Xie <miaox@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef __BTRFS_MATH_H
+#define __BTRFS_MATH_H
+
+#include <asm/div64.h>
+
+static inline u64 div_factor(u64 num, int factor)
+{
+	num *= factor;
+	do_div(num, 100);
+	return num;
+}
+
+#endif
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f193096..2254478 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3813,7 +3813,7 @@ restart:
 			}
 		}
 
-		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 5);
+		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 50);
 		if (ret < 0) {
 			if (ret != -ENOSPC) {
 				err = ret;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index cf98dbc..115f054 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -489,7 +489,7 @@ static int should_end_transaction(struct btrfs_trans_handle
*trans,
 {
 	int ret;
 
-	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
5);
+	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
50);
 	return ret ? 1 : 0;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f4e70e..1fd43a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
-#include <asm/div64.h>
 #include "compat.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -36,6 +35,7 @@
 #include "async-thread.h"
 #include "check-integrity.h"
 #include "rcu-string.h"
+#include "math.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
 	return 1;
 }
 
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor <= 0)
-		return 0;
-	if (factor >= 100)
-		return num;
-
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
 			      struct btrfs_balance_args *bargs)
 {
@@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
 	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
 	chunk_used = btrfs_block_group_used(&cache->item);
 
-	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
+	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
+	user_thresh = div_factor(cache->key.offset, bargs->usage);
 	if (chunk_used < user_thresh)
 		ret = 0;
 
@@ -2501,15 +2490,6 @@ static int should_balance_chunk(struct btrfs_root *root,
 	return 1;
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
 static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
@@ -2534,7 +2514,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	devices = &fs_info->fs_devices->devices;
 	list_for_each_entry(device, devices, dev_list) {
 		old_size = device->total_bytes;
-		size_to_free = div_factor(old_size, 1);
+		size_to_free = div_factor(old_size, 10);
 		size_to_free = min(size_to_free, (u64)1 * 1024 * 1024);
 		if (!device->writeable ||
 		    device->total_bytes - device->bytes_used > size_to_free)
@@ -3284,7 +3264,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle
*trans,
 	}
 
 	/* we don''t want a chunk larger than 10% of writeable space */
-	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
+	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 10),
 			     max_chunk_size);
 
 	devices_info = kzalloc(sizeof(*devices_info) * fs_devices->rw_devices,
-- 
1.7.6.5

--
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

David Sterba

2012-Sep-21 15:24 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie
wrote:> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
>  
>  			goto do_balance;
>  		}
> +
> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->data.usage < 0 || bargs->data.usage > 100)) {
the 0 checks belong here
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->meta.usage < 0 || bargs->meta.usage > 100)) {
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
>  	} else {
>  		bargs = NULL;
>  	}
> @@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
>  	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>  	chunk_used = btrfs_block_group_used(&cache->item);
>  
> -	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
> +	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
otherwise it reliably crashes here
> +	user_thresh = div_factor(cache->key.offset, bargs->usage);
>  	if (chunk_used < user_thresh)
>  		ret = 0;
>  
other than that, patch looks good tome,

thanks,
david
--
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

Miao Xie

2012-Sep-23 09:54 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Fri, 21 Sep 2012 17:24:44 +0200, David Sterba wrote:> On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie wrote:
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file
*file, void __user *arg)
>>  
>>  			goto do_balance;
>>  		}
>> +
>> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->data.usage < 0 || bargs->data.usage > 100))
{
> 
> the 0 checks belong here
> 
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>> +
>> +		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->meta.usage < 0 || bargs->meta.usage > 100))
{
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>> +
>> +		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>>  	} else {
>>  		bargs = NULL;
>>  	}
>> @@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct
btrfs_fs_info *fs_info, u64 chunk_offset,
>>  	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>>  	chunk_used = btrfs_block_group_used(&cache->item);
>>  
>> -	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
>> +	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
> 
> otherwise it reliably crashes here
Sorry, I don''t know why it will crash here if we input 0. I tried to
input 0,
and it worked well.

I think the only case we must take into account is the users might input the
wrong value (>100 or <0)
on the old kernel, and it can be stored into the filesystem. If we mount this
filesystem
on the new kernel, some problems may happen.

Thanks
Miao
--
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

Ilya Dryomov

2012-Sep-23 11:49 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie
wrote:> div_factor{_fine} has been implemented for two times, and these two
functions
> are very similar, so cleanup the reduplicate implement and drop the
original
> div_factor(), and then rename div_factor_fine() to div_factor(). So the
divisor
> of the new div_factor() is 100, not 10.
> 
> And I move div_factor into a independent file named math.h because it is a
> common math function, may be used by every composition of btrfs.
> 
> Because these functions are mostly used on the hot path, and we are sure
> the parameters are right in the most cases, we don''t add complex
checks
> for the parameters. But in the other place, we must check and make sure
> the parameters are right. So besides the code cleanup, this patch also
> add a check for the usage of the space balance, it is the only place that
> we need add check to make sure the parameters of div_factor are right till
now.
> 
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> Changelog v2 -> v3:
> - drop the original div_factor and rename div_factor_fine to div_factor
> - drop the check of the factor
> 
> Changelog v1 -> v2:
> - add missing check
> ---
>  fs/btrfs/extent-tree.c |   29 ++++++-----------------------
>  fs/btrfs/ioctl.c       |   18 ++++++++++++++++++
>  fs/btrfs/math.h        |   33 +++++++++++++++++++++++++++++++++
>  fs/btrfs/relocation.c  |    2 +-
>  fs/btrfs/transaction.c |    2 +-
>  fs/btrfs/volumes.c     |   30 +++++-------------------------
>  6 files changed, 64 insertions(+), 50 deletions(-)
>  create mode 100644 fs/btrfs/math.h
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a010234..bcb9ced 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -33,6 +33,7 @@
>  #include "volumes.h"
>  #include "locking.h"
>  #include "free-space-cache.h"
> +#include "math.h"
>  
>  #undef SCRAMBLE_DELAYED_REFS
>  
> @@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
>  	rcu_read_unlock();
>  }
>  
> -static u64 div_factor(u64 num, int factor)
> -{
> -	if (factor == 10)
> -		return num;
> -	num *= factor;
> -	do_div(num, 10);
> -	return num;
> -}
> -
> -static u64 div_factor_fine(u64 num, int factor)
> -{
> -	if (factor == 100)
> -		return num;
> -	num *= factor;
> -	do_div(num, 100);
> -	return num;
> -}
> -
>  u64 btrfs_find_block_group(struct btrfs_root *root,
>  			   u64 search_start, u64 search_hint, int owner)
>  {
> @@ -674,7 +657,7 @@ u64 btrfs_find_block_group(struct btrfs_root *root,
>  	u64 last = max(search_hint, search_start);
>  	u64 group_start = 0;
>  	int full_search = 0;
> -	int factor = 9;
> +	int factor = 90;
>  	int wrapped = 0;
>  again:
>  	while (1) {
> @@ -708,7 +691,7 @@ again:
>  	if (!full_search && factor < 10) {
>  		last = search_start;
>  		full_search = 1;
> -		factor = 10;
> +		factor = 100;
>  		goto again;
>  	}
>  found:
> @@ -3513,7 +3496,7 @@ static int should_alloc_chunk(struct btrfs_root
*root,
>  	if (force == CHUNK_ALLOC_LIMITED) {
>  		thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
>  		thresh = max_t(u64, 64 * 1024 * 1024,
> -			       div_factor_fine(thresh, 1));
> +			       div_factor(thresh, 1));
>  
>  		if (num_bytes - num_allocated < thresh)
>  			return 1;
> @@ -3521,12 +3504,12 @@ static int should_alloc_chunk(struct btrfs_root
*root,
>  	thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
>  
>  	/* 256MB or 2% of the FS */
> -	thresh = max_t(u64, 256 * 1024 * 1024, div_factor_fine(thresh, 2));
> +	thresh = max_t(u64, 256 * 1024 * 1024, div_factor(thresh, 2));
>  	/* system chunks need a much small threshold */
>  	if (sinfo->flags & BTRFS_BLOCK_GROUP_SYSTEM)
>  		thresh = 32 * 1024 * 1024;
>  
> -	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 8))
> +	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 80))
>  		return 0;
>  	return 1;
>  }
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9384a2a..d8d53f7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
>  
>  			goto do_balance;
>  		}
> +
> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->data.usage < 0 || bargs->data.usage > 100)) {
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->meta.usage < 0 || bargs->meta.usage > 100)) {
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
> +
> +		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
> +		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
> +			ret = -EINVAL;
> +			goto out_bargs;
> +		}
>  	} else {
>  		bargs = NULL;
>  	}
Why not drop this hunk ...
> diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
> new file mode 100644
> index 0000000..a157665
> --- /dev/null
> +++ b/fs/btrfs/math.h
> @@ -0,0 +1,33 @@
> +
> +/*
> + * Copyright (C) 2012 Fujitsu.  All rights reserved.
> + * Written by Miao Xie <miaox@cn.fujitsu.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; if not, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#ifndef __BTRFS_MATH_H
> +#define __BTRFS_MATH_H
> +
> +#include <asm/div64.h>
> +
> +static inline u64 div_factor(u64 num, int factor)
> +{
> +	num *= factor;
> +	do_div(num, 100);
> +	return num;
> +}
> +
> +#endif
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index f193096..2254478 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3813,7 +3813,7 @@ restart:
>  			}
>  		}
>  
> -		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 5);
> +		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 50);
>  		if (ret < 0) {
>  			if (ret != -ENOSPC) {
>  				err = ret;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index cf98dbc..115f054 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -489,7 +489,7 @@ static int should_end_transaction(struct
btrfs_trans_handle *trans,
>  {
>  	int ret;
>  
> -	ret = btrfs_block_rsv_check(root,
&root->fs_info->global_block_rsv, 5);
> +	ret = btrfs_block_rsv_check(root,
&root->fs_info->global_block_rsv, 50);
>  	return ret ? 1 : 0;
>  }
>  
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 3f4e70e..1fd43a4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -25,7 +25,6 @@
>  #include <linux/capability.h>
>  #include <linux/ratelimit.h>
>  #include <linux/kthread.h>
> -#include <asm/div64.h>
>  #include "compat.h"
>  #include "ctree.h"
>  #include "extent_map.h"
> @@ -36,6 +35,7 @@
>  #include "async-thread.h"
>  #include "check-integrity.h"
>  #include "rcu-string.h"
> +#include "math.h"
>  
>  static int init_first_rw_device(struct btrfs_trans_handle *trans,
>  				struct btrfs_root *root,
> @@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
>  	return 1;
>  }
>  
> -static u64 div_factor_fine(u64 num, int factor)
> -{
> -	if (factor <= 0)
> -		return 0;
> -	if (factor >= 100)
> -		return num;
> -
> -	num *= factor;
> -	do_div(num, 100);
> -	return num;
> -}
> -
>  static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64
chunk_offset,
>  			      struct btrfs_balance_args *bargs)
>  {
> @@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
>  	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>  	chunk_used = btrfs_block_group_used(&cache->item);
>  
> -	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
> +	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
> +	user_thresh = div_factor(cache->key.offset, bargs->usage);
>  	if (chunk_used < user_thresh)
>  		ret = 0;
 ... and leave the check where it was before?

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6019fb2..ff86f91 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2334,8 +2334,13 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
 	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
 	chunk_used = btrfs_block_group_used(&cache->item);
 
-	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
-	user_thresh = div_factor(cache->key.offset, bargs->usage);
+	if (bargs->usage == 0)
+		user_thresh = 0;
+	else if (bargs->usage >= 100)
+		user_thresh = cache->key.offset;
+	else
+		user_thresh = div_factor(cache->key.offset, bargs->usage);
+
 	if (chunk_used < user_thresh)
 		ret = 0;

(diff is on top of the patch in question)

This is the most straightforward transformation I can think of.  It
doesn''t result in an unnecessary BUG_ON, keeps churn to a minimum and
doesn''t change the "style" of the balance ioctl.  (If I were
to check
every filter argument that way, btrfs_balance_ioctl() would be very long
and complicated.)

Thanks,

		Ilya
--
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

Miao Xie

2012-Sep-24 02:05 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Sun, 23 Sep 2012 14:49:24 +0300, Ilya Dryomov wrote:>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 9384a2a..d8d53f7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file
*file, void __user *arg)
>>  
>>  			goto do_balance;
>>  		}
>> +
>> +		if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->data.usage < 0 || bargs->data.usage > 100))
{
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>> +
>> +		if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->meta.usage < 0 || bargs->meta.usage > 100))
{
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>> +
>> +		if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
>> +		    (bargs->sys.usage < 0 || bargs->sys.usage > 100)) {
>> +			ret = -EINVAL;
>> +			goto out_bargs;
>> +		}
>>  	} else {
>>  		bargs = NULL;
>>  	}
> 
> Why not drop this hunk ...
Generally, we should check the value when it is input. If not, we might
run our program with the wrong value, and it is possible to cause unknown
problems. So I think the above chunk is necessary.
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6019fb2..ff86f91 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2334,8 +2334,13 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
>  	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>  	chunk_used = btrfs_block_group_used(&cache->item);
>  
> -	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
> -	user_thresh = div_factor(cache->key.offset, bargs->usage);
> +	if (bargs->usage == 0)
> +		user_thresh = 0;
> +	else if (bargs->usage >= 100)
> +		user_thresh = cache->key.offset;
> +	else
> +		user_thresh = div_factor(cache->key.offset, bargs->usage);
> +
>  	if (chunk_used < user_thresh)
>  		ret = 0;
> 
> (diff is on top of the patch in question)
> 
> This is the most straightforward transformation I can think of.  It
> doesn''t result in an unnecessary BUG_ON, keeps churn to a minimum
and
agree with you.
> doesn''t change the "style" of the balance ioctl.  (If I
were to check
> every filter argument that way, btrfs_balance_ioctl() would be very long
> and complicated.)

I think the check in btrfs_balance_ioctl() is necessary, the reason is above.

Thanks
Miao
--
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

David Sterba

2012-Sep-24 16:33 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Sun, Sep 23, 2012 at 05:54:18PM +0800, Miao Xie
wrote:> >> @@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct
btrfs_fs_info *fs_info, u64 chunk_offset,
> >>  	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> >>  	chunk_used = btrfs_block_group_used(&cache->item);
> >>  
> >> -	user_thresh = div_factor_fine(cache->key.offset,
bargs->usage);
> >> +	BUG_ON(bargs->usage < 0 || bargs->usage > 100);
> > 
> > otherwise it reliably crashes here
> 
> Sorry, I don''t know why it will crash here if we input 0. I tried
to input 0,
> and it worked well.
My oversight, sorry.
> I think the only case we must take into account is the users might input
the wrong value (>100 or <0)
> on the old kernel, and it can be stored into the filesystem. If we mount
this filesystem
> on the new kernel, some problems may happen.
So better avoid a BUG_ON.
--
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

David Sterba

2012-Sep-24 16:47 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie
wrote:> Generally, we should check the value when it is input. If not, we might
> run our program with the wrong value, and it is possible to cause unknown
> problems. So I think the above chunk is necessary.
The difference is that giving an invalid value will exit early (your
version), while Ilya''s version will clamp the ''usage''
to the allowed
range during processing. From usability POV, I''d prefer to let the
command fail early with a verbose error eg. that the range is invalid.
> > (diff is on top of the patch in question)
> > 
> > This is the most straightforward transformation I can think of.  It
> > doesn''t result in an unnecessary BUG_ON, keeps churn to a
minimum and
> 
> agree with you.
> 
> > doesn''t change the "style" of the balance ioctl. 
(If I were to check
> > every filter argument that way, btrfs_balance_ioctl() would be very
long
> > and complicated.)
> 
> I think the check in btrfs_balance_ioctl() is necessary, the reason is
above.
btrfs_balance_ioctl does not seem as the right place, it does the
processing related to the state of balance (resume/cancel etc). Look at
btrfs_balance() itself, it does lot more sanity checks of the
parameters. We can put the extra checks into helpers (and not only this
one) if clarity and readability of the function becomes a concern.


david
--
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

Ilya Dryomov

2012-Sep-24 18:42 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On Mon, Sep 24, 2012 at 06:47:42PM +0200, David Sterba
wrote:> On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote:
> > Generally, we should check the value when it is input. If not, we
might
> > run our program with the wrong value, and it is possible to cause
unknown
> > problems. So I think the above chunk is necessary.
> 
> The difference is that giving an invalid value will exit early (your
> version), while Ilya''s version will clamp the
''usage'' to the allowed
> range during processing. From usability POV, I''d prefer to let the
> command fail early with a verbose error eg. that the range is invalid.
I think that''s the job for progs, and that''s where this and a
few other
checks are currently implemented.

There is no possibility for "unknown problems", that''s
exactly how it''s
been implemented before the cleanup.  The purpose of balance filters
(and the usage filter in particular) is to lower the number of chunks we
have to relocate.  If someone decides to use raw ioctls, and supplies us
with the invalid value, we just cut it down to a 100 and proceed.
usage=100 is the equivalent of not using the usage filter at all, so the
raw ioctl user will get what he deserves.

This is very similar to what we currently do with other filters.  For
example, "soft" can only be used with "convert", and this is
checked by
progs.  But, if somebody were to set a "soft" flag without setting
"convert" we would simply ignore that "soft".  Same goes for
"drange"
and "devid", invalid ranges, invalid devids, etc.  Pulling all these
checks into the kernel is questionable at best, and, if enough people
insist on it, should be discussed separately.

Thanks,

		Ilya
--
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

Miao Xie

2012-Sep-25 10:24 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

On 	mon, 24 Sep 2012 21:42:42 +0300, Ilya Dryomov wrote:> On Mon, Sep 24, 2012 at 06:47:42PM +0200, David Sterba wrote:
>> On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote:
>>> Generally, we should check the value when it is input. If not, we
might
>>> run our program with the wrong value, and it is possible to cause
unknown
>>> problems. So I think the above chunk is necessary.
>>
>> The difference is that giving an invalid value will exit early (your
>> version), while Ilya''s version will clamp the
''usage'' to the allowed
>> range during processing. From usability POV, I''d prefer to let
the
>> command fail early with a verbose error eg. that the range is invalid.
> 
> I think that''s the job for progs, and that''s where this
and a few other
> checks are currently implemented.
> 
> There is no possibility for "unknown problems", that''s
exactly how it''s
> been implemented before the cleanup.  The purpose of balance filters
> (and the usage filter in particular) is to lower the number of chunks we
> have to relocate.  If someone decides to use raw ioctls, and supplies us
> with the invalid value, we just cut it down to a 100 and proceed.
> usage=100 is the equivalent of not using the usage filter at all, so the
> raw ioctl user will get what he deserves.
> 
> This is very similar to what we currently do with other filters.  For
> example, "soft" can only be used with "convert", and
this is checked by
> progs.  But, if somebody were to set a "soft" flag without
setting
> "convert" we would simply ignore that "soft".  Same
goes for "drange"
> and "devid", invalid ranges, invalid devids, etc.  Pulling all
these
> checks into the kernel is questionable at best, and, if enough people
> insist on it, should be discussed separately.
I think the usage is a special case that doesn''t cause critical problem
and
are not used everywhere. But if there is a variant which is referenced at
several places and the kernel would crash if it is invalid, in this case,
we would add the check everywhere and make the code more complex and ugly
if we don''t check it when it is passed in. Besides that if we have done
lots of work before the check, we must roll back when we find the variant
is invalid, it wastes time. So I think doing the check and returning the
error number as early as possible is a rule we should follow.

Thanks
Miao
--
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

Miao Xie

2012-Sep-27 10:15 UTC

head link

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Sorry to reply late.

On Mon, 24 Sep 2012 18:47:42 +0200, David Sterba wrote:>>> This is the most straightforward transformation I can think of.  It
>>> doesn''t result in an unnecessary BUG_ON, keeps churn to a
minimum and
>>
>> agree with you.
>>
>>> doesn''t change the "style" of the balance ioctl.
(If I were to check
>>> every filter argument that way, btrfs_balance_ioctl() would be very
long
>>> and complicated.)
>>
>> I think the check in btrfs_balance_ioctl() is necessary, the reason is
above.
> 
> btrfs_balance_ioctl does not seem as the right place, it does the
> processing related to the state of balance (resume/cancel etc). Look at
> btrfs_balance() itself, it does lot more sanity checks of the parameters
I think we should not put the check in btrfs_balance(), because the arguments
are valid forever if they pass the check when they are input, if we put the
check in btrfs_balance(), the check will be done every time we resume the
balance.
it is unnecessary.
> We can put the extra checks into helpers (and not only this
> one) if clarity and readability of the function becomes a concern.
Agree. I will put this check into a helper in the next version of this patch.
And I will make a separate patch to move the current check in btrfs_balance
from btrfs_balance to the above helper after this patch is received.

Thanks
Miao
--
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

Miao Xie

2012-Sep-27 10:19 UTC

head link

[PATCH V4 1/2] Btrfs: cleanup duplicated division functions

div_factor{_fine} has been implemented for two times, and these two functions
are very similar, so cleanup the reduplicate implement and drop the original
div_factor(), and then rename div_factor_fine() to div_factor(). So the factor
of the new div_factor() is 100, not 10.

And I move div_factor into a independent file named math.h because it is a
common math function, may be used by every composition of btrfs.

Because these functions are mostly used on the hot path, and we are sure
the parameters are right in the most cases, we don''t add complex checks
for the parameters. But in the other place, we must check and make sure
the parameters are right. So besides the code cleanup, this patch also
add a check for the usage of the space balance, it is the only place that
we need add check to make sure the parameters of div_factor are right till
now. Besides that, the old kernel may hold the wrong usage value, so we
must rectify it.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v3 -> v4:
- deal with the wrong usage that was input on the old kernel

Changelog v2 -> v3:
- drop the original div_factor and rename div_factor_fine to div_factor
- drop the check of the factor

Changelog v1 -> v2:
- add missing check
---
 fs/btrfs/extent-tree.c |   29 +++++--------------------
 fs/btrfs/ioctl.c       |   21 ++++++++++++++++++
 fs/btrfs/math.h        |   35 ++++++++++++++++++++++++++++++
 fs/btrfs/relocation.c  |    2 +-
 fs/btrfs/transaction.c |    2 +-
 fs/btrfs/volumes.c     |   55 ++++++++++++++++++++++++++---------------------
 6 files changed, 94 insertions(+), 50 deletions(-)
 create mode 100644 fs/btrfs/math.h

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..bcb9ced 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
 #include "volumes.h"
 #include "locking.h"
 #include "free-space-cache.h"
+#include "math.h"
 
 #undef SCRAMBLE_DELAYED_REFS
 
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
 	rcu_read_unlock();
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor == 100)
-		return num;
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 u64 btrfs_find_block_group(struct btrfs_root *root,
 			   u64 search_start, u64 search_hint, int owner)
 {
@@ -674,7 +657,7 @@ u64 btrfs_find_block_group(struct btrfs_root *root,
 	u64 last = max(search_hint, search_start);
 	u64 group_start = 0;
 	int full_search = 0;
-	int factor = 9;
+	int factor = 90;
 	int wrapped = 0;
 again:
 	while (1) {
@@ -708,7 +691,7 @@ again:
 	if (!full_search && factor < 10) {
 		last = search_start;
 		full_search = 1;
-		factor = 10;
+		factor = 100;
 		goto again;
 	}
 found:
@@ -3513,7 +3496,7 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	if (force == CHUNK_ALLOC_LIMITED) {
 		thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 		thresh = max_t(u64, 64 * 1024 * 1024,
-			       div_factor_fine(thresh, 1));
+			       div_factor(thresh, 1));
 
 		if (num_bytes - num_allocated < thresh)
 			return 1;
@@ -3521,12 +3504,12 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 
 	/* 256MB or 2% of the FS */
-	thresh = max_t(u64, 256 * 1024 * 1024, div_factor_fine(thresh, 2));
+	thresh = max_t(u64, 256 * 1024 * 1024, div_factor(thresh, 2));
 	/* system chunks need a much small threshold */
 	if (sinfo->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		thresh = 32 * 1024 * 1024;
 
-	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 8))
+	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 80))
 		return 0;
 	return 1;
 }
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9384a2a..121339c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3297,6 +3297,23 @@ void update_ioctl_balance_args(struct btrfs_fs_info
*fs_info, int lock,
 	}
 }
 
+static int btrfs_check_balance_args(struct btrfs_ioctl_balance_args *bargs)
+{
+	if ((bargs->data.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+	    (bargs->data.usage < 0 || bargs->data.usage > 100))
+		return -EINVAL;
+
+	if ((bargs->meta.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+	    (bargs->meta.usage < 0 || bargs->meta.usage > 100))
+		return -EINVAL;
+
+	if ((bargs->sys.flags & BTRFS_BALANCE_ARGS_USAGE) &&
+	    (bargs->sys.usage < 0 || bargs->sys.usage > 100))
+		return -EINVAL;
+
+	return 0;
+}
+
 static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 {
 	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
@@ -3335,6 +3352,10 @@ static long btrfs_ioctl_balance(struct file *file, void
__user *arg)
 
 			goto do_balance;
 		}
+
+		ret = btrfs_check_balance_args(bargs);
+		if (ret)
+			goto out_bargs;
 	} else {
 		bargs = NULL;
 	}
diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
new file mode 100644
index 0000000..4fef49f
--- /dev/null
+++ b/fs/btrfs/math.h
@@ -0,0 +1,35 @@
+
+/*
+ * Copyright (C) 2012 Fujitsu.  All rights reserved.
+ * Written by Miao Xie <miaox@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef __BTRFS_MATH_H
+#define __BTRFS_MATH_H
+
+#include <asm/div64.h>
+
+static inline u64 div_factor(u64 num, int factor)
+{
+	WARN_ON(factor > 100 || factor < 0);
+
+	num *= factor;
+	do_div(num, 100);
+	return num;
+}
+
+#endif
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f193096..2254478 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3813,7 +3813,7 @@ restart:
 			}
 		}
 
-		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 5);
+		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 50);
 		if (ret < 0) {
 			if (ret != -ENOSPC) {
 				err = ret;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index cf98dbc..115f054 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -489,7 +489,7 @@ static int should_end_transaction(struct btrfs_trans_handle
*trans,
 {
 	int ret;
 
-	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
5);
+	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
50);
 	return ret ? 1 : 0;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f4e70e..dc25431 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
-#include <asm/div64.h>
 #include "compat.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -36,6 +35,7 @@
 #include "async-thread.h"
 #include "check-integrity.h"
 #include "rcu-string.h"
+#include "math.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
 	return 1;
 }
 
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor <= 0)
-		return 0;
-	if (factor >= 100)
-		return num;
-
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
 			      struct btrfs_balance_args *bargs)
 {
@@ -2347,7 +2335,7 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
 	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
 	chunk_used = btrfs_block_group_used(&cache->item);
 
-	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
+	user_thresh = div_factor(cache->key.offset, bargs->usage);
 	if (chunk_used < user_thresh)
 		ret = 0;
 
@@ -2501,15 +2489,6 @@ static int should_balance_chunk(struct btrfs_root *root,
 	return 1;
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
 static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
@@ -2534,7 +2513,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	devices = &fs_info->fs_devices->devices;
 	list_for_each_entry(device, devices, dev_list) {
 		old_size = device->total_bytes;
-		size_to_free = div_factor(old_size, 1);
+		size_to_free = div_factor(old_size, 10);
 		size_to_free = min(size_to_free, (u64)1 * 1024 * 1024);
 		if (!device->writeable ||
 		    device->total_bytes - device->bytes_used > size_to_free)
@@ -2939,6 +2918,32 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 	btrfs_balance_sys(leaf, item, &disk_bargs);
 	btrfs_disk_balance_args_to_cpu(&bctl->sys, &disk_bargs);
 
+	/*
+	 * Now we check the balanace argument to make sure the usage is in
+	 * the range [0, 100], but the old kernel didn''t do it, it is
possible
+	 * that the users might input the wrong usage, we must rectify it.
+	 */
+	if (bctl->data.flags & BTRFS_BALANCE_ARGS_USAGE) {
+		if (bctl->data.usage < 0)
+			bctl->data.usage = 0;
+		if (bctl->data.usage > 100)
+			bctl->data.usage = 100;
+	}
+
+	if (bctl->meta.flags & BTRFS_BALANCE_ARGS_USAGE) {
+		if (bctl->meta.usage < 0)
+			bctl->meta.usage = 0;
+		if (bctl->meta.usage > 100)
+			bctl->meta.usage = 100;
+	}
+
+	if (bctl->sys.flags & BTRFS_BALANCE_ARGS_USAGE) {
+		if (bctl->sys.usage < 0)
+			bctl->sys.usage = 0;
+		if (bctl->sys.usage > 100)
+			bctl->sys.usage = 100;
+	}
+
 	mutex_lock(&fs_info->volume_mutex);
 	mutex_lock(&fs_info->balance_mutex);
 
@@ -3284,7 +3289,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle
*trans,
 	}
 
 	/* we don''t want a chunk larger than 10% of writeable space */
-	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
+	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 10),
 			     max_chunk_size);
 
 	devices_info = kzalloc(sizeof(*devices_info) * fs_devices->rw_devices,
-- 
1.7.6.5

--
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

Ilya Dryomov

2012-Sep-27 16:56 UTC

head link

Re: [PATCH V4 1/2] Btrfs: cleanup duplicated division functions

Hi Miao,

You haven''t addressed any of my concerns with v3.

On Thu, Sep 27, 2012 at 06:19:58PM +0800, Miao Xie wrote:

(snipped)
> the parameters are right. So besides the code cleanup, this patch also
> add a check for the usage of the space balance, it is the only place that
> we need add check to make sure the parameters of div_factor are right till
> now. Besides that, the old kernel may hold the wrong usage value, so we
> must rectify it.
Cleaning up/unifying duplicated functions and changing the existing
logic are two very different things.  If you, in the course of writing
this patch, became unhappy with the way balancing ioctl deals with
"invalid" input, please send a separate patch.

Before your patch, volumes.c had its own copy of div_factor_fine():

static u64 div_factor_fine(u64 num, int factor)
{
	if (factor <= 0)
		return 0;
	if (factor >= 100)
		return num;

	num *= factor;
	do_div(num, 100);
	return num;
}

which was called from chunk_usage_filter() on unvalidated user input.
As far as the cleanup part of your patch goes, you''ve dropped
factor <= 0 / factor >= 100 logic, merged volumes.c''s copy with
extent-tree.c''s copy and renamed div_factor_fine() to div_factor().  To
make chunk_usage_filter() happy again, it''s enough to move the dropped
logic directly to the call site:

static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
			      struct btrfs_balance_args *bargs)
{
	...

-	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
+	if (bargs->usage == 0)
+		user_thresh = 0;
+	else if (bargs->usage >= 100)
+		user_thresh = cache->key.offset;
+	else
+		user_thresh = div_factor(cache->key.offset, bargs->usage);

	...
}

So I would suggest you drop all hunks related to changing the way
balancing ioctl works and make the above change to chunk_usage_filter()
instead.  Once again, if you are unhappy with usage filter argument
handling, send a separate patch.

Thanks,

		Ilya
--
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

Miao Xie

2012-Sep-28 01:30 UTC

head link

Re: [PATCH V4 1/2] Btrfs: cleanup duplicated division functions

On thu, 27 Sep 2012 19:56:24 +0300, Ilya Dryomov wrote:>> the parameters are right. So besides the code cleanup, this patch also
>> add a check for the usage of the space balance, it is the only place
that
>> we need add check to make sure the parameters of div_factor are right
till
>> now. Besides that, the old kernel may hold the wrong usage value, so we
>> must rectify it.
> 
> Cleaning up/unifying duplicated functions and changing the existing
> logic are two very different things.  If you, in the course of writing
> this patch, became unhappy with the way balancing ioctl deals with
> "invalid" input, please send a separate patch.
> 
> Before your patch, volumes.c had its own copy of div_factor_fine():
> 
> static u64 div_factor_fine(u64 num, int factor)
> {
> 	if (factor <= 0)
> 		return 0;
> 	if (factor >= 100)
> 		return num;
> 
> 	num *= factor;
> 	do_div(num, 100);
> 	return num;
> }
> 
> which was called from chunk_usage_filter() on unvalidated user input.
> As far as the cleanup part of your patch goes, you''ve dropped
> factor <= 0 / factor >= 100 logic, merged volumes.c''s copy
with
> extent-tree.c''s copy and renamed div_factor_fine() to
div_factor().  To
> make chunk_usage_filter() happy again, it''s enough to move the
dropped
> logic directly to the call site:
> 
> static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64
chunk_offset,
> 			      struct btrfs_balance_args *bargs)
> {
> 	...
> 
> -	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
> +	if (bargs->usage == 0)
> +		user_thresh = 0;
> +	else if (bargs->usage >= 100)
> +		user_thresh = cache->key.offset;
> +	else
> +		user_thresh = div_factor(cache->key.offset, bargs->usage);
> 
> 	...
> }
> 
> So I would suggest you drop all hunks related to changing the way
> balancing ioctl works and make the above change to chunk_usage_filter()
> instead.  Once again, if you are unhappy with usage filter argument
> handling, send a separate patch.
Fine.
(I forget the rule that one patch just do one thing)

Thanks
Miao
--
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

Miao Xie

2012-Sep-28 01:49 UTC

head link

[PATCH V5 1/2] Btrfs: cleanup duplicated division functions

div_factor{_fine} has been implemented for two times, and these two functions
are very similar, so cleanup the reduplicate implement and drop the original
div_factor(), and then rename div_factor_fine() to div_factor(). So the factor
of the new div_factor() is 100, not 10.

And I move div_factor into a independent file named math.h because it is a
common math function, may be used by every composition of btrfs.

Because these functions are mostly used on the hot path, and we are sure
the parameters are right in the most cases, we don''t add complex checks
for the parameters. But in the other place, we must check and make sure
the parameters are right. So besides the code cleanup, this patch also
add a check for the usage of the space balance, it is the only place that
we need add check to make sure the parameters of div_factor are right till now.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v4 -> v5:
- drop the check in the space balance, and make the churn to a minimum

Changelog v3 -> v4:
- deal with the wrong usage that was input on the old kernel

Changelog v2 -> v3:
- drop the original div_factor and rename div_factor_fine to div_factor
- drop the check of the factor

Changelog v1 -> v2:
- add missing check
---
 fs/btrfs/extent-tree.c |   29 ++++++-----------------------
 fs/btrfs/math.h        |   35 +++++++++++++++++++++++++++++++++++
 fs/btrfs/relocation.c  |    2 +-
 fs/btrfs/transaction.c |    2 +-
 fs/btrfs/volumes.c     |   35 ++++++++++-------------------------
 5 files changed, 53 insertions(+), 50 deletions(-)
 create mode 100644 fs/btrfs/math.h

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..bcb9ced 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
 #include "volumes.h"
 #include "locking.h"
 #include "free-space-cache.h"
+#include "math.h"
 
 #undef SCRAMBLE_DELAYED_REFS
 
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
 	rcu_read_unlock();
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor == 100)
-		return num;
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 u64 btrfs_find_block_group(struct btrfs_root *root,
 			   u64 search_start, u64 search_hint, int owner)
 {
@@ -674,7 +657,7 @@ u64 btrfs_find_block_group(struct btrfs_root *root,
 	u64 last = max(search_hint, search_start);
 	u64 group_start = 0;
 	int full_search = 0;
-	int factor = 9;
+	int factor = 90;
 	int wrapped = 0;
 again:
 	while (1) {
@@ -708,7 +691,7 @@ again:
 	if (!full_search && factor < 10) {
 		last = search_start;
 		full_search = 1;
-		factor = 10;
+		factor = 100;
 		goto again;
 	}
 found:
@@ -3513,7 +3496,7 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	if (force == CHUNK_ALLOC_LIMITED) {
 		thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 		thresh = max_t(u64, 64 * 1024 * 1024,
-			       div_factor_fine(thresh, 1));
+			       div_factor(thresh, 1));
 
 		if (num_bytes - num_allocated < thresh)
 			return 1;
@@ -3521,12 +3504,12 @@ static int should_alloc_chunk(struct btrfs_root *root,
 	thresh = btrfs_super_total_bytes(root->fs_info->super_copy);
 
 	/* 256MB or 2% of the FS */
-	thresh = max_t(u64, 256 * 1024 * 1024, div_factor_fine(thresh, 2));
+	thresh = max_t(u64, 256 * 1024 * 1024, div_factor(thresh, 2));
 	/* system chunks need a much small threshold */
 	if (sinfo->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		thresh = 32 * 1024 * 1024;
 
-	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 8))
+	if (num_bytes > thresh && sinfo->bytes_used <
div_factor(num_bytes, 80))
 		return 0;
 	return 1;
 }
diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
new file mode 100644
index 0000000..4fef49f
--- /dev/null
+++ b/fs/btrfs/math.h
@@ -0,0 +1,35 @@
+
+/*
+ * Copyright (C) 2012 Fujitsu.  All rights reserved.
+ * Written by Miao Xie <miaox@cn.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#ifndef __BTRFS_MATH_H
+#define __BTRFS_MATH_H
+
+#include <asm/div64.h>
+
+static inline u64 div_factor(u64 num, int factor)
+{
+	WARN_ON(factor > 100 || factor < 0);
+
+	num *= factor;
+	do_div(num, 100);
+	return num;
+}
+
+#endif
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f193096..2254478 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3813,7 +3813,7 @@ restart:
 			}
 		}
 
-		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 5);
+		ret = btrfs_block_rsv_check(rc->extent_root, rc->block_rsv, 50);
 		if (ret < 0) {
 			if (ret != -ENOSPC) {
 				err = ret;
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index cf98dbc..115f054 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -489,7 +489,7 @@ static int should_end_transaction(struct btrfs_trans_handle
*trans,
 {
 	int ret;
 
-	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
5);
+	ret = btrfs_block_rsv_check(root, &root->fs_info->global_block_rsv,
50);
 	return ret ? 1 : 0;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f4e70e..763edc4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
-#include <asm/div64.h>
 #include "compat.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -36,6 +35,7 @@
 #include "async-thread.h"
 #include "check-integrity.h"
 #include "rcu-string.h"
+#include "math.h"
 
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
@@ -2325,18 +2325,6 @@ static int chunk_profiles_filter(u64 chunk_type,
 	return 1;
 }
 
-static u64 div_factor_fine(u64 num, int factor)
-{
-	if (factor <= 0)
-		return 0;
-	if (factor >= 100)
-		return num;
-
-	num *= factor;
-	do_div(num, 100);
-	return num;
-}
-
 static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset,
 			      struct btrfs_balance_args *bargs)
 {
@@ -2347,7 +2335,13 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
 	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
 	chunk_used = btrfs_block_group_used(&cache->item);
 
-	user_thresh = div_factor_fine(cache->key.offset, bargs->usage);
+	if (bargs->usage <= 0)
+		user_thresh = 0;
+	else if (bargs->usage >= 100)
+		user_thresh = cache->key.offset;
+	else
+		user_thresh = div_factor(cache->key.offset, bargs->usage);
+
 	if (chunk_used < user_thresh)
 		ret = 0;
 
@@ -2501,15 +2495,6 @@ static int should_balance_chunk(struct btrfs_root *root,
 	return 1;
 }
 
-static u64 div_factor(u64 num, int factor)
-{
-	if (factor == 10)
-		return num;
-	num *= factor;
-	do_div(num, 10);
-	return num;
-}
-
 static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
@@ -2534,7 +2519,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	devices = &fs_info->fs_devices->devices;
 	list_for_each_entry(device, devices, dev_list) {
 		old_size = device->total_bytes;
-		size_to_free = div_factor(old_size, 1);
+		size_to_free = div_factor(old_size, 10);
 		size_to_free = min(size_to_free, (u64)1 * 1024 * 1024);
 		if (!device->writeable ||
 		    device->total_bytes - device->bytes_used > size_to_free)
@@ -3284,7 +3269,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle
*trans,
 	}
 
 	/* we don''t want a chunk larger than 10% of writeable space */
-	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
+	max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 10),
 			     max_chunk_size);
 
 	devices_info = kzalloc(sizeof(*devices_info) * fs_devices->rw_devices,
-- 
1.7.6.5

--
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

David Sterba

2012-Sep-28 10:09 UTC

head link

Re: [PATCH V5 1/2] Btrfs: cleanup duplicated division functions

On Fri, Sep 28, 2012 at 09:49:13AM +0800, Miao Xie
wrote:> diff --git a/fs/btrfs/math.h b/fs/btrfs/math.h
> new file mode 100644
> index 0000000..4fef49f
> --- /dev/null
> +++ b/fs/btrfs/math.h
> @@ -0,0 +1,35 @@
> +
> +/*
> + * Copyright (C) 2012 Fujitsu.  All rights reserved.
> + * Written by Miao Xie <miaox@cn.fujitsu.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; if not, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#ifndef __BTRFS_MATH_H
> +#define __BTRFS_MATH_H
> +
> +#include <asm/div64.h>
> +
> +static inline u64 div_factor(u64 num, int factor)
> +{
> +	WARN_ON(factor > 100 || factor < 0);
> +
> +	num *= factor;
> +	do_div(num, 100);
> +	return num;
> +}
> +
> +#endif
Sorry no, a 4 line function does not deserve a separate file.

david
--
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

Btrfs devel - Sep 2012 - [PATCH 1/2] Btrfs: cleanup duplicated division functions

[PATCH 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH 1/2] Btrfs: cleanup duplicated division functions

[PATCH V2 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V2 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V2 1/2] Btrfs: cleanup duplicated division functions

[PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions

[PATCH V4 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V4 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V4 1/2] Btrfs: cleanup duplicated division functions

[PATCH V5 1/2] Btrfs: cleanup duplicated division functions

Re: [PATCH V5 1/2] Btrfs: cleanup duplicated division functions