From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: mm/shmem: add shmem_insert_folio() Date: Sat, 16 May 2026 13:43:40 +1000 Message-ID: In-Reply-To: <20260512110339.6244-2-thomas.hellstrom@linux.intel.com> References: <20260512110339.6244-1-thomas.hellstrom@linux.intel.com> <20260512110339.6244-2-thomas.hellstrom@linux.intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **mm/page_alloc.c =E2=80=94 `undo_compound_page()`** The new function reverses `prep_compound_page()`: ```c void undo_compound_page(struct page *page) { unsigned int i, nr =3D 1U << compound_order(page); page[1].flags.f &=3D ~PAGE_FLAGS_SECOND; for (i =3D 1; i < nr; i++) { page[i].mapping =3D NULL; clear_compound_head(&page[i]); } ClearPageHead(page); } ``` This correctly reverses what `prep_compound_tail()` does (sets `mapping =3D= TAIL_MAPPING`, `set_compound_head()`, `set_page_private(0)`), and clears `= PAGE_FLAGS_SECOND` which covers the order bits stored by `folio_set_order()= ` (stored in `folio->_flags_1` which aliases `page[1].flags.f`). However, `prep_compound_head()` also initializes several fields in `page[1]= ` that alias folio metadata: `_large_mapcount`, `_entire_mapcount`, `_pinco= unt`, `_nr_pages_mapped`, `_mm_ids`, and `_deferred_list`. These are NOT re= stored by `undo_compound_page()`. This is safe *only* because `ClearPageHea= d()` prevents any folio API from reading them, and the pages are required t= o be isolated. But it means `page[1]`'s underlying storage has been modifie= d =E2=80=94 fields like `lru.prev` that overlapped with folio metadata are = now stale. If a caller ever inspects those fields after undo, it will see g= arbage. The docstring should explicitly state that tail page `lru` fields a= re clobbered and must not be relied upon after undo. **Suggestion:** Consider whether this function should be `static` to `mm/sh= mem.c` (or at most `mm/internal.h`) rather than exported in `include/linux/= mm.h`. It's a very specialized operation and exposing it globally invites m= isuse. **mm/shmem.c =E2=80=94 `shmem_insert_folio()`** The function is well-documented and has appropriate `VM_BUG_ON` guards. A f= ew issues: 1. **Writeback path missing `folio_clear_dirty_for_io()` and `folio_set_rec= laim()`:** The old `ttm_backup_backup_page()` carefully prepared the folio for writ= eback: ```c folio_clear_dirty_for_io(to_folio); folio_set_reclaim(to_folio); ret =3D shmem_writeout(to_folio, NULL, NULL); ``` =20 The new code skips both: ```c folio_mark_dirty(folio); /* ... accounting ... */ if (writeback) { ret =3D shmem_writeout(folio, NULL, NULL); ``` =20 Looking at `shmem_writeout()`, it does NOT call `folio_clear_dirty_for_i= o()` internally =E2=80=94 it's a writeback callback that expects the writeb= ack infrastructure to have done this already. When called directly, the old= code did it explicitly. The new code doesn't. While `shmem_writeout()` may= still function because it does its own dirty state management via `shmem_d= elete_from_page_cache()` and `folio_mark_dirty()` on the redirty path, this= deviates from the established protocol and could interact poorly with dirt= y page accounting in the address space. Similarly, `folio_set_reclaim()` is a performance hint that tells reclai= m to free the page promptly after writeback completes. Omitting it means su= ccessfully written-out pages may linger longer before being freed. 2. **EEXIST handling silently truncates:** ```c ret =3D shmem_add_to_page_cache(folio, mapping, index, NULL, folio_gfp); if (ret =3D=3D -EEXIST) { shmem_truncate_range(inode, (loff_t)index << PAGE_SHIFT, ((loff_t)(index + nr_pages) << PAGE_SHIFT) - 1); ret =3D shmem_add_to_page_cache(folio, mapping, index, NULL, folio_gfp); } ``` =20 This silently discards whatever was previously at `@index`. For TTM's us= e case this might be acceptable (a previous partial backup was abandoned), = but it could mask logic bugs where a caller accidentally inserts at an alre= ady-occupied index. A `WARN_ON_ONCE` or at least a `pr_debug` before the tr= uncate would help catch misuse. 3. **shmem_inode_acct_blocks failure path:** ```c ret =3D shmem_inode_acct_blocks(inode, nr_pages); if (ret) { filemap_remove_folio(folio); goto err_uncharge; } ``` After `shmem_add_to_page_cache()` succeeds, the folio is in the page cac= he with `folio->mapping` set and its refcount bumped by `nr`. `filemap_remo= ve_folio()` reverses this. Then `err_uncharge` does `mem_cgroup_uncharge()`= + `folio_unlock()` + optionally `undo_compound_page()`. The flow appears c= orrect =E2=80=94 `filemap_remove_folio` should clear `folio->mapping` and d= rop the page-cache refs, leaving the folio in its pre-insertion state. But = the window between `shmem_add_to_page_cache` and `shmem_inode_acct_blocks` = has the folio visible in the page cache without proper block accounting; th= e folio lock prevents racing reclaim. 4. **Return value when writeback fails:** ```c if (writeback) { ret =3D shmem_writeout(folio, NULL, NULL); if (ret =3D=3D AOP_WRITEPAGE_ACTIVATE) { folio_add_lru(folio); folio_unlock(folio); } /* ret =3D=3D 0 or ret < 0: folio unlocked by shmem_writeout */ } =20 return 0; ``` The function returns 0 (success) regardless of the writeback result, whi= ch matches the "best-effort" contract in the docstring. The comment about `= ret < 0` is defensive =E2=80=94 analysis of `shmem_writeout()` shows it onl= y returns `0` (folio unlocked, swapped out) or `AOP_WRITEPAGE_ACTIVATE` (fo= lio locked, swap failed). It never returns negative errors. The handling is= correct for the current `shmem_writeout()` semantics, but the comment is m= isleading about the actual return value space. **include/linux/mm.h:** ```c extern void undo_compound_page(struct page *page); ``` As noted above, exporting this in the global mm header is arguably too broa= d for such a specialized function. --- Generated by Claude Code Patch Reviewer