mm/filemap.c: Update comments in filemap_fault()
Subsystems (2)
PATCH 1/1 mm/filemap.c: Update comments in filemap_fault()
The code structure in filemap_fault() has evolved significantly since these comments were added making them out of date and slightly confusing. Update them to more accurately reflect the code.
---
The comments also suggest there is no performance concern in the page_not_uptodate path. This isn't entirely true, because this is a common path for MADV_RANDOM due to the lack of read-ahead/around.
We always drop mmap_lock and retry when handling faults for !folio_test_uptodate(). Not dropping the lock resulted in a ~3-7% performance increase on a simple test on my local system. However we typically avoid holding mmap_lock during IO to reduce contention so not sure if changing that is worth pursuing or not?
--- mm/filemap.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
mm/filemap.c +6 -6
@@ -3168,8 +3168,9 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)VM_BUG_ON_FOLIO (!folio_contains (folio, index), folio);/* -* We have a locked page in the page cache, now we need to check -* that it's up-to-date. If not, it is going to be due to an error. +* We have a locked page in the page cache. Check that it's up-to-date. +* If not it is because there was no page in the page cache (only likely +* in the case of MADV_RANDOM) or due to error. */ if (unlikely (!folio_test_uptodate (folio))) {/*
@@ -3214,10 +3215,9 @@ vm_fault_t filemap_fault(struct vm_fault *vmf) page_not_uptodate:/* -* Umm, take care of errors if the page isn't up-to-date. -* Try to re-read it _once_. We do this synchronously, -* because there really aren't any performance issues here -* and we need to check for errors. +* Read in the page if it's not up-to-date. We have to do this +* synchronously so we can map the page to handle the fault or deal with +* errors. */ fpin =maybe_unlock_mmap_for_io (vmf, fpin); error =filemap_read_folio (file, mapping, folio);
Signature · 1 line
2.35.1
Matthew Wilcox
maintainermm/filemap.c vm_fault_t filemap_fault(struct vm_fault *vmf) · line 3173
Quoted 10 lines · Alistair Popple
+++
b/mm/filemap.cmm/filemap.c +3
-2@@ -3168,8 +3168,9 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)VM_BUG_ON_FOLIO (!folio_contains (folio, index), folio);/* -* We have a locked page in the page cache, now we need to check -* that it's up-to-date. If not, it is going to be due to an error. +* We have a locked page in the page cache. Check that it's up-to-date. +* If not it is because there was no page in the page cache (only likely +* in the case of MADV_RANDOM) or due to error.
The comment is correct, but the code is buggy!
We should be calling page_cache_ra_order() in do_sync_mmap_readahead(), even if VM_RAND_READ is set (or !ra_pages, or mmap_miss is high). At least for one page. That would allow us to drop the FGP_FOR_MMAP flag as page_cache_ra_order() will instantiate a folio for us.
Do you want to write that patch or should I do it?
Quoted 5 lines · Matthew Wilcox
Quoted 4 lines · Alistair Popple
+++
b/mm/filemap.cmm/filemap.c +0
-0@@ -3168,8 +3168,9 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)VM_BUG_ON_FOLIO (!folio_contains (folio, index), folio);
Quoted 8 lines
mm/filemap.c vm_fault_t filemap_fault(struct vm_fault *vmf) · line 3173
Quoted 6 lines
mm/filemap.c +3
-2/* -* We have a locked page in the page cache, now we need to check -* that it's up-to-date. If not, it is going to be due to an error. +* We have a locked page in the page cache. Check that it's up-to-date. +* If not it is because there was no page in the page cache (only likely +* in the case of MADV_RANDOM) or due to error. The comment is correct, but the code is buggy!
Ha!
Quoted 6 lines
We should be calling
page_cache_ra_order()indo_sync_mmap_readahead(), even ifVM_RAND_READis set (or !ra_pages, ormmap_missis high). At least for one page. That would allow us to drop theFGP_FOR_MMAPflag aspage_cache_ra_order()will instantiate a folio for us.Do you want to write that patch or should I do it?
Thanks for the pointers, I can do it.
- Alistair