From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/gem-shmem: Track folio accessed/dirty status in
 mmap
Date: Wed, 11 Feb 2026 17:04:34 +1000
Message-ID: <review-patch5-20260209133241.238813-6-tzimmermann@suse.de>
In-Reply-To: <20260209133241.238813-6-tzimmermann@suse.de>
References: <20260209133241.238813-1-tzimmermann@suse.de> <20260209133241.238813-6-tzimmermann@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Review:**

Core functionality: adds folio tracking in mmap path.

**Accessed tracking:**
```c
+	if (likely(!(ret & VM_FAULT_ERROR)))
+		folio_mark_accessed(folio);
```

This marks the folio as accessed on successful page fault. Good for LRU management.

**Issue 1 - PMD vs PTE tracking:**
The `folio_mark_accessed()` is called regardless of whether a PMD or PTE was inserted. For PMD mappings (huge pages), marking the folio accessed is correct since the whole folio was mapped. Good.

**Dirty tracking:**
```c
+static vm_fault_t drm_gem_shmem_pfn_mkwrite(struct vm_fault *vmf)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	struct drm_gem_object *obj = vma->vm_private_data;
+	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff;
+	struct page *page = shmem->pages[page_offset];
+	struct folio *folio = page_folio(page);
+
+	file_update_time(vma->vm_file);
+	folio_mark_dirty(folio);
+
+	return 0;
+}
```

**Issue 2 - Missing locking:**
The `pfn_mkwrite` handler accesses `shmem->pages[page_offset]` without holding `obj->resv` lock. Compare with the `fault` handler which does:

```c
dma_resv_lock(obj->resv, NULL);
page = pages[page_offset];
...
dma_resv_unlock(obj->resv);
```

**This is a race condition.** Between the time the page is faulted and mkwrite is called, the pages array could be freed/reallocated. The `pfn_mkwrite` needs to:
1. Lock `obj->resv`
2. Verify `shmem->pages` is still valid
3. Verify the page at `page_offset` is still valid
4. Mark dirty
5. Unlock

**Issue 3 - PMD mkwrite:**
As mentioned in PATCH 4, PMDs are inserted read-only:
```c
vmf_insert_pfn_pmd(vmf, pfn, false);  // false = read-only
```

But `pfn_mkwrite` only handles PTE granularity (it uses `vmf->pgoff` to get a single page). **There's no `huge_fault` or PMD-specific mkwrite handler.** This means:

1. PMD inserted read-only
2. Write occurs
3. PMD is split by kernel into PTEs (expensive!)
4. PTE mkwrite called

This defeats the purpose of huge page mappings for write-heavy workloads. Should either:
- Insert PMDs with write permission if mapping allows
- Implement PMD-granularity mkwrite

**Issue 4 - File timestamp update:**
```c
file_update_time(vma->vm_file);
```

This is called on every write fault. For frequently-written pages, this could cause significant overhead. Traditional page cache code only updates time periodically, not on every mkwrite. Consider using `file_modified()` with appropriate checks, or rate-limiting updates.

**Issue 5 - Return value:**
```c
return 0;
```

The function returns 0 (success) unconditionally. What if `folio_mark_dirty()` fails or the folio is in a bad state? Should validate folio state before marking dirty.

**Verdict:** ⚠ Missing locking (critical bug), PMD handling incomplete, potential performance issues

---

---
Generated by Claude Code Patch Reviewer