I was more concerend with the multiple ways in which the readhead changed, and the fact that we were going down the sequential read path on random I/O requests. More comments are always good, but that was not really my concern.
I have enclosed 5 patches that address each of the issues.
1 . Code is obtuse and hard to maintain
The best I could do is update the comments to reflect the
current code. Hopefully that should help.
attached patch 1_comment.patch takes care of that part to
some extent.
I think this will be way too agressive. This means turn off readhead if 1 max_readhead I/O is completely in cache. You then will need to do multiple 4k I/Os to get it turned back on.
2. page cache hits not handled properly.
I fixed this by decrementing the size of the next readahead window
by the number of pages hit in the page cache. Now it slowly
accomodates the page cache hits.
attached patch 2_cachehits.patch takes care of this issue.
3. queue congestion not handled.Yeah, I had thought about something along these lines. Just not sure if it is worth it.
The fix is: call force_page_cache_readahead() if we are populating pages in the current window.
And call do_page_cache_readahead() if we are populating
pages in the ahead window. However if do_page_cache_readahead()
return with congestion, the readahead window is collapsed back to size zero. This will ensure that the next time ahead window
is attempted to populate.
attached patch 3_queuecongestion.patch handles this issue.
4. page thrash handled ineffectively.Same comments as on 2. Way too agressive.
The fix is: on page thrash detection shutdown readahead.
attached patch 4_pagethrash.patch handles this issue.
5. slow read path is too slow.Step in the right direction. Now if I could just get you to pick up the rest we would be done :-)
I could not figure out a way to atleast-read-the-requested-
number-of-pages if readahead is shutdown, without incorporating
the readsize parameter to page_cache_readahead(). So had
to pick some of your code in filemap.c to do that. Thanks!
attached patch 5_fixedslowread.patch handles this issue.
Apart from this you have noticed other issuesUmm, actually you do (if the code works). When you get too many cache hits you turn off readahead. This will disable the multiple lookups until you re-enable readhead which will only happen in handle_ra_miss which means you are reading pages not in page cache so that is ok.
6. cache lookup done unneccessrily twice for pagecache_hits.
I have not handled this issue currently. But should be doable
if I introducing a flag, which notes when readahead is
shutdown by pagecahche hits. And hence attempts to lookup
the page only once.
And you have other features in your patch which will be the realGlad we agree.
differentiating factors.
7. exponential expand and shrink of window sizes.
8. overlapped read of current window and ahead window.
( I think both are desirable feature )
I did run some premilinary tests using your patch and the above patchesCare to expand on slightly better? Also these tests don't cover many cases. You are only running iozone single threaded which won't show much, you don't seem to vary IO requests sizes to see the effect (all this based on your readahead web page). Also it would be really helpful if you had some sort of machine description, disk, memory etc. Also for the DSS workload I need ot know what type of IO pattern you generate. I know it is mostly random IO, but the size of the IOs and the number of prefetcher threads and the number of disks make a huge difference.
and found
your patch was doing slightly better on iozone and sysbench.
however the above patch were doing slightly better with DSS workload.
But my setup is rather tiny compared to your setup, so my comparisonNot really. I made sure that I ran this on machine from single cpu ide up through really big machines. It is important to run well across a variety of platforms which I tried to ensure my code does.
is rather incomplete.