[PATCH] btrfs: do not clear page dirty at extent_write_cache_pages()

[PATCH] btrfs: do not clear page dirty at extent_write_cache_pages()

[Qu Wenruo]

[PROBLEM]
Currently we call folio_clear_dirty_for_io() for the locked dirty folio
inside extent_write_cache_pages().

However this call is not really subpage aware, it's from the older days
where one page can only have one sector.

But with nowadays subpage support, we can have multiple sectors inside
one page, thus if we clear the whole page dirty flag, it would make the
subpage and page dirty flags desynchronize.

Thankfully this is not a big deal as our current subpage routine always
call __extent_writepage_io() for all the subpage dirty ranges, thus it
would ensure there is no subpage range dirty left.

[FIX]
So here we just drop the folio_clear_dirty_for_io() call, and let
__extent_writepage_io() and extent_clear_unlock_delalloc() (which is for
compression path) to handle the dirty page and subapge clearing.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
This patch is independent from the subpage zoned fixes, thus it can be
applied either before or after the subpage zoned fixes.
---
 fs/btrfs/extent_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7275bd919a3e..a8fc0fcfa69f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2231,8 +2231,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
 				folio_wait_writeback(folio);
 			}
 
-			if (folio_test_writeback(folio) ||
-			    !folio_clear_dirty_for_io(folio)) {
+			if (folio_test_writeback(folio)) {
 				folio_unlock(folio);
 				continue;
 			}
-- 
2.45.1

Re: [PATCH] btrfs: do not clear page dirty at extent_write_cache_pages()

[Qu Wenruo]

在 2024/5/20 13:00, Qu Wenruo 写道:
> [PROBLEM]
> Currently we call folio_clear_dirty_for_io() for the locked dirty folio
> inside extent_write_cache_pages().
> 
> However this call is not really subpage aware, it's from the older days
> where one page can only have one sector.
> 
> But with nowadays subpage support, we can have multiple sectors inside
> one page, thus if we clear the whole page dirty flag, it would make the
> subpage and page dirty flags desynchronize.
> 
> Thankfully this is not a big deal as our current subpage routine always
> call __extent_writepage_io() for all the subpage dirty ranges, thus it
> would ensure there is no subpage range dirty left.
> 
> [FIX]
> So here we just drop the folio_clear_dirty_for_io() call, and let
> __extent_writepage_io() and extent_clear_unlock_delalloc() (which is for
> compression path) to handle the dirty page and subapge clearing.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Please drop the patch.

Weirdly with this one, generic/027 would hang on locking the page...

Thanks,
Qu
> ---
> This patch is independent from the subpage zoned fixes, thus it can be
> applied either before or after the subpage zoned fixes.
> ---
>   fs/btrfs/extent_io.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 7275bd919a3e..a8fc0fcfa69f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2231,8 +2231,7 @@ static int extent_write_cache_pages(struct address_space *mapping,
>   				folio_wait_writeback(folio);
>   			}
>   
> -			if (folio_test_writeback(folio) ||
> -			    !folio_clear_dirty_for_io(folio)) {
> +			if (folio_test_writeback(folio)) {
>   				folio_unlock(folio);
>   				continue;
>   			}

Re: [PATCH] btrfs: do not clear page dirty at extent_write_cache_pages()

[Qu Wenruo]

在 2024/5/20 15:34, Qu Wenruo 写道:
> 
> 
> 在 2024/5/20 13:00, Qu Wenruo 写道:
>> [PROBLEM]
>> Currently we call folio_clear_dirty_for_io() for the locked dirty folio
>> inside extent_write_cache_pages().
>>
>> However this call is not really subpage aware, it's from the older days
>> where one page can only have one sector.
>>
>> But with nowadays subpage support, we can have multiple sectors inside
>> one page, thus if we clear the whole page dirty flag, it would make the
>> subpage and page dirty flags desynchronize.
>>
>> Thankfully this is not a big deal as our current subpage routine always
>> call __extent_writepage_io() for all the subpage dirty ranges, thus it
>> would ensure there is no subpage range dirty left.
>>
>> [FIX]
>> So here we just drop the folio_clear_dirty_for_io() call, and let
>> __extent_writepage_io() and extent_clear_unlock_delalloc() (which is for
>> compression path) to handle the dirty page and subapge clearing.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Please drop the patch.
> 
> Weirdly with this one, generic/027 would hang on locking the page...

More weirdly, this only happens for aarch64 subpage cases...

> 
> Thanks,
> Qu
>> ---
>> This patch is independent from the subpage zoned fixes, thus it can be
>> applied either before or after the subpage zoned fixes.
>> ---
>>   fs/btrfs/extent_io.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 7275bd919a3e..a8fc0fcfa69f 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2231,8 +2231,7 @@ static int extent_write_cache_pages(struct 
>> address_space *mapping,
>>                   folio_wait_writeback(folio);
>>               }
>> -            if (folio_test_writeback(folio) ||
>> -                !folio_clear_dirty_for_io(folio)) {
>> +            if (folio_test_writeback(folio)) {
>>                   folio_unlock(folio);
>>                   continue;
>>               }
>