From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cn.fujitsu.com ([222.73.24.84]:53712 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1750789Ab2GTDvf (ORCPT ); Thu, 19 Jul 2012 23:51:35 -0400 Message-ID: <5008D5A8.4090000@cn.fujitsu.com> Date: Fri, 20 Jul 2012 11:51:04 +0800 From: Liu Bo MIME-Version: 1.0 To: David Sterba CC: chris.mason@fusionio.com, linux-btrfs@vger.kernel.org, JBacik@fusionio.com Subject: Re: [PATCH v3] Btrfs: improve multi-thread buffer read References: <1342461925-22495-1-git-send-email-liubo2009@cn.fujitsu.com> <20120718115735.GI11226@twin.jikos.cz> <50075EAA.9030806@cn.fujitsu.com> <20120719020518.GE17430@twin.jikos.cz> <50077169.4070203@cn.fujitsu.com> <20120720033610.GF17430@twin.jikos.cz> In-Reply-To: <20120720033610.GF17430@twin.jikos.cz> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >