[PATCH v4] Btrfs: compress_file_range() change page dirty status once

[PATCH v4] Btrfs: compress_file_range() change page dirty status once

[Timofey Titovets]

We need to call extent_range_clear_dirty_for_io()
on compression range to prevent application from changing
page content, while pages compressing.

extent_range_clear_dirty_for_io() run on each loop iteration,
"(end - start)" can be much (up to 1024 times) bigger
then compression range (BTRFS_MAX_UNCOMPRESSED).

That produce extra calls to page managment code.

Fix that behaviour by call extent_range_clear_dirty_for_io()
only once.

v1 -> v2:
 - Make that more obviously and more safeprone

v2 -> v3:
 - Rebased on:
   Btrfs: compress_file_range() remove dead variable num_bytes
 - Update change log
 - Add comments

v3 -> v4:
 - Rebased on: kdave for-next
 - To avoid dirty bit clear/set behaviour change
   call clear_bit once, istead of per compression range

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b93fe05a39c7..5816dd3cb6e6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -536,8 +536,10 @@ static noinline void compress_file_range(struct inode *inode,
 		 * If the compression fails for any reason, we set the pages
 		 * dirty again later on.
 		 */
-		extent_range_clear_dirty_for_io(inode, start, end);
-		redirty = 1;
+		if (!redirty) {
+			extent_range_clear_dirty_for_io(inode, start, end);
+			redirty = 1;
+		}

 		/* Compression level is applied here and only here */
 		ret = btrfs_compress_pages(
--
2.14.2

Re: [PATCH v4] Btrfs: compress_file_range() change page dirty status once

[Timofey Titovets]

Gentle ping

2017-10-24 1:29 GMT+03:00 Timofey Titovets <nefelim4ag@gmail.com>:
> We need to call extent_range_clear_dirty_for_io()
> on compression range to prevent application from changing
> page content, while pages compressing.
>
> extent_range_clear_dirty_for_io() run on each loop iteration,
> "(end - start)" can be much (up to 1024 times) bigger
> then compression range (BTRFS_MAX_UNCOMPRESSED).
>
> That produce extra calls to page managment code.
>
> Fix that behaviour by call extent_range_clear_dirty_for_io()
> only once.
>
> v1 -> v2:
>  - Make that more obviously and more safeprone
>
> v2 -> v3:
>  - Rebased on:
>    Btrfs: compress_file_range() remove dead variable num_bytes
>  - Update change log
>  - Add comments
>
> v3 -> v4:
>  - Rebased on: kdave for-next
>  - To avoid dirty bit clear/set behaviour change
>    call clear_bit once, istead of per compression range
>
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> ---
>  fs/btrfs/inode.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b93fe05a39c7..5816dd3cb6e6 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -536,8 +536,10 @@ static noinline void compress_file_range(struct inode *inode,
>                  * If the compression fails for any reason, we set the pages
>                  * dirty again later on.
>                  */
> -               extent_range_clear_dirty_for_io(inode, start, end);
> -               redirty = 1;
> +               if (!redirty) {
> +                       extent_range_clear_dirty_for_io(inode, start, end);
> +                       redirty = 1;
> +               }
>
>                 /* Compression level is applied here and only here */
>                 ret = btrfs_compress_pages(
> --
> 2.14.2



-- 
Have a nice day,
Timofey.

Re: [PATCH v4] Btrfs: compress_file_range() change page dirty status once

[David Sterba]

On Tue, Oct 24, 2017 at 01:29:48AM +0300, Timofey Titovets wrote:
> We need to call extent_range_clear_dirty_for_io()
> on compression range to prevent application from changing
> page content, while pages compressing.
> 
> extent_range_clear_dirty_for_io() run on each loop iteration,
> "(end - start)" can be much (up to 1024 times) bigger
> then compression range (BTRFS_MAX_UNCOMPRESSED).
> 
> That produce extra calls to page managment code.
> 
> Fix that behaviour by call extent_range_clear_dirty_for_io()
> only once.
> 
> v1 -> v2:
>  - Make that more obviously and more safeprone
> 
> v2 -> v3:
>  - Rebased on:
>    Btrfs: compress_file_range() remove dead variable num_bytes
>  - Update change log
>  - Add comments
> 
> v3 -> v4:
>  - Rebased on: kdave for-next
>  - To avoid dirty bit clear/set behaviour change
>    call clear_bit once, istead of per compression range

This version looks safe to me. There's no functional difference if we
redirty the pages repeatedly, but there's probably a noticeable runtime
difference if we do it just once.

Reviewed-by: David Sterba <dsterba@suse.com>