diff mbox

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

Message ID 5064284E.9020504@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miao Xie Sept. 27, 2012, 10:19 a.m. UTC
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

Comments

Ilya Dryomov Sept. 27, 2012, 4:56 p.m. UTC | #1
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 Sept. 28, 2012, 1:30 a.m. UTC | #2
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
diff mbox

Patch

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,