Claude review: mm/shmem: add shmem_insert_folio()

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**mm/page_alloc.c — `undo_compound_page()`**

The new function reverses `prep_compound_page()`:

```c
void undo_compound_page(struct page *page)
{
	unsigned int i, nr = 1U << compound_order(page);

	page[1].flags.f &= ~PAGE_FLAGS_SECOND;
	for (i = 1; i < nr; i++) {
		page[i].mapping = NULL;
		clear_compound_head(&page[i]);
	}
	ClearPageHead(page);
}
```

This correctly reverses what `prep_compound_tail()` does (sets `mapping = 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`, `_pincount`, `_nr_pages_mapped`, `_mm_ids`, and `_deferred_list`. These are NOT restored by `undo_compound_page()`. This is safe *only* because `ClearPageHead()` prevents any folio API from reading them, and the pages are required to be isolated. But it means `page[1]`'s underlying storage has been modified — fields like `lru.prev` that overlapped with folio metadata are now stale. If a caller ever inspects those fields after undo, it will see garbage. The docstring should explicitly state that tail page `lru` fields are clobbered and must not be relied upon after undo.

**Suggestion:** Consider whether this function should be `static` to `mm/shmem.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 misuse.

**mm/shmem.c — `shmem_insert_folio()`**

The function is well-documented and has appropriate `VM_BUG_ON` guards. A few issues:

1. **Writeback path missing `folio_clear_dirty_for_io()` and `folio_set_reclaim()`:**

   The old `ttm_backup_backup_page()` carefully prepared the folio for writeback:
   ```c
   folio_clear_dirty_for_io(to_folio);
   folio_set_reclaim(to_folio);
   ret = shmem_writeout(to_folio, NULL, NULL);
   ```
   
   The new code skips both:
   ```c
   folio_mark_dirty(folio);
   /* ... accounting ... */
   if (writeback) {
       ret = shmem_writeout(folio, NULL, NULL);
   ```
   
   Looking at `shmem_writeout()`, it does NOT call `folio_clear_dirty_for_io()` internally — it's a writeback callback that expects the writeback 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_delete_from_page_cache()` and `folio_mark_dirty()` on the redirty path, this deviates from the established protocol and could interact poorly with dirty page accounting in the address space.

   Similarly, `folio_set_reclaim()` is a performance hint that tells reclaim to free the page promptly after writeback completes. Omitting it means successfully written-out pages may linger longer before being freed.

2. **EEXIST handling silently truncates:**

   ```c
   ret = shmem_add_to_page_cache(folio, mapping, index, NULL, folio_gfp);
   if (ret == -EEXIST) {
       shmem_truncate_range(inode,
                    (loff_t)index << PAGE_SHIFT,
                    ((loff_t)(index + nr_pages) << PAGE_SHIFT) - 1);
       ret = shmem_add_to_page_cache(folio, mapping, index, NULL,
                         folio_gfp);
   }
   ```
   
   This silently discards whatever was previously at `@index`. For TTM's use case this might be acceptable (a previous partial backup was abandoned), but it could mask logic bugs where a caller accidentally inserts at an already-occupied index. A `WARN_ON_ONCE` or at least a `pr_debug` before the truncate would help catch misuse.

3. **shmem_inode_acct_blocks failure path:**

   ```c
   ret = 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 cache with `folio->mapping` set and its refcount bumped by `nr`. `filemap_remove_folio()` reverses this. Then `err_uncharge` does `mem_cgroup_uncharge()` + `folio_unlock()` + optionally `undo_compound_page()`. The flow appears correct — `filemap_remove_folio` should clear `folio->mapping` and drop 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; the folio lock prevents racing reclaim.

4. **Return value when writeback fails:**

   ```c
   if (writeback) {
       ret = shmem_writeout(folio, NULL, NULL);
       if (ret == AOP_WRITEPAGE_ACTIVATE) {
           folio_add_lru(folio);
           folio_unlock(folio);
       }
       /* ret == 0 or ret < 0: folio unlocked by shmem_writeout */
   }
   
   return 0;
   ```

   The function returns 0 (success) regardless of the writeback result, which matches the "best-effort" contract in the docstring. The comment about `ret < 0` is defensive — analysis of `shmem_writeout()` shows it only returns `0` (folio unlocked, swapped out) or `AOP_WRITEPAGE_ACTIVATE` (folio locked, swap failed). It never returns negative errors. The handling is correct for the current `shmem_writeout()` semantics, but the comment is misleading 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 broad for such a specialized function.

---
Generated by Claude Code Patch Reviewer