Messages in this thread |  | | From | Pratyush Yadav <> | | Subject | Re: [PATCH v3 6/7] mm/memfd_luo: remove folio from page cache when accounting fails | | Date | Thu, 02 Apr 2026 11:52:57 +0000 |
| |
On Thu, Mar 26 2026, Chenghao Duan wrote:
> In memfd_luo_retrieve_folios(), when shmem_inode_acct_blocks() fails > after successfully adding the folio to the page cache, the code jumps > to unlock_folio without removing the folio from the page cache. > > This leaves the folio permanently abandoned in the page cache: > - The folio was added via shmem_add_to_page_cache() which set up > mapping, index, and incremented nrpages/shmem stats. > - folio_unlock() and folio_put() do not remove it from the cache. > - folio_add_lru() was never called, so it cannot be reclaimed.
This is just not true. The folio is _not_ "permanently abandoned" in the page cache. When fput() is called by memfd_luo_retrieve(), it will eventually call shmem_undo_range() on the whole mapping and free all the folios in there.
I went and looked at shmem_undo_range() and the accompanying accounting logic, and all that seems to be impervious to this type of superfluous folio in the filemap. Main reason being that shmem_recalc_inode() directly uses mapping->nrpages after truncation so even if you don't account for the folio, as long as you get rid of the whole file (which we do) it doesn't matter.
I think the only place I can see this causing trouble is maybe in LRU accounting, though I really don't understand how any of that works so dunno.
Anyway, I do think this patch is worth having. It keeps the filemap clean and gets rid of the need of this complex reasoning to figure out if this is safe.
So I think the commit message needs reworking. Perhaps something like the below:
mm/memfd_luo: remove folio from page cache when accounting fails
In memfd_luo_retrieve_folios(), when shmem_inode_acct_blocks() fails after successfully adding the folio to the page cache, the code jumps to unlock_folio without removing the folio from the page cache.
While the folio eventually will be freed when the file is released by memfd_luo_retrieve(), it is a good idea to directly remove a folio that was not fully added to the file. This avoids the possibility of accounting mismatches in shmem or filemap core.
Fix by adding a remove_from_cache label that calls filemap_remove_folio() before unlocking, matching the error handling pattern in shmem_alloc_and_add_folio().
This issue was identified by the AI review. https://sashiko.dev/#/patchset/20260323110747.193569-1-duanchenghao@kylinos.cn
With that,
Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
> > Fix by adding a remove_from_cache label that calls filemap_remove_folio() > before unlocking, matching the error handling pattern in > shmem_alloc_and_add_folio(). > > This issue was identified by the AI review. > https://sashiko.dev/#/patchset/20260323110747.193569-1-duanchenghao@kylinos.cn > > Signed-off-by: Chenghao Duan <duanchenghao@kylinos.cn> [...]
-- Regards, Pratyush Yadav
|  |