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.
