khugepaged: fix readahead lock inversion in collapse_file()
Subsystems (2)
Lance Yang
reviewerPATCH v3 1/1 khugepaged: fix readahead lock inversion in collapse_file()
collapse_file() can end up calling page_cache_sync_readahead() with source folios from earlier in the range still locked. If truncate already holds mapping->invalidate_lock exclusively and is waiting for one of those folios, readahead blocks on the same invalidate_lock and neither side can make progress.
Do readahead in a separate pass before we start holding source folios locked for collapse. Keep page_cache_sync_readahead() and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.
Shmem handles swapin and fallocated folios through shmem_get_folio() without taking mapping->invalidate_lock, so it isn't affected by this lock inversion.
---
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wide invalidate_lock and page_cache_ra_unbounded() with a preparation pass using page_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/
mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)
mm/khugepaged.c +31 -11
@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) { + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ; + index =folio_next_index (folio); +folio_unlock (folio); +folio_put (folio); +if (result !=SCAN_SUCCEED ) + goto rollback; + } +/* drain lru cache to help folio_isolate_lru() */ +lru_add_drain (); + } +/* * Ensure we have slots for all the pages in the range. This is * almost certainly a no-op because most of the pages must be present
@@ -2320,17 +2348,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, } }else {/* !is_shmem */ if (!folio ||xa_is_value (folio)) { -xas_unlock_irq (&xas); -page_cache_sync_readahead (mapping, &file->f_ra, - file, index, - end - index); -/* drain lru cache to help folio_isolate_lru() */ -lru_add_drain (); - folio =filemap_lock_folio (mapping, index); -if (IS_ERR (folio)) { - result =SCAN_FAIL ; - goto xa_unlocked; - } +/* The range changed after the preparation pass. */ + result =SCAN_PAGE_LOCK ; + goto xa_locked; }else if (folio_test_dirty (folio)) {/* * This page is dirty because it hasn't
Signature · 1 line
2.49.0
Baolin Wang
reviewerQuoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.
Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 23 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2270
Quoted 9 lines
mm/khugepaged.c +6
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {
why drop xas_unlock_irq()?
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 12 lines
mm/khugepaged.c +12
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;
Do we need to check PG_uptodate after locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following code also checks the writeback flag, so the uptodate check seems unnecessary to me.
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2291
Quoted 9 lines
mm/khugepaged.c +9
-0+ index =folio_next_index (folio); +folio_unlock (folio); +folio_put (folio); +if (result !=SCAN_SUCCEED ) + goto rollback; + } +/* drain lru cache to help folio_isolate_lru() */ +lru_add_drain (); + }
Could we factor this code out into a small helper?
Quoted 25 lines
mm/khugepaged.c +4
-11+/* * Ensure we have slots for all the pages in the range. This is * almost certainly a no-op because most of the pages must be present @@ -2320,17 +2348,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, } }else {/* !is_shmem */ if (!folio ||xa_is_value (folio)) { -xas_unlock_irq (&xas); -page_cache_sync_readahead (mapping, &file->f_ra, - file, index, - end - index); -/* drain lru cache to help folio_isolate_lru() */ -lru_add_drain (); - folio =filemap_lock_folio (mapping, index); -if (IS_ERR (folio)) { - result =SCAN_FAIL ; - goto xa_unlocked; - } +/* The range changed after the preparation pass. */ + result =SCAN_PAGE_LOCK ; + goto xa_locked; }else if (folio_test_dirty (folio)) {/* * This page is dirty because it hasn't
Quoted 62 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 23 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2269
Quoted 8 lines
mm/khugepaged.c +6
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?
Quoted 15 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 12 lines
mm/khugepaged.c +12
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following
Yes, you do; a read can "complete" without the IO having completed successfully, and in those cases PG_uptodate is not set, but the folio is unlocked.
Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
Signature · 1 line
Pedro
Quoted 79 lines · Pedro Falcato
Quoted 62 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 23 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2269
Quoted 8 lines
mm/khugepaged.c +6
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?Quoted 15 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 12 lines
mm/khugepaged.c +12
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following
Quoted 6 lines
Yes, you do; a read can "complete" without the IO having completed successfully, and in those cases
PG_uptodateis not set, but the folio is unlocked.Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
To clarify: I would do this kind of RA as an error path if isolation failed. Namely, at the end of rollback, after unlocking folios. It doesn't make sense to me to do this kind of checking upfront. I don't think it's particularly realistic that, starting RA upfront, you'd see uptodate completed folio reads when doing the actual isolation.
Signature · 1 line
Pedro
Lance Yang
reviewerQuoted 87 lines · Pedro Falcato
Quoted 79 lines · Pedro Falcato
Quoted 62 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 23 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2269
Quoted 8 lines
mm/khugepaged.c +6
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?Quoted 15 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2281
Quoted 12 lines
mm/khugepaged.c +12
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The followingQuoted 6 lines
Yes, you do; a read can "complete" without the IO having completed successfully, and in those cases
PG_uptodateis not set, but the folio is unlocked.Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
Quoted 5 lines
To clarify: I would do this kind of RA as an error path if isolation failed. Namely, at the end of rollback, after unlocking folios. It doesn't make sense to me to do this kind of checking upfront. I don't think it's particularly realistic that, starting RA upfront, you'd see uptodate completed folio reads when doing the actual isolation.
Good call! I should have thought of that :P We can do this on demand after rollback rather than walk the range every time :)
I played around with it a bit, something like:
---8<---
mm/khugepaged.c +33 -11
@@ -2290,6 +2290,8 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,enum scan_result result =SCAN_SUCCEED ;int nr_none =0 ;bool is_shmem =shmem_file (file); +bool do_readahead = false; +bool readahead_done = false;/* * MADV_COLLAPSE ignores shmem huge config, so do not check shmem
@@ -2307,6 +2309,10 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,mapping_set_update (&xas, mapping); +retry: + result =SCAN_SUCCEED ; + do_readahead = false; +xas_set_order (&xas, start,HPAGE_PMD_ORDER );__folio_set_locked (new_folio);if (is_shmem)__folio_set_swapbacked (new_folio);
@@ -2371,17 +2377,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, } }else {/* !is_shmem */ if (!folio ||xa_is_value (folio)) { -xas_unlock_irq (&xas); -page_cache_sync_readahead (mapping, &file->f_ra, - file, index, - end - index); -/* drain lru cache to help folio_isolate_lru() */ -lru_add_drain (); - folio =filemap_lock_folio (mapping, index); -if (IS_ERR (folio)) { - result =SCAN_FAIL ; - goto xa_unlocked; - } + do_readahead = !readahead_done; + result =SCAN_FAIL ; + goto xa_locked; }else if (folio_test_dirty (folio)) {/* * This page is dirty because it hasn't
@@ -2704,6 +2702,30 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->mapping =NULL ;folio_unlock (new_folio); + +/* Readahead may take invalidate_lock; source folios are unlocked. */ +if (do_readahead) { +for (index = start; index < end;) { + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto out_put; + } + } + index =folio_next_index (folio); +folio_unlock (folio); +folio_put (folio); + } +/* Drain the local LRU batch before retrying isolation. */ +lru_add_drain (); + readahead_done = true; + goto retry; + } +out_put:folio_put (new_folio); out:VM_BUG_ON (!list_empty (&pagelist));
--
Cheers, Lance
Baolin Wang
reviewerQuoted 80 lines · Pedro Falcato
Quoted 79 lines · Pedro Falcato
Quoted 77 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 23 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2269
Quoted 8 lines
mm/khugepaged.c +6
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; +if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 12 lines
mm/khugepaged.c +12
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + } + } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following
Quoted 2 lines
Quoted 2 lines
Yes, you do; a read can "complete" without the IO having completed successfully, and in those cases
PG_uptodateis not set, but the folio is unlocked.
OK.
Quoted 5 lines
Quoted 3 lines
Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
To clarify: I would do this kind of RA as an error path if isolation failed.
Sounds good.
Quoted 4 lines
Namely, at the end of rollback, after unlocking folios. It doesn't make sense to me to do this kind of checking upfront. I don't think it's particularly realistic that, starting RA upfront, you'd see uptodate completed folio reads when doing the actual isolation.
But why not add RA logic in collapse_pmd(), as we do for SCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and this would also simplify the long collapse_file().
Lance Yang
reviewerQuoted 96 lines · Baolin Wang
Quoted 79 lines · Pedro Falcato
Quoted 78 lines · Pedro Falcato
Quoted 77 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 29 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/20260913163644.122133-1- [email protected]/
mm/khugepaged.c| 42 +++++++++++++++++++++++++++++++-----------1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +3
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; + if (!is_shmem) { + /* +* Populate the range before accumulating locked source Quoted 4 lines
folios:
+ * readahead may block oninvalidate_lock. + */
+ for (index = start; index < end;) {why drop
xas_unlock_irq()?Quoted 12 lines
+ folio =
filemap_lock_folio(mapping, index); + if (IS_ERR(folio)) {
+ page_cache_sync_readahead(mapping,&file->f_ra, + file, index, end - index); + folio =filemap_lock_folio(mapping, index); + if (IS_ERR(folio)) {
+ result =SCAN_FAIL;
+ goto rollback;
+ }
+ }
+ if (!folio_test_uptodate(folio))
+ result =SCAN_FAIL;Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The followingYes, you do; a read can "complete" without the IO having completed successfully, and in those cases
PG_uptodateis not set, but the folio is unlocked.OK.
Quoted 5 lines
Quoted 3 lines
Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
To clarify: I would do this kind of RA as an error path if isolation failed.
Quoted 8 lines
Sounds good.
Quoted 4 lines
Namely, at the end of rollback, after unlocking folios. It doesn't make sense to me to do this kind of checking upfront. I don't think it's particularly realistic that, starting RA upfront, you'd see uptodate completed folio reads when doing the actual isolation.
But why not add RA logic in
collapse_pmd(), as we do forSCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and this would also simplify the longcollapse_file().
That would be cleaner, yeah. Some older stable trees don't have that outer retry loop, though ... I'd prefer to get the fix backported first, then move the RA/retry logic out in a follow-up, wdyt?
Cheers, Lance
Baolin Wang
reviewerQuoted 106 lines · Lance Yang
Quoted 103 lines · Baolin Wang
Quoted 79 lines · Pedro Falcato
Quoted 78 lines · Pedro Falcato
Quoted 77 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Quoted 29 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/20260913163644.122133-1- [email protected]/
mm/khugepaged.c| 42 +++++++++++++++++++++++++++++++-----------1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +3
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping; + if (!is_shmem) { + /* +* Populate the range before accumulating locked source Quoted 4 lines
folios:
+ * readahead may block oninvalidate_lock. + */
+ for (index = start; index < end;) {why drop
xas_unlock_irq()?Quoted 12 lines
+ folio =
filemap_lock_folio(mapping, index); + if (IS_ERR(folio)) {
+ page_cache_sync_readahead(mapping,&file->f_ra, + file, index, end - index); + folio =filemap_lock_folio(mapping, index); + if (IS_ERR(folio)) {
+ result =SCAN_FAIL;
+ goto rollback;
+ }
+ }
+ if (!folio_test_uptodate(folio))
+ result =SCAN_FAIL;Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The followingYes, you do; a read can "complete" without the IO having completed successfully, and in those cases
PG_uptodateis not set, but the folio is unlocked.OK.
Quoted 5 lines
Quoted 3 lines
Either way, why are we checking for uptodate here? We only care for starting readahead. Anyway, I would possibly recommend we do this as post-processing, not pre-processing. The current loop is a little overhead-happy.
To clarify: I would do this kind of RA as an error path if isolation failed.
Sounds good.
Quoted 4 lines
Namely, at the end of rollback, after unlocking folios. It doesn't make sense to me to do this kind of checking upfront. I don't think it's particularly realistic that, starting RA upfront, you'd see uptodate completed folio reads when doing the actual isolation.
Quoted 5 lines
Quoted 1 line
But why not add RA logic in
collapse_pmd(), as we do forSCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and this would also simplify the longcollapse_file().That would be cleaner, yeah. Some older stable trees don't have that outer retry loop, though ... I'd prefer to get the fix backported first, then move the RA/retry logic out in a follow-up, wdyt?
IIRC, the approach to bug fixes is to fix bugs properly and cleanly for upstream, without being constrained by backporting requirements.
Anyway, I don't have a strong opinion on this. Let's also see what David prefers.
David Hildenbrand (Arm)
maintainerQuoted 19 lines · Baolin Wang
Quoted 16 lines · Lance Yang
Quoted 12 lines · Baolin Wang
OK.
Sounds good.
But why not add RA logic in
collapse_pmd(), as we do forSCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and this would also simplify the longcollapse_file().
Quoted 8 lines
Quoted 3 lines
That would be cleaner, yeah. Some older stable trees don't have that outer retry loop, though ... I'd prefer to get the fix backported first, then move the RA/retry logic out in a follow-up, wdyt?
IIRC, the approach to bug fixes is to fix bugs properly and cleanly for upstream, without being constrained by backporting requirements.
Anyway, I don't have a strong opinion on this. Let's also see what David prefers.
Let's get this fixed in a acceptable way in a reasonable timeframe; it's been broken for too long. ... and follow up with bigger cleanups later.
@Lance is this version good to review for that purpose or are you planning a resend?
Signature · 3 lines
Cheers, David
Lance Yang
reviewerQuoted 27 lines · David Hildenbrand (Arm)
Quoted 26 lines · Baolin Wang
Quoted 12 lines · Lance Yang
Quoted 9 lines · Baolin Wang
OK.
Sounds good.
Quoted 7 lines
Quoted 3 lines
But why not add RA logic in
collapse_pmd(), as we do forSCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and this would also simplify the longcollapse_file().That would be cleaner, yeah. Some older stable trees don't have that outer retry loop, though ... I'd prefer to get the fix backported first, then move the RA/retry logic out in a follow-up, wdyt?
IIRC, the approach to bug fixes is to fix bugs properly and cleanly for upstream, without being constrained by backporting requirements.
Quoted 6 lines
Quoted 1 line
Anyway, I don't have a strong opinion on this. Let's also see what David prefers.
Let's get this fixed in a acceptable way in a reasonable timeframe; it's been broken for too long. ... and follow up with bigger cleanups later.
@Lance is this version good to review for that purpose or are you planning a resend?
Right, as for the bigger cleanup, that can come later. Will resend the current version as-is :)
Lance Yang
reviewer+Cc: Pedro
Quoted 20 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.
Quoted 7 lines
Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Cheers!
Quoted 26 lines
Quoted 26 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping;
Quoted 8 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2270
Quoted 6 lines
mm/khugepaged.c +6
-0+if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?
Hmm ... we haven't taken the lock here yet, so there's just nothing to unlock, no?
Quoted 9 lines
Quoted 9 lines
mm/khugepaged.c +9
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + }
Quoted 8 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 3 lines
mm/khugepaged.c +3
-0+ } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following code also checks the writeback flag, so the uptodate check seems unnecessary to me.
IIRC, the folio gets unlocked even if the read fails. I just wanted to catch read errors early. And yeah, we already check uptodate later, so this one can go, thanks!
Quoted 11 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2291
Quoted 9 lines
mm/khugepaged.c +9
-0+ index =folio_next_index (folio); +folio_unlock (folio); +folio_put (folio); +if (result !=SCAN_SUCCEED ) + goto rollback; + } +/* drain lru cache to help folio_isolate_lru() */ +lru_add_drain (); + }Could we factor this code out into a small helper?
Okay, let me play with it a bit :) Might pull it out if it reads cleaner.
Thanks, Lance
Quoted 27 lines
Quoted 25 lines
mm/khugepaged.c +4
-11+/* * Ensure we have slots for all the pages in the range. This is * almost certainly a no-op because most of the pages must be present @@ -2320,17 +2348,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, } }else {/* !is_shmem */ if (!folio ||xa_is_value (folio)) { -xas_unlock_irq (&xas); -page_cache_sync_readahead (mapping, &file->f_ra, - file, index, - end - index); -/* drain lru cache to help folio_isolate_lru() */ -lru_add_drain (); - folio =filemap_lock_folio (mapping, index); -if (IS_ERR (folio)) { - result =SCAN_FAIL ; - goto xa_unlocked; - } +/* The range changed after the preparation pass. */ + result =SCAN_PAGE_LOCK ; + goto xa_locked; }else if (folio_test_dirty (folio)) {/* * This page is dirty because it hasn't
Baolin Wang
reviewerQuoted 68 lines · Lance Yang
+Cc: Pedro
Quoted 20 lines · Baolin Wang
Quoted 17 lines · Lance Yang
collapse_file()can end up callingpage_cache_sync_readahead()with source folios from earlier in the range still locked. If truncate already holdsmapping->invalidate_lockexclusively and is waiting for one of those folios, readahead blocks on the sameinvalidate_lockand neither side can make progress.Do readahead in a separate pass before we start holding source folios locked for collapse. Keep
page_cache_sync_readahead()and its existing policy, and drop each folio's lock and reference after checking that it is uptodate. If truncate or reclaim removes a folio between the two passes, roll back instead of doing readahead again with source folios locked.Shmem handles swapin and fallocated folios through
shmem_get_folio()without takingmapping->invalidate_lock, so it isn't affected by this lock inversion.Quoted 7 lines
Quoted 5 lines
Reported-by [email protected]Fixes 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")Signed-off-by Lance Yang <[email protected]>---
Thanks for the fix.
Cheers!
Quoted 26 lines
Quoted 26 lines
Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm happy to take this over.
Some older stable trees lack the folio infrastructure used here, and the code will differ for each stable tree, so each backport will need quite a bit of rework and its own review. I'm not sure whether to Cc stable here. Once we agree on the fix, I'll send a separate backport for each maintained stable tree except 5.10, which isn't affected.
v2 -> v3:
- Replace the scan-wideinvalidate_lockandpage_cache_ra_unbounded()with a preparation pass usingpage_cache_sync_readahead().
- Roll back on a new page cache miss during collapse.
- https://lore.kernel.org/all/[email protected]/mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-)mm/khugepaged.c +0
-0@@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, new_folio->index = start; new_folio->mapping = mapping;Quoted 8 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2270
Quoted 6 lines
mm/khugepaged.c +6
-0+if (!is_shmem) { +/* +* Populate the range before accumulating locked source folios: +* readahead may block on invalidate_lock. +*/ +for (index = start; index < end;) {why drop
xas_unlock_irq()?
Quoted 2 lines
Hmm ... we haven't taken the lock here yet, so there's just nothing to unlock, no?
Ah, yes, I misread the diff. Sorry for noise:) .
Quoted 34 lines
Quoted 9 lines
Quoted 9 lines
mm/khugepaged.c +9
-0+ folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { +page_cache_sync_readahead (mapping, &file->f_ra, + file, index, end - index); + folio =filemap_lock_folio (mapping, index); +if (IS_ERR (folio)) { + result =SCAN_FAIL ; + goto rollback; + }Quoted 8 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2282
Quoted 3 lines
mm/khugepaged.c +3
-0+ } +if (!folio_test_uptodate (folio)) + result =SCAN_FAIL ;Do we need to check
PG_uptodateafter locking the folio? If we successfully lock the folio, doesn't that mean the read I/O has completed? The following code also checks the writeback flag, so the uptodate check seems unnecessary to me.IIRC, the folio gets unlocked even if the read fails. I just wanted to catch read errors early. And yeah, we already check uptodate later, so this one can go, thanks!
Quoted 11 lines
mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2291
Quoted 9 lines
mm/khugepaged.c +9
-0+ index =folio_next_index (folio); +folio_unlock (folio); +folio_put (folio); +if (result !=SCAN_SUCCEED ) + goto rollback; + } +/* drain lru cache to help folio_isolate_lru() */ +lru_add_drain (); + }Could we factor this code out into a small helper?
Quoted 1 line
Okay, let me play with it a bit :) Might pull it out if it reads cleaner.
Thanks. Because collapse_file() is already too long.