On 07/21/2012 02:42 AM, Zach Brown wrote:

quoted

+	struct page *page;
+	int i = 0;
+	int nr = 0;

i doesn't need to be initialized.

quoted

 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {
-		struct page *page = list_entry(pages->prev, struct page, lru);
+		page = list_entry(pages->prev, struct page, lru);
 
 		prefetchw(&page->flags);
 		list_del(&page->lru);
 		if (!add_to_page_cache_lru(page, mapping,
 					page->index, GFP_NOFS)) {
-			__extent_read_full_page(tree, page, get_extent,
+			page_cache_get(page);
+			pagepool[nr++] = page;
+			if (nr == 16) {

ARRAY_SIZE(pagepool) instead of duplicating 16.

quoted

+				for (i = 0; i < nr; i++) {
+					__extent_read_full_page(tree,
+						pagepool[i], get_extent,
 						&bio, 0, &bio_flags);
+					page_cache_release(pagepool[i]);
+				}
+				nr = 0;
+			}
 		}
 		page_cache_release(page);

It looks like you can optimize away a page cache ref here.  Don't add a
ref when the page is added to the pool, instead use the existing ref.
Then only release this ref here if add_to_page_cache_lru() succeeds.
Then always release the ref when __extent_read_full_page is called on
the pages in the pool.

Sounds good, it makes btrfs's readpages hook a little different with others' though.

I'd also invert the nested if()s to reduce the painful indent level:

  		if (add_to_page_cache_lru(page, mapping,
  					page->index, GFP_NOFS)) {
			page_cache_release(page);
			continue;
		}

		pagepool[nr++] = page;
		if (nr < ARRAY_SIZE(pagepool))
			continue;

		for (i = 0; i < nr; i++) {
			__extent_read_full_page(tree, ...

- z

I'll update it.  Thanks Zach.

thanks,
liubo