Re: [PATCH 1/2] btrfs: inode: Don't compress if NODATASUM or NODATACOW set

[David Sterba <dsterba@suse.cz>]

On Fri, Jun 28, 2019 at 08:09:46PM +0800, Qu Wenruo wrote:
> 
> 
> 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?

Yes.