fixes for uncached IO

[PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Jingbo Xu 10 months ago

... otherwise this is a behavior change for the previous callers of
invalidate_complete_folio2(), e.g. the page invalidation routine.

Fixes: 4a9e23159fd3 ("mm/truncate: add folio_unmap_invalidate() helper")
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 mm/truncate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/truncate.c b/mm/truncate.c
index e2e115adfbc5..76d8fcd89bd0 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -548,8 +548,6 @@ int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
 
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
 
-	if (folio_test_dirty(folio))
-		return 0;
 	if (folio_mapped(folio))
 		unmap_mapping_folio(folio);
 	BUG_ON(folio_mapped(folio));
-- 
2.19.1.6.gb485710b

Re: [PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Dave Chinner 10 months ago

On Tue, Feb 18, 2025 at 08:02:09PM +0800, Jingbo Xu wrote:
> ... otherwise this is a behavior change for the previous callers of
> invalidate_complete_folio2(), e.g. the page invalidation routine.
> 
> Fixes: 4a9e23159fd3 ("mm/truncate: add folio_unmap_invalidate() helper")
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>  mm/truncate.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index e2e115adfbc5..76d8fcd89bd0 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -548,8 +548,6 @@ int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
>  
>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>  
> -	if (folio_test_dirty(folio))
> -		return 0;

Shouldn't that actually return -EBUSY because the folio could not be
invalidated?

Indeed, further down the function the folio gets locked and the
dirty test is repeated. If it fails there it returns -EBUSY....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Jingbo Xu 10 months ago


On 2/19/25 8:11 AM, Dave Chinner wrote:
> On Tue, Feb 18, 2025 at 08:02:09PM +0800, Jingbo Xu wrote:
>> ... otherwise this is a behavior change for the previous callers of
>> invalidate_complete_folio2(), e.g. the page invalidation routine.
>>
>> Fixes: 4a9e23159fd3 ("mm/truncate: add folio_unmap_invalidate() helper")
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>>  mm/truncate.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index e2e115adfbc5..76d8fcd89bd0 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@ -548,8 +548,6 @@ int folio_unmap_invalidate(struct address_space *mapping, struct folio *folio,
>>  
>>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>>  
>> -	if (folio_test_dirty(folio))
>> -		return 0;
> 
> Shouldn't that actually return -EBUSY because the folio could not be
> invalidated?
> 
> Indeed, further down the function the folio gets locked and the
> dirty test is repeated. If it fails there it returns -EBUSY....
> 
> -Dave.

Yeah, the original logic of invalidate_inode_pages2_range() is like

```
invalidate_inode_pages2_range
  # lock page
  # launder the page if it's dirty

  invalidate_complete_folio2
    # recheck if it's dirty, and skip the dirty page (no idea how page
could be redirtied after launder_page())
```

while after commit 4a9e23159fd3 ("mm/truncate: add
folio_unmap_invalidate() helper"), this logic is changed to:

```
invalidate_inode_pages2_range
  # lock page
  folio_unmap_invalidate
    # check if it's dirty, and skip dirty page
    # launder the page if it's dirty (doubt if it's noops)

    # recheck if it's dirty, and skip the dirty page
```


-- 
Thanks,
Jingbo

Re: [PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Kirill A. Shutemov 10 months ago

On Tue, Feb 18, 2025 at 08:02:09PM +0800, Jingbo Xu wrote:
> ... otherwise this is a behavior change for the previous callers of
> invalidate_complete_folio2(), e.g. the page invalidation routine.

Hm. Shouldn't the check be moved to caller of the helper in mm/filemap.c?

Otherwise we would drop pages without writing them back. And lose user's
data.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Jingbo Xu 10 months ago


On 2/18/25 8:32 PM, Kirill A. Shutemov wrote:
> On Tue, Feb 18, 2025 at 08:02:09PM +0800, Jingbo Xu wrote:
>> ... otherwise this is a behavior change for the previous callers of
>> invalidate_complete_folio2(), e.g. the page invalidation routine.
> 
> Hm. Shouldn't the check be moved to caller of the helper in mm/filemap.c?
> 
> Otherwise we would drop pages without writing them back. And lose user's
> data.
> 

IMHO this check is not needed as the following folio_launder() called
inside folio_unmap_invalidate() will write back the dirty page.

Hi Jens,

What do you think about it?

-- 
Thanks,
Jingbo

Re: [PATCH 2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()

Posted by Jens Axboe 10 months ago

On 2/18/25 6:23 PM, Jingbo Xu wrote:
> 
> 
> On 2/18/25 8:32 PM, Kirill A. Shutemov wrote:
>> On Tue, Feb 18, 2025 at 08:02:09PM +0800, Jingbo Xu wrote:
>>> ... otherwise this is a behavior change for the previous callers of
>>> invalidate_complete_folio2(), e.g. the page invalidation routine.
>>
>> Hm. Shouldn't the check be moved to caller of the helper in mm/filemap.c?
>>
>> Otherwise we would drop pages without writing them back. And lose user's
>> data.
>>
> 
> IMHO this check is not needed as the following folio_launder() called
> inside folio_unmap_invalidate() will write back the dirty page.
> 
> Hi Jens,
> 
> What do you think about it?

Yep agree on that.

-- 
Jens Axboe