On Fri 07-12-18 10:57:50, Jan Kara wrote: > On Fri 30-11-18 14:58:10, Josef Bacik wrote: > > If we do not have a page at filemap_fault time we'll do this weird > > forced page_cache_read thing to populate the page, and then drop it > > again and loop around and find it. This makes for 2 ways we can read a > > page in filemap_fault, and it's not really needed. Instead add a > > FGP_FOR_MMAP flag so that pagecache_get_page() will return a unlocked > > page that's in pagecache. Then use the normal page locking and readpage > > logic already in filemap_fault. This simplifies the no page in page > > cache case significantly. > > > > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx> > > Thanks for the patch. I like the simplification but I think it could be > even improved... see below. > > > @@ -2449,9 +2426,11 @@ vm_fault_t filemap_fault(struct vm_fault *vmf) > > count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT); > > ret = VM_FAULT_MAJOR; > > retry_find: > > - page = find_get_page(mapping, offset); > > + page = pagecache_get_page(mapping, offset, > > + FGP_CREAT|FGP_FOR_MMAP, > > + vmf->gfp_mask); > > if (!page) > > - goto no_cached_page; > > + return vmf_error(-ENOMEM); > > So why don't you just do: > > page = pagecache_get_page(mapping, offset, > FGP_CREAT | FGP_LOCK, vmf->gfp_mask); > if (!page) > return vmf_error(-ENOMEM); > goto check_uptodate; > > where check_uptodate would be a label before 'PageUptodate' check? > > Then you don't have to introduce new flag for pagecache_get_page() and you > also don't have to unlock and then lock again the page... And you can still > delete all the code you've deleted. Ah, you don't want lock_page() to block in case someone raced with you and instantiated the page so that you can drop mmap_sem. OK, the patch looks good to me then. You can add: Reviewed-by: Jan Kara <jack@xxxxxxx> Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
