Re: [PATCH v3] Btrfs: add skeleton code for compression heuristic

[Anand Jain <anand.jain@oracle.com>]

On 07/18/2017 02:30 AM, David Sterba wrote:
> So it basically looks good, I could not resist and rewrote the changelog
> and comments. There's one code fix:
> 
> On Mon, Jul 17, 2017 at 04:52:58PM +0300, Timofey Titovets wrote:
>> -static inline int inode_need_compress(struct inode *inode)
>> +static inline int inode_need_compress(struct inode *inode, u64 start, u64 end)
>>   {
>>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>
>>   	/* force compress */
>>   	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
>> -		return 1;
>> +		return btrfs_compress_heuristic(inode, start, end);



> This must stay 'return 1', if force-compress is on, so the change is
> reverted.

  Initially I thought 'return 1' is correct, but looking in depth,
  it is not correct as below..

  The biggest beneficiary of the estimating the compression ratio
  in advance (heuristic) is when customers are using the
  -o compress-force. But 'return 1' here is making them not to
  use heuristic. So definitely something is wrong.

  -o compress is about the whether each of the compression-granular bytes
  (BTRFS_MAX_UNCOMPRESSED) of the inode should be tried to compress OR
  just give up for the whole inode by looking at the compression ratio
  of the current compression-granular.
  This approach can be overridden by -o compress-force. So in
  -o compress-force there will be a lot more efforts in _trying_
  to compression than in -o compress. We must use heuristic for
  -o compress-force.

  btrfs_compress_heuristic is about the way of figuring out
  whether the current compression-granular is compression capable.
  Currently we are doing this part by trial-method (which is more
  accurate than the heuristic approach), in the function
  btrfs_compress_pages() and then we giving up in the function
  compress_file_range(). So IMO. btrfs_compress_heuristic() should
  be called at these functions.

  Further, heuristic is just estimating which may go wrong based
  on future compression algorithm ? (I am not sure).

  IMO. its a good idea to either add an option to enable/disable
  the heuristic.

Thanks, Anand



> I'm adding the patch to for-next.
> 
>>   	/* bad compression ratios */
>>   	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
>>   		return 0;
>>   	if (btrfs_test_opt(fs_info, COMPRESS) ||
>>   	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
>>   	    BTRFS_I(inode)->force_compress)
>> -		return 1;
>> +		return btrfs_compress_heuristic(inode, start, end);
>>   	return 0;
>>   }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>