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

[Ilya Dryomov <idryomov@gmail.com>]

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