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 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.