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

[Miao Xie <miaox@cn.fujitsu.com>]

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