On 07/20/2012 11:36 AM, David Sterba wrote:

On Thu, Jul 19, 2012 at 10:31:05AM +0800, Liu Bo wrote:

quoted

quoted

128 is too much, this would snip 128 * 8 = 1K off the stack.

That's why I give up 128. :)

It's good as a reference point, nobody says it should stay at 128.

quoted

quoted

quoted

But as Chris suggested, my test is really a race case in practical use, half of improvement
is somehow enough, so we turn to use pagevec struct because it is closer to how we solve
similar problems in other parts of the kernel.

Yes it's an optimization, nice and simple one, but I don't see the
use of pagevec justified. By the other parts of kernel is probably meant
memory management, and pagevec's are used along with lookups to inode
mappings, plus there are other sideefects on pagecache (like calling
lru_add_drain() from pagevec_release, as can be seen in your code).

Filesystems can use pagevec_lookup instead of find_get_pages,
like ext4 does, but btrfs uses simple arrays of 16 pages, in
lock_delalloc_pages, end_compressed_writeback, __unlock_for_delalloc and
extent_clear_unlock_delalloc (in connection with find_get_pages).

I was specifically interested in benchmarking pagevec used as in V3
against simple array with 16 elements, but now that I looked around
while writing this mail, I think that pagevec is not the way to go.

Sorry, I see no difference between 16 pages array and pagevec(14 pages),

The difference is 2 pages, at least. Besides [quoting patch from the
first post for reference]

quoted

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c

@@ -3557,7 +3557,10 @@ int extent_readpages(struct extent_io_tree *tree,
        struct bio *bio = NULL;
        unsigned page_idx;
        unsigned long bio_flags = 0;
+       struct pagevec pvec;
+       int i = 0;

+       pagevec_init(&pvec, 0);
        for (page_idx = 0; page_idx < nr_pages; page_idx++) {
                struct page *page = list_entry(pages->prev, struct page, lru);

@@ -3565,11 +3568,22 @@ int extent_readpages(struct extent_io_tree *tree,
                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);
+                       if (pagevec_add(&pvec, page) == 0) {
+                               for (i = 0; i < pagevec_count(&pvec); i++)
+                                       __extent_read_full_page(tree,
+                                               pvec.pages[i], get_extent,
                                                &bio, 0, &bio_flags);
+                               pagevec_release(&pvec);

                                  ^^^^^^^^^^^^^^^^^^^^^^
here

quoted

+                       }
                }
                page_cache_release(page);
        }
+       for (i = 0; i < pagevec_count(&pvec); i++)
+               __extent_read_full_page(tree, pvec.pages[i], get_extent,
+                                       &bio, 0, &bio_flags);
+       pagevec_release(&pvec);

          ^^^^^^^^^^^^^^^^^^^^^^
and here

quoted

+
        BUG_ON(!list_empty(pages));
        if (bio)
                return submit_one_bio(READ, bio, 0, bio_flags);

you actually call pagevec_release. And I pointed out that this is not a
simple operation (like the other pagevec_* functions just doing some
arithmetics) -- it calls lru_add_drain(), this does lots of things with
pagecache and LRU lists, follow the call chain from there if you don't
believe me.

well, you're totally right.  It does make some side effects.

quoted

and I have no idea why ext4 use 16 pages array(maybe historical
reasons),

sigh, I didn't say that ext4 uses 16 pointer array, quite the opposite:

oh, sorry, I owe you.

quoted

quoted

like ext4 does, but btrfs uses simple arrays of 16 pages, in
lock_delalloc_pages, end_compressed_writeback, __unlock_for_delalloc and
extent_clear_unlock_delalloc (in connection with find_get_pages).

quoted

but IMO it is proper and natural to use pagevec to manage pages.

As you've benchmarked, the more pages one can batch here at once the
better and I don't see why we should miss the opportunity for 2 another
pages just because it's shorter/nicer to write it via pagevec's.

Thanks for your explanation and review, I will give it a hit ASAP.

thanks,
liubo

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html