2017-07-03 20:30 GMT+03:00 David Sterba <dste...@suse.cz>:
> On Sat, Jul 01, 2017 at 07:56:02PM +0300, Timofey Titovets wrote:
>> Add a heuristic computation before compression,
>> for avoiding load resource heavy compression workspace,
>> if data are probably can't be compressed
>>
>> Signed-off-by: Timofey Titovets <nefelim...@gmail.com>
>> --- /dev/null
>> +++ b/fs/btrfs/heuristic.c
>
> For the new heuristic-related code, please use the compression.h and .c
> files and don't add a new one. When we're going to add the clever
> heuristics, that could go to a separate file.
Okay.
>> @@ -0,0 +1,336 @@
>> +enum compression_advice btrfs_compress_heuristic(struct inode *inode,
>
> This returns enum but the caller treats it as a bool, this should be
> synced up.
>
I think for now, enum and values can be just dropped,
because it's only a proof of concept that in future, heuristic
can return more complex answer and then some of that values
can be used as acceptable for zlib and unnacceptable for lzo & etc.
Also that can be easy added again later.
>
> As mentioned in the previous mail, we'll add the skeleton code only for
> now, which means this loop, that simply calls find_get_page, kunmap and
> put_page.
>
So, i will send a separate patch for skeleton code.
And if i understood all correctly, later, we'll add complex code one by one.
>> +
>> +enum compression_advice {
>> + COMPRESS_NONE,
>> + COMPRESS_COST_UNKNOWN,
>> + COMPRESS_COST_EASY,
>> + COMPRESS_COST_MEDIUM,
>> + COMPRESS_COST_HARD
>
> I don't find the naming good, as the cost is not easy or hard, but
> rather low/medium/high. Or you can rename the whole prefix if you find a
> better naming scheme.
(Explain above)
So just drop that for now.
About naming scheme, at least i preffer to use numbers, something like:
COMPRESS_COST_INF, // as false
COMPRESS_COST_1,
COMPRESS_COST_2,
Because it's very dependence on the step when code make a decision, and
code can't directly detect how many cpu time will be used for that data.
>> struct btrfs_key *location;
>> @@ -458,16 +459,16 @@ static noinline void compress_file_range(struct inode
>> *inode,
>> unsigned long total_compressed = 0;
>> unsigned long total_in = 0;
>> int i;
>> - int will_compress;
>> + bool will_compress;
>
> Please remove this change.
>
>> int compress_type = fs_info->compress_type;
>> - int redirty = 0;
>> + bool redirty = false;
>
> And this.
>
Okay, i will drop bool changes,
Thank you very match!
--
Have a nice day,
Timofey.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
