Fix Readahead stalling by plugged device queues
in [Kernel]
|
Prev: [tip:perf/urgent] perf, x86: Properly account n_added
Next: [tip:perf/pebs] perf: Add attr->precise support to raw event parsing
From: Wu Fengguang on 10 Mar 2010 08:20 > --- linux.orig/mm/readahead.c > +++ linux/mm/readahead.c > @@ -188,8 +188,11 @@ __do_page_cache_readahead(struct address > * uptodate then the caller will launch readpage again, and > * will then handle the error. > */ > - if (ret) > + if (ret) { > read_pages(mapping, filp, &page_pool, ret); > + /* unplug backing dev to avoid latencies */ > + blk_run_address_space(mapping); > + } Christian, did you notice this commit for 2.6.33? commit 65a80b4c61f5b5f6eb0f5669c8fb120893bfb388 Author: Hisashi Hifumi <hifumi.hisashi(a)oss.ntt.co.jp> Date: Thu Dec 17 15:27:26 2009 -0800 readahead: add blk_run_backing_dev --- a/mm/readahead.c +++ b/mm/readahead.c @@ -547,5 +547,17 @@ page_cache_async_readahead(struct address_space *mapping, /* do read-ahead */ ondemand_readahead(mapping, ra, filp, true, offset, req_size); + +#ifdef CONFIG_BLOCK + /* + * Normally the current page is !uptodate and lock_page() will be + * immediately called to implicitly unplug the device. However this + * is not always true for RAID conifgurations, where data arrives + * not strictly in their submission order. In this case we need to + * explicitly kick off the IO. + */ + if (PageUptodate(page)) + blk_run_backing_dev(mapping->backing_dev_info, NULL); +#endif } It should at least improve performance between .32 and .33, because once two readahead requests are merged into one single IO request, the PageUptodate() will be true at next readahead, and hence blk_run_backing_dev() get called to break out of the suboptimal situation. Your patch does reduce the possible readahead submit latency to 0. Is your workload a simple dd on a single disk? If so, it sounds like something illogical hidden in the block layer. Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Christian Ehrhardt on 10 Mar 2010 09:40 Wu Fengguang wrote: [...] > Christian, did you notice this commit for 2.6.33? > > commit 65a80b4c61f5b5f6eb0f5669c8fb120893bfb388 [...] I didn't see that particular one, due to the fact that whatever the result is it needs to work .32 Anyway I'll test it tomorrow and if that already accepted one fixes my issue as well I'll recommend distros older than 2.6.33 picking that one up in their on top patches. > > It should at least improve performance between .32 and .33, because > once two readahead requests are merged into one single IO request, > the PageUptodate() will be true at next readahead, and hence > blk_run_backing_dev() get called to break out of the suboptimal > situation. As you saw from my blktrace thats already the case without that patch. Once the second readahead comes in and merged it gets unplugged in 2.6.32 too - but still that is bad behavior as it denies my things like 68% throughput improvement :-). > > Your patch does reduce the possible readahead submit latency to 0. yeah and I think/hope that is fine, because as I stated: - low utilized disk -> not an issue - high utilized disk -> unplug is an noop At least personally I consider a case where merging of a readahead window with anything except its own sibling very rare - and therefore fair to unplug after and RA is submitted. > Is your workload a simple dd on a single disk? If so, it sounds like > something illogical hidden in the block layer. It might still be illogical hidden as e.g. 2.6.27 unplugged after the first readahead as well :-) But no my load is iozone running with different numbers of processes with one disk per process. That neatly resembles e.g. nightly backup jobs which tend to take longer and longer in all time increasing customer scenarios. Such an improvement might banish the backups back to the night were they belong :-) > Thanks, > Fengguang -- Gr�sse / regards, Christian Ehrhardt IBM Linux Technology Center, System z Linux Performance -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Wu Fengguang on 10 Mar 2010 20:50 On Wed, Mar 10, 2010 at 10:31:46PM +0800, Christian Ehrhardt wrote: > > > Wu Fengguang wrote: > [...] > > Christian, did you notice this commit for 2.6.33? > > > > commit 65a80b4c61f5b5f6eb0f5669c8fb120893bfb388 > [...] > > I didn't see that particular one, due to the fact that whatever the > result is it needs to work .32 > > Anyway I'll test it tomorrow and if that already accepted one fixes my > issue as well I'll recommend distros older than 2.6.33 picking that one > up in their on top patches. OK, thanks! > > > > It should at least improve performance between .32 and .33, because > > once two readahead requests are merged into one single IO request, > > the PageUptodate() will be true at next readahead, and hence > > blk_run_backing_dev() get called to break out of the suboptimal > > situation. > > As you saw from my blktrace thats already the case without that patch. > Once the second readahead comes in and merged it gets unplugged in > 2.6.32 too - but still that is bad behavior as it denies my things like > 68% throughput improvement :-). I mean, when readahead windows A and B are submitted in one IO -- let's call it AB -- commit 65a80b4c61 will explicitly unplug on doing readahead C. While in your trace, the unplug appears on AB. The 68% improvement is very impressive. Wondering if commit 65a80b4c61 (the _conditional_ unplug) can achieve the same level of improvement :) > > > > Your patch does reduce the possible readahead submit latency to 0. > > yeah and I think/hope that is fine, because as I stated: > - low utilized disk -> not an issue > - high utilized disk -> unplug is an noop > > At least personally I consider a case where merging of a readahead > window with anything except its own sibling very rare - and therefore > fair to unplug after and RA is submitted. They are reasonable assumptions. However I'm not sure if this unconditional unplug will defeat CFQ's anticipatory logic -- if there are any. You know commit 65a80b4c61 is more about a *defensive* protection against the rare case that two readahead windows get merged. > > Is your workload a simple dd on a single disk? If so, it sounds like > > something illogical hidden in the block layer. > > It might still be illogical hidden as e.g. 2.6.27 unplugged after the > first readahead as well :-) > But no my load is iozone running with different numbers of processes > with one disk per process. > That neatly resembles e.g. nightly backup jobs which tend to take longer > and longer in all time increasing customer scenarios. Such an > improvement might banish the backups back to the night were they belong :-) Exactly one process per disk? Are they doing sequential reads or more complicated access patterns? Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Christian Ehrhardt on 11 Mar 2010 05:00 Wu Fengguang wrote: > On Wed, Mar 10, 2010 at 10:31:46PM +0800, Christian Ehrhardt wrote: >> >> Wu Fengguang wrote: >> [...] >>> Christian, did you notice this commit for 2.6.33? >>> >>> commit 65a80b4c61f5b5f6eb0f5669c8fb120893bfb388 >> [...] >> >> I didn't see that particular one, due to the fact that whatever the >> result is it needs to work .32 >> >> Anyway I'll test it tomorrow and if that already accepted one fixes my >> issue as well I'll recommend distros older than 2.6.33 picking that one >> up in their on top patches. > > OK, thanks! That patch fixes my issue completely and is as we discussed less aggressive which is fine - thanks for pointing it out - Now I have something already upstream accepted to fix the issue, thats much better! >>> It should at least improve performance between .32 and .33, because >>> once two readahead requests are merged into one single IO request, >>> the PageUptodate() will be true at next readahead, and hence >>> blk_run_backing_dev() get called to break out of the suboptimal >>> situation. >> As you saw from my blktrace thats already the case without that patch. >> Once the second readahead comes in and merged it gets unplugged in >> 2.6.32 too - but still that is bad behavior as it denies my things like >> 68% throughput improvement :-). > > I mean, when readahead windows A and B are submitted in one IO -- > let's call it AB -- commit 65a80b4c61 will explicitly unplug on doing > readahead C. While in your trace, the unplug appears on AB. > > The 68% improvement is very impressive. Wondering if commit 65a80b4c61 > (the _conditional_ unplug) can achieve the same level of improvement :) Yep it can ! We can post update the patch description to bigger numbers :-) >>> Your patch does reduce the possible readahead submit latency to 0. >> yeah and I think/hope that is fine, because as I stated: >> - low utilized disk -> not an issue >> - high utilized disk -> unplug is an noop >> >> At least personally I consider a case where merging of a readahead >> window with anything except its own sibling very rare - and therefore >> fair to unplug after and RA is submitted. > > They are reasonable assumptions. However I'm not sure if this > unconditional unplug will defeat CFQ's anticipatory logic -- if there > are any. You know commit 65a80b4c61 is more about a *defensive* > protection against the rare case that two readahead windows get > merged. > >>> Is your workload a simple dd on a single disk? If so, it sounds like >>> something illogical hidden in the block layer. >> It might still be illogical hidden as e.g. 2.6.27 unplugged after the >> first readahead as well :-) >> But no my load is iozone running with different numbers of processes >> with one disk per process. >> That neatly resembles e.g. nightly backup jobs which tend to take longer >> and longer in all time increasing customer scenarios. Such an >> improvement might banish the backups back to the night were they belong :-) > > Exactly one process per disk? Are they doing sequential reads or more > complicated access patterns? Just sequential read where I see the win, but I also had sequential write, and random read/write as well as some mixed stuff like dbench. It improved sequential read and did not impact the others which is fine. Thank you for you quick replies! > Thanks, > Fengguang -- Gr�sse / regards, Christian Ehrhardt IBM Linux Technology Center, System z Linux Performance -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Wu Fengguang on 11 Mar 2010 08:30
On Thu, Mar 11, 2010 at 05:58:08PM +0800, Christian Ehrhardt wrote: > Wu Fengguang wrote: > > On Wed, Mar 10, 2010 at 10:31:46PM +0800, Christian Ehrhardt wrote: > >> > >> Wu Fengguang wrote: > >> [...] > >>> Christian, did you notice this commit for 2.6.33? > >>> > >>> commit 65a80b4c61f5b5f6eb0f5669c8fb120893bfb388 > >> [...] > >> > >> I didn't see that particular one, due to the fact that whatever the > >> result is it needs to work .32 > >> > >> Anyway I'll test it tomorrow and if that already accepted one fixes my > >> issue as well I'll recommend distros older than 2.6.33 picking that one > >> up in their on top patches. > > > > OK, thanks! > > That patch fixes my issue completely and is as we discussed less > aggressive which is fine - thanks for pointing it out - Now I have > something already upstream accepted to fix the issue, thats much better! That's great news, it works beyond my expectation.. :) > >>> It should at least improve performance between .32 and .33, because > >>> once two readahead requests are merged into one single IO request, > >>> the PageUptodate() will be true at next readahead, and hence > >>> blk_run_backing_dev() get called to break out of the suboptimal > >>> situation. > >> As you saw from my blktrace thats already the case without that patch. > >> Once the second readahead comes in and merged it gets unplugged in > >> 2.6.32 too - but still that is bad behavior as it denies my things like > >> 68% throughput improvement :-). > > > > I mean, when readahead windows A and B are submitted in one IO -- > > let's call it AB -- commit 65a80b4c61 will explicitly unplug on doing > > readahead C. While in your trace, the unplug appears on AB. > > > > The 68% improvement is very impressive. Wondering if commit 65a80b4c61 > > (the _conditional_ unplug) can achieve the same level of improvement :) > > Yep it can ! > We can post update the patch description to bigger numbers :-) Andrew/Greg, shall we push the patch to .32 stable? That would give us an opportunity to change the patch description ;) > >>> Your patch does reduce the possible readahead submit latency to 0. > >> yeah and I think/hope that is fine, because as I stated: > >> - low utilized disk -> not an issue > >> - high utilized disk -> unplug is an noop > >> > >> At least personally I consider a case where merging of a readahead > >> window with anything except its own sibling very rare - and therefore > >> fair to unplug after and RA is submitted. > > > > They are reasonable assumptions. However I'm not sure if this > > unconditional unplug will defeat CFQ's anticipatory logic -- if there > > are any. You know commit 65a80b4c61 is more about a *defensive* > > protection against the rare case that two readahead windows get > > merged. > > > >>> Is your workload a simple dd on a single disk? If so, it sounds like > >>> something illogical hidden in the block layer. > >> It might still be illogical hidden as e.g. 2.6.27 unplugged after the > >> first readahead as well :-) > >> But no my load is iozone running with different numbers of processes > >> with one disk per process. > >> That neatly resembles e.g. nightly backup jobs which tend to take longer > >> and longer in all time increasing customer scenarios. Such an > >> improvement might banish the backups back to the night were they belong :-) > > > > Exactly one process per disk? Are they doing sequential reads or more > > complicated access patterns? > > Just sequential read where I see the win, but I also had sequential > write, and random read/write as well as some mixed stuff like dbench. > It improved sequential read and did not impact the others which is fine. Ah OK. > Thank you for you quick replies! You are welcome~ Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |