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

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

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