Re: [PATCH 03/14] mm: Protect operations adding pages to page cache with invalidate_lock

Christoph Hellwig
2021-07-13T06:25:48Z

Still looks good. That being said the additional conditional locking in
filemap_fault makes it fall over the readbility cliff for me. Something
like this on top of your series would help:

diff --git a/mm/filemap.c b/mm/filemap.c
index fd3f94d36c49..0fad08331cf4 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3040,21 +3040,23 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
* Do we have something in the page cache already?
*/
page = find_get_page(mapping, offset);
- if (likely(page) && !(vmf->flags & FAULT_FLAG_TRIED)) {
+ if (likely(page)) {
/*
- * We found the page, so try async readahead before
- * waiting for the lock.
+ * We found the page, so try async readahead before waiting for
+ * the lock.
*/
- fpin = do_async_mmap_readahead(vmf, page);
- } else if (!page) {
+ if (!(vmf->flags & FAULT_FLAG_TRIED))
+ fpin = do_async_mmap_readahead(vmf, page);
+ if (unlikely(!PageUptodate(page))) {
+ filemap_invalidate_lock_shared(mapping);
+ mapping_locked = true;
+ }
+ } else {
/* No page in the page cache at all */
count_vm_event(PGMAJFAULT);
count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT);
ret = VM_FAULT_MAJOR;
fpin = do_sync_mmap_readahead(vmf);
- }
-
- if (!page) {
retry_find:
/*
* See comment in filemap_create_page() why we need
@@ -3073,9 +3075,6 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
filemap_invalidate_unlock_shared(mapping);
return VM_FAULT_OOM;
}
- } else if (unlikely(!PageUptodate(page))) {
- filemap_invalidate_lock_shared(mapping);
- mapping_locked = true;
}

if (!lock_page_maybe_drop_mmap(vmf, page, &fpin))