Dave Hansen wrote: > You could create a hpagecache_nr_pages() helper or something I guess. Makes sense. > > >>> + } > >>> + __mod_zone_page_state(page_zone(page), NR_FILE_PAGES, nr); > >>> + if (PageTransHuge(page)) > >>> + __inc_zone_page_state(page, NR_FILE_TRANSPARENT_HUGEPAGES); > >>> + mapping->nrpages += nr; > >>> + spin_unlock_irq(&mapping->tree_lock); > >>> + radix_tree_preload_end(); > >>> + trace_mm_filemap_add_to_page_cache(page); > >>> + return 0; > >>> +err: > >>> + if (i != 0) > >>> + error = -ENOSPC; /* no space for a huge page */ > >>> + page_cache_release(page + i); > >>> + page[i].mapping = NULL; > >> > >> I guess it's a slight behaviour change (I think it's harmless) but if > >> you delay doing the page_cache_get() and page[i].mapping= until after > >> the radix tree insertion, you can avoid these two lines. > > > > Hm. I don't think it's safe. The spinlock protects radix-tree against > > modification, but find_get_page() can see it just after > > radix_tree_insert(). > > Except that the mapping->tree_lock is still held. I don't think > find_get_page() can find it in the radix tree without taking the lock. It can. Lookup is rcu-protected. ->tree_lock is only for add/delete/replace. > > > The page is locked and IIUC never uptodate at this point, so nobody will > > be able to do much with it, but leave it without valid ->mapping is a bad > > idea. > > ->mapping changes are protected by lock_page(). You can't keep > ->mapping stable without holding it. If you unlock_page(), you have to > recheck ->mapping after you reacquire the lock. > > In other words, I think the code is fine. You are right. > > >> I'm also trying to figure out how and when you'd actually have to unroll > >> a partial-huge-page worth of radix_tree_insert(). In the small-page > >> case, you can collide with another guy inserting in to the page cache. > >> But, can that happen in the _middle_ of a THP? > > > > E.g. if you enable THP after some uptime, the mapping can contain small pages > > already. > > Or if a process map the file with bad alignement (MAP_FIXED) and touch the > > area, it will get small pages. > > Could you put a comment in explaining this case a bit? It's a bit subtle. okay. -- Kirill A. Shutemov -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
