| Message ID | 20250218120209.88093-3-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fixes for uncached IO | expand |
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.
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.
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?
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 ```
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.
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));
... 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(-)