[PATCH] btrfs: make use of inode_need_compress()

[PATCH] btrfs: make use of inode_need_compress()

[Anand Jain]

Its better to have the policy enforcement going through a function,
so that we have better control and visibility of the decision logic.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/inode.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 06dea7c89bbd..d0cc3de120b7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1189,11 +1189,10 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 		async_cow->locked_page = locked_page;
 		async_cow->start = start;
 
-		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
-		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
-			cur_end = end;
-		else
+		if (inode_need_compress(inode))
 			cur_end = min(end, start + SZ_512K - 1);
+		else
+			cur_end = end;
 
 		async_cow->end = cur_end;
 		INIT_LIST_HEAD(&async_cow->extents);
-- 
2.13.1

Re: [PATCH] btrfs: make use of inode_need_compress()

[David Sterba]

On Tue, Jul 18, 2017 at 05:37:47PM +0800, Anand Jain wrote:
> Its better to have the policy enforcement going through a function,
> so that we have better control and visibility of the decision logic.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/inode.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 06dea7c89bbd..d0cc3de120b7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1189,11 +1189,10 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>  		async_cow->locked_page = locked_page;
>  		async_cow->start = start;
>  
> -		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> -		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
> -			cur_end = end;
> -		else
> +		if (inode_need_compress(inode))
>  			cur_end = min(end, start + SZ_512K - 1);
> +		else
> +			cur_end = end;

The opencoded test should be cleaned up, however cow_file_range_async is
called from run_delalloc_range if the inode_need_compress passes the
'inode_need_compress' condition. So at minimum, checking again here is
redundant, but we still might need to know if the compression is
desired.

The check does no depend on anything inside the loop, so it should be
moved out of it.  If I understand the logic correctly, we get to
cow_file_range_async and always want to compress, so the test should be
dropped entirely.

Re: [PATCH] btrfs: make use of inode_need_compress()

[Anand Jain]

On 07/18/2017 11:01 PM, David Sterba wrote:
> On Tue, Jul 18, 2017 at 05:37:47PM +0800, Anand Jain wrote:
>> Its better to have the policy enforcement going through a function,
>> so that we have better control and visibility of the decision logic.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/inode.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 06dea7c89bbd..d0cc3de120b7 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1189,11 +1189,10 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
>>   		async_cow->locked_page = locked_page;
>>   		async_cow->start = start;
>>   
>> -		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
>> -		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
>> -			cur_end = end;
>> -		else
>> +		if (inode_need_compress(inode))
>>   			cur_end = min(end, start + SZ_512K - 1);
>> +		else
>> +			cur_end = end;
> 
> The opencoded test should be cleaned up, however cow_file_range_async is
> called from run_delalloc_range if the inode_need_compress passes the
> 'inode_need_compress' condition. So at minimum, checking again here is
> redundant,

  David, no its not redundant. If the preceding SZ_512K block is not
  compressible, then we would have already set the BTRFS_INODE_NOCOMPRESS
  flag, which then we would straightaway go up to the end.

> but we still might need to know if the compression is
> desired.
  Uh ?

> The check does no depend on anything inside the loop, so it should be
> moved out of it. 

  No, it should not be, if you move check outside then it will act as if
  compress-force is set even if you don't set it.

> If I understand the logic correctly, we get to
> cow_file_range_async and always want to compress, so the test should be
> dropped entirely.

  sorry. its wrong. as above.

Re: [PATCH] btrfs: make use of inode_need_compress()

[David Sterba]

On Thu, Jul 20, 2017 at 10:22:13AM +0800, Anand Jain wrote:
> >> @@ -1189,11 +1189,10 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
> >>   		async_cow->locked_page = locked_page;
> >>   		async_cow->start = start;
> >>   
> >> -		if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS &&
> >> -		    !btrfs_test_opt(fs_info, FORCE_COMPRESS))
> >> -			cur_end = end;
> >> -		else
> >> +		if (inode_need_compress(inode))
> >>   			cur_end = min(end, start + SZ_512K - 1);
> >> +		else
> >> +			cur_end = end;
> > 
> > The opencoded test should be cleaned up, however cow_file_range_async is
> > called from run_delalloc_range if the inode_need_compress passes the
> > 'inode_need_compress' condition. So at minimum, checking again here is
> > redundant,
> 
>   David, no its not redundant. If the preceding SZ_512K block is not
>   compressible, then we would have already set the BTRFS_INODE_NOCOMPRESS
>   flag, which then we would straightaway go up to the end.

Right, I missed that it's using 'start', that changes in the loop and
that NOCOMPRESS can get set indirectly through the async write between
the iterations.

I'm still counting too many times where the expensive "should we
compress" check can get triggerd. It's at least once before we go to
cow_file_range_async and then again for each compressed range in
compress_file_range. I think we can fix that by two modes of
inode_need_compres or separate the lightweight and expensive checks.