[PATCH 0/2] fixes for uncached IO

[PATCH 0/2] fixes for uncached IO

[Jingbo Xu]

Jingbo Xu (2):
  mm/filemap: fix miscalculated file range for
    filemap_fdatawrite_range_kick()
  mm/truncate: don't skip dirty page in folio_unmap_invalidate()

 include/linux/fs.h | 4 ++--
 mm/filemap.c       | 2 +-
 mm/truncate.c      | 2 --
 3 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/2] mm/filemap: fix miscalculated file range for filemap_fdatawrite_range_kick()

[Jingbo Xu]

iocb->ki_pos has been updated with the number of written bytes since
generic_perform_write().

Besides __filemap_fdatawrite_range() accepts the inclusive end of the
data range.

Fixes: 1d4457576570 ("mm: call filemap_fdatawrite_range_kick() after IOCB_DONTCACHE issue")
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 include/linux/fs.h | 4 ++--
 mm/filemap.c       | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2c3b2f8a621f..f12774291423 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2975,8 +2975,8 @@ static inline ssize_t generic_write_sync(struct kiocb *iocb, ssize_t count)
 	} else if (iocb->ki_flags & IOCB_DONTCACHE) {
 		struct address_space *mapping = iocb->ki_filp->f_mapping;
 
-		filemap_fdatawrite_range_kick(mapping, iocb->ki_pos,
-					      iocb->ki_pos + count);
+		filemap_fdatawrite_range_kick(mapping, iocb->ki_pos - count,
+					      iocb->ki_pos - 1);
 	}
 
 	return count;
diff --git a/mm/filemap.c b/mm/filemap.c
index 804d7365680c..d4564a79eb35 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -445,7 +445,7 @@ EXPORT_SYMBOL(filemap_fdatawrite_range);
  * filemap_fdatawrite_range_kick - start writeback on a range
  * @mapping:	target address_space
  * @start:	index to start writeback on
- * @end:	last (non-inclusive) index for writeback
+ * @end:	last (inclusive) index for writeback
  *
  * This is a non-integrity writeback helper, to start writing back folios
  * for the indicated range.
-- 
2.19.1.6.gb485710b

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

[Jingbo Xu]

... 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()

[Kirill A. Shutemov]

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()

[Dave Chinner]

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()

[Jingbo Xu]

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()

[Jingbo Xu]

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()

[Jens Axboe]

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

Re: [PATCH 1/2] mm/filemap: fix miscalculated file range for filemap_fdatawrite_range_kick()

[Jens Axboe]

On 2/18/25 5:02 AM, Jingbo Xu wrote:
> iocb->ki_pos has been updated with the number of written bytes since
> generic_perform_write().
> 
> Besides __filemap_fdatawrite_range() accepts the inclusive end of the
> data range.

Fix looks good, this got introduced because the logic got moved into
generic_write_sync() rather than being a separate helper.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH 0/2] fixes for uncached IO

[Jingbo Xu]

Hi Christian,

On 2/18/25 8:02 PM, Jingbo Xu wrote:
> Jingbo Xu (2):
>   mm/filemap: fix miscalculated file range for
>     filemap_fdatawrite_range_kick()
>   mm/truncate: don't skip dirty page in folio_unmap_invalidate()
> 
>  include/linux/fs.h | 4 ++--
>  mm/filemap.c       | 2 +-
>  mm/truncate.c      | 2 --
>  3 files changed, 3 insertions(+), 5 deletions(-)
> 

Would you consider these fixes?

-- 
Thanks,
Jingbo

Re: [PATCH 0/2] fixes for uncached IO

[Christian Brauner]

On Tue, 18 Feb 2025 20:02:07 +0800, Jingbo Xu wrote:
> Jingbo Xu (2):
>   mm/filemap: fix miscalculated file range for
>     filemap_fdatawrite_range_kick()
>   mm/truncate: don't skip dirty page in folio_unmap_invalidate()
> 
> include/linux/fs.h | 4 ++--
>  mm/filemap.c       | 2 +-
>  mm/truncate.c      | 2 --
>  3 files changed, 3 insertions(+), 5 deletions(-)
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/2] mm/filemap: fix miscalculated file range for filemap_fdatawrite_range_kick()
      https://git.kernel.org/vfs/vfs/c/8510edf191d2
[2/2] mm/truncate: don't skip dirty page in folio_unmap_invalidate()
      https://git.kernel.org/vfs/vfs/c/927289988068