[patch]btrfs: finish read pages in the order they are submitted

[patch]btrfs: finish read pages in the order they are submitted

[Shaohua Li]

the endio is done at reverse order of bio vectors. That means for a sequential
read, the page first submitted will finish last in a bio. Considering we will
do checksum (making cache hot) for every page, this does introduce delay (and
chance to squeeze cache used soon) for pages submitted at the begining. I
don't observe obvious performance difference with below patch at my simple test,
but seems more natural to finish read in the order they are submitted.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 96577e8..4df0c56 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1750,7 +1750,8 @@ static void end_bio_extent_writepage(struct bio *bio, int err)
 static void end_bio_extent_readpage(struct bio *bio, int err)
 {
 	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
-	struct bio_vec *bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
+	struct bio_vec *bvec_end = bio->bi_io_vec + bio->bi_vcnt - 1;
+	struct bio_vec *bvec = bio->bi_io_vec;
 	struct extent_io_tree *tree;
 	u64 start;
 	u64 end;
@@ -1773,7 +1774,7 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
 		else
 			whole_page = 0;
 
-		if (--bvec >= bio->bi_io_vec)
+		if (++bvec <= bvec_end)
 			prefetchw(&bvec->bv_page->flags);
 
 		if (uptodate && tree->ops && tree->ops->readpage_end_io_hook) {
@@ -1818,7 +1819,7 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
 			}
 			check_page_locked(tree, page);
 		}
-	} while (bvec >= bio->bi_io_vec);
+	} while (bvec <= bvec_end);
 
 	bio_put(bio);
 }

Re: [patch]btrfs: finish read pages in the order they are submitted

[Chris Mason]

On Wed, Feb 03, 2010 at 03:45:11PM +0800, Shaohua Li wrote:
> the endio is done at reverse order of bio vectors. That means for a sequential
> read, the page first submitted will finish last in a bio. Considering we will
> do checksum (making cache hot) for every page, this does introduce delay (and
> chance to squeeze cache used soon) for pages submitted at the begining. I
> don't observe obvious performance difference with below patch at my simple test,
> but seems more natural to finish read in the order they are submitted.

Interesting, I wonder if we'd be able to see this on a higher throughput
system.  Jens, care to give it a shot (patch below)?

-chris


Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 96577e8..4df0c56 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1750,7 +1750,8 @@ static void end_bio_extent_writepage(struct bio *bio, int err)
 static void end_bio_extent_readpage(struct bio *bio, int err)
 {
 	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
-	struct bio_vec *bvec = bio->bi_io_vec + bio->bi_vcnt - 1;
+	struct bio_vec *bvec_end = bio->bi_io_vec + bio->bi_vcnt - 1;
+	struct bio_vec *bvec = bio->bi_io_vec;
 	struct extent_io_tree *tree;
 	u64 start;
 	u64 end;
@@ -1773,7 +1774,7 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
 		else
 			whole_page = 0;
 
-		if (--bvec >= bio->bi_io_vec)
+		if (++bvec <= bvec_end)
 			prefetchw(&bvec->bv_page->flags);
 
 		if (uptodate && tree->ops && tree->ops->readpage_end_io_hook) {
@@ -1818,7 +1819,7 @@ static void end_bio_extent_readpage(struct bio *bio, int err)
 			}
 			check_page_locked(tree, page);
 		}
-	} while (bvec >= bio->bi_io_vec);
+	} while (bvec <= bvec_end);
 
 	bio_put(bio);
 }
--
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

Re: [patch]btrfs: finish read pages in the order they are submitted

[Jens Axboe]

On Wed, Feb 03 2010, Chris Mason wrote:
> On Wed, Feb 03, 2010 at 03:45:11PM +0800, Shaohua Li wrote:
> > the endio is done at reverse order of bio vectors. That means for a sequential
> > read, the page first submitted will finish last in a bio. Considering we will
> > do checksum (making cache hot) for every page, this does introduce delay (and
> > chance to squeeze cache used soon) for pages submitted at the begining. I
> > don't observe obvious performance difference with below patch at my simple test,
> > but seems more natural to finish read in the order they are submitted.
> 
> Interesting, I wonder if we'd be able to see this on a higher throughput
> system.  Jens, care to give it a shot (patch below)?

Sure, I gave it a spin. Baseline is current -git (-rc7'ish), and the
workload is just stream reading 8 16GB files. I used large streaming
reads as the bigger ios would hopefully help show the effect of doing
the reverse completions. The run takes ~1 minute, and the results are
averaged over 3 runs.

Throughput:

Kernel          Slowest         Fastest         Average
-------------------------------------------------------
baseline        2041MB/sec      2229MB/sec      2155MB/sec
patched         2052MB/sec      2071MB/sec      2062MB/sec


Completion latency average (msecs):

Kernel          Best            Worst           Average
-------------------------------------------------------
baseline        1.72            1.89            1.79
patche          1.83            1.89            1.85


Probably would need a LOT more runs to get a statistically significant
number here, it would be nice if O_DIRECT worked (hint, hint!) which
usually makes these things easier to test. If I look at the throughput
of the runs, the baseline usually starts a little slower (1.8GB/sec or
so) and gets faster, while the patched run starts much higher (close to
3.0GB/sec) and drops to 2.0GB/sec after that for the rest of the run.

So I did some perf stat checks too, to see if we see an improvement for
cache utilization. Results below.


Cache stats (millions)

Kernel          References              Misses
----------------------------------------------
baseline        3547                    2387
patched         3822                    2351o

These numbers are very stable, the above were also averaged over 3 runs,
but variability was very low.

My feeling is that the patch should be included. Cache misses are
provably down and the patch makes a lot of sense just logically. The
patched runs seemed more stable, and my gut tells me that the unpatched
runs may have been a bit flukey (one fast run, should probably be
excluded).

Let me know if you want more tests.

-- 
Jens Axboe

Re: [patch]btrfs: finish read pages in the order they are submitted

[Jens Axboe]

On Mon, Feb 08 2010, Jens Axboe wrote:
> Cache stats (millions)
> 
> Kernel          References              Misses
> ----------------------------------------------
> baseline        3547                    2387
> patched         3822                    2351
> 
> These numbers are very stable, the above were also averaged over 3 runs,
> but variability was very low.

Update on this. I setup the storage system for more stable runs and
repeated the above test. It runs a bit faster as well, completes the
workload at 2.5GB/sec average.

Cache stats (millions)

Kernel          References              Misses
----------------------------------------------
baseline        3384                    2318
baseline        3417                    2313
baseline        3382                    2323
baseline avg    3394                    2318

patched         3518                    2258
patched         3428                    2201
patched         3536                    2274
patched avg     3494                    2244

So for those runs, ~3% more references and ~3 less misses. Even with the
variability here, that looks like a win in my book.

-- 
Jens Axboe