On 07/20/2012 11:36 AM, David Sterba wrote:
> On Thu, Jul 19, 2012 at 10:31:05AM +0800, Liu Bo wrote:
>>> 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.
>
>>>> 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]
>
>> --- 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
>
>> + }
>> }
>> 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
>
>> +
>> 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.
>> 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.
>>> 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).
>
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
