On 2019/6/28 下午7:34, David Sterba wrote:
> On Fri, Jun 28, 2019 at 09:26:53AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/6/27 下午10:58, David Sterba wrote:
>>> On Tue, Jun 25, 2019 at 04:24:57PM +0800, Qu Wenruo wrote:
>>>> Ping?
>>>>
>>>> This patch should fix the problem of compressed extent even when
>>>> nodatasum is set.
>>>>
>>>> It has been one year but we still didn't get a conclusion on where
>>>> force_compress should behave.
>>>
>>> Note that pings to patches sent year ago will get lost, I noticed only
>>> because you resent it and I remembered that we had some discussions,
>>> without conclusions.
>>>
>>>> But at least to me, NODATASUM is a strong exclusion for compress, no
>>>> matter whatever option we use, we should NEVER compress data without
>>>> datasum/datacow.
>>>
>>> That's correct, but the way you fix it is IMO not right. This was also
>>> noticed by Nikolay, that there are 2 locations that call
>>> inode_need_compress but with different semantics.
>>>
>>> One is the decision if compression applies at all,
>>
>>> and the second one
>>> when that's certain it's compression, to do it or not based on the
>>> status decision of eg. heuristics.
>>
>> The second call is in compress_file_extent(), with inode_need_compress()
>> return 0 for NODATACOW/NODATASUM inodes, we will not go into
>> cow_file_range_async() branch at all.
>>
>> So would you please explain how this could cause problem?
>> To me, prevent the problem in inode_need_compress() is the safest location.
> 
> Let me repeat: two places with different semantics. So this means that
> we need two functions that reflect the differences. That it's in one
> function that works both contexts is ok from functionality point of
> view, but if we care about clarity of design and code we want two
> functions.
>

OK, so in next version I'll split the inode_need_compress() into two
functions for different semantics:
- inode_can_compress()
  The hard requirement for compress code. E.g. COW and checksum checks.
- inode_need_compress()
  The soft requirement, for things like ratio, force_compress checks.

Will this modification be fine?

Thanks,
Qu