| Message ID | 5051BAB8.7080200@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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;
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