cfq-iosched: fixing RQ_NOIDLE handling.
in [Kernel]
|
Prev: cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD
Next: [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD
From: Vivek Goyal on 8 Jul 2010 10:40 On Wed, Jul 07, 2010 at 01:03:08PM -0400, Jeff Moyer wrote: > Corrado Zoccolo <czoccolo(a)gmail.com> writes: > > > Hi Jens, > > patch 8e55063 "cfq-iosched: fix corner cases in idling logic", is > > suspected for some regressions on high end hardware. > > The two patches from this series: > > - [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle > > - [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD > > fix two issues that I have identified, related to how RQ_NOIDLE is > > used by the upper layers. > > First patch makes sure that a RQ_NOIDLE coming after a sequence of > > possibly idling requests from the same queue on the no-idle tree will > > clear the noidle_tree_requires_idle flag. > > Second patch enables RQ_NOIDLE for queues in the idling tree, > > restoring the behaviour pre-8e55063 patch. > > Hi, Corrado, > > I ran your kernel through my tests. Here are the results, up against > vanilla, deadline, and the blk_yield patch set: > > just just > fs_mark fio mixed > -------------------------------+-------------- > deadline 529.44 151.4 | 450.0 78.2 > vanilla cfq 107.88 164.4 | 6.6 137.2 > blk_yield cfq 530.82 158.7 | 113.2 78.6 > corrado cfq 80.82 138.1 | 4.5 130.7 > > fs_mark results are in files/second, fio results are in MB/s. All > results are the average of 5 runs. In order to get results for the > mixed workload for both vanilla and Corrado's kernels, I had to extend > the runtime from 30s to 300s. > > So, the changes proposed in this thread actually make performance worse > across the board. > > I re-ran my tests against a RHEL 5 kernel (which is based on 2.6.18), > and it shows that fs_mark performance is much better than stock CFQ in > 2.6.35-rc3, and the mixed workload results are much the same as they are > now (which is to say, the fs_mark process is completely starved by the > sequential reader). So, that problem has existed for a long time. > > I'm still in the process of collecting data from production servers and > will report back with my findings there. Hi Jeff and all, How about if we simply get rid of idling on RQ_NOIDLE threads (as corrado's patch series does) and not try to solve the problem of fsync being starved in the presence of sequential readers. I mean it might just be a theoritical problem and not many people are running into it. That's how CFQ has been behaving for long-2 time and if nobody is complaining then we probably don't have to fix it. Thanks Vivek -- 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: Jeff Moyer on 8 Jul 2010 10:40 Vivek Goyal <vgoyal(a)redhat.com> writes: > On Wed, Jul 07, 2010 at 01:03:08PM -0400, Jeff Moyer wrote: >> Corrado Zoccolo <czoccolo(a)gmail.com> writes: >> >> > Hi Jens, >> > patch 8e55063 "cfq-iosched: fix corner cases in idling logic", is >> > suspected for some regressions on high end hardware. >> > The two patches from this series: >> > - [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle >> > - [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD >> > fix two issues that I have identified, related to how RQ_NOIDLE is >> > used by the upper layers. >> > First patch makes sure that a RQ_NOIDLE coming after a sequence of >> > possibly idling requests from the same queue on the no-idle tree will >> > clear the noidle_tree_requires_idle flag. >> > Second patch enables RQ_NOIDLE for queues in the idling tree, >> > restoring the behaviour pre-8e55063 patch. >> >> Hi, Corrado, >> >> I ran your kernel through my tests. Here are the results, up against >> vanilla, deadline, and the blk_yield patch set: >> >> just just >> fs_mark fio mixed >> -------------------------------+-------------- >> deadline 529.44 151.4 | 450.0 78.2 >> vanilla cfq 107.88 164.4 | 6.6 137.2 >> blk_yield cfq 530.82 158.7 | 113.2 78.6 >> corrado cfq 80.82 138.1 | 4.5 130.7 >> >> fs_mark results are in files/second, fio results are in MB/s. All >> results are the average of 5 runs. In order to get results for the >> mixed workload for both vanilla and Corrado's kernels, I had to extend >> the runtime from 30s to 300s. >> >> So, the changes proposed in this thread actually make performance worse >> across the board. >> >> I re-ran my tests against a RHEL 5 kernel (which is based on 2.6.18), >> and it shows that fs_mark performance is much better than stock CFQ in >> 2.6.35-rc3, and the mixed workload results are much the same as they are >> now (which is to say, the fs_mark process is completely starved by the >> sequential reader). So, that problem has existed for a long time. >> >> I'm still in the process of collecting data from production servers and >> will report back with my findings there. > > Hi Jeff and all, > > How about if we simply get rid of idling on RQ_NOIDLE threads (as > corrado's patch series does) and not try to solve the problem of fsync > being starved in the presence of sequential readers. I mean it might just > be a theoritical problem and not many people are running into it. That's > how CFQ has been behaving for long-2 time and if nobody is complaining > then we probably don't have to fix it. I would instead suggest we just revert that one commit, if this is the route we're going to go. Please keep in mind, though, that folks who may have experienced this issue may also have just switched to deadline. Cheers, Jeff -- 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: Corrado Zoccolo on 8 Jul 2010 10:40 On Wed, Jul 7, 2010 at 10:06 PM, Jeff Moyer <jmoyer(a)redhat.com> wrote: > Corrado Zoccolo <czoccolo(a)gmail.com> writes: > >> On Wed, Jul 7, 2010 at 7:03 PM, Jeff Moyer <jmoyer(a)redhat.com> wrote: >>> Corrado Zoccolo <czoccolo(a)gmail.com> writes: >>> >>>> Hi Jens, >>>> patch 8e55063 "cfq-iosched: fix corner cases in idling logic", is >>>> suspected for some regressions on high end hardware. >>>> The two patches from this series: >>>> - [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle >>>> - [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD >>>> fix two issues that I have identified, related to how RQ_NOIDLE is >>>> used by the upper layers. >>>> First patch makes sure that a RQ_NOIDLE coming after a sequence of >>>> possibly idling requests from the same queue on the no-idle tree will >>>> clear the noidle_tree_requires_idle flag. >>>> Second patch enables RQ_NOIDLE for queues in the idling tree, >>>> restoring the behaviour pre-8e55063 patch. >>> >>> Hi, Corrado, >>> >>> I ran your kernel through my tests. Here are the results, up against >>> vanilla, deadline, and the blk_yield patch set: >>> >> Hi Jeff, >> can you also add cfq with 8e55063 reverted to the testing mix? > > Sure, the results now look like this: > > just just > fs_mark fio mixed > -------------------------------+-------------- > deadline 529.44 151.4 | 450.0 78.2 > vanilla cfq 107.88 164.4 | 6.6 137.2 > blk_yield cfq 530.82 158.7 | 113.2 78.6 > corrado cfq 110.16 220.6 | 7.0 159.8 > 8e55063 revert 559.66 198.9 | 16.1 153.3 > > I had accidentally run your patch set (corrado cfq) on ext3, so the > numbers were a bit off (everything else was run against ext4). The > corrected numbers above reflect the performance on ext4, which is much > better for the sequential reader, but still not great for the fs_mark > run. Reverting 8e55063 definitely gets us into better shape. However, > if we care about the mixed workload, then it won't be enough. I wonder why 8e55063 revert gives such a big improvement on fsync ops. Maybe, before 8e55063, we ended up not idling even if cfq_arm_slice_timer was called, due to other requests still pending? I think your patch that allows both async and sync requests to be in flight at the same time could help here. > > It's worth noting that I can't explain that jump from 151MB/s for > deadline vs 220MB/s for corrado cfq. I'm not sure how you can vary > driving a single queue depth sequential read at all. Those are the > averages of 5 runs and this storage should be solely accessible by me, > so I am at a loss. I guess ext4 tries to be smart, and issues some background reads of fs data structures needed to keep reading the sequential file without interruption. Those reads will be far from the current head, so if you service them immediately (as deadline would), they can cause a degradation, while delaying them to when you are servicing other random requests (as cfq would) can help. > > Cheers, > Jeff > -- __________________________________________________________________________ dott. Corrado Zoccolo mailto:czoccolo(a)gmail.com PhD - Department of Computer Science - University of Pisa, Italy -------------------------------------------------------------------------- The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness. Tales of Power - C. Castaneda -- 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: Corrado Zoccolo on 8 Jul 2010 10:50 On Thu, Jul 8, 2010 at 4:35 PM, Vivek Goyal <vgoyal(a)redhat.com> wrote: > On Wed, Jul 07, 2010 at 01:03:08PM -0400, Jeff Moyer wrote: >> Corrado Zoccolo <czoccolo(a)gmail.com> writes: >> >> > Hi Jens, >> > patch 8e55063 "cfq-iosched: fix corner cases in idling logic", is >> > suspected for some regressions on high end hardware. >> > The two patches from this series: >> > - [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle >> > - [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD >> > fix two issues that I have identified, related to how RQ_NOIDLE is >> > used by the upper layers. >> > First patch makes sure that a RQ_NOIDLE coming after a sequence of >> > possibly idling requests from the same queue on the no-idle tree will >> > clear the noidle_tree_requires_idle flag. >> > Second patch enables RQ_NOIDLE for queues in the idling tree, >> > restoring the behaviour pre-8e55063 patch. >> >> Hi, Corrado, >> >> I ran your kernel through my tests. Here are the results, up against >> vanilla, deadline, and the blk_yield patch set: >> >> just just >> fs_mark fio mixed >> -------------------------------+-------------- >> deadline 529.44 151.4 | 450.0 78.2 >> vanilla cfq 107.88 164.4 | 6.6 137.2 >> blk_yield cfq 530.82 158.7 | 113.2 78.6 >> corrado cfq 80.82 138.1 | 4.5 130.7 >> >> fs_mark results are in files/second, fio results are in MB/s. All >> results are the average of 5 runs. In order to get results for the >> mixed workload for both vanilla and Corrado's kernels, I had to extend >> the runtime from 30s to 300s. >> >> So, the changes proposed in this thread actually make performance worse >> across the board. >> >> I re-ran my tests against a RHEL 5 kernel (which is based on 2.6.18), >> and it shows that fs_mark performance is much better than stock CFQ in >> 2.6.35-rc3, and the mixed workload results are much the same as they are >> now (which is to say, the fs_mark process is completely starved by the >> sequential reader). So, that problem has existed for a long time. >> >> I'm still in the process of collecting data from production servers and >> will report back with my findings there. > > Hi Jeff and all, > > How about if we simply get rid of idling on RQ_NOIDLE threads (as > corrado's patch series does) and not try to solve the problem of fsync > being starved in the presence of sequential readers. I mean it might just > be a theoritical problem and not many people are running into it. That's > how CFQ has been behaving for long-2 time and if nobody is complaining > then we probably don't have to fix it. 8e55063 was done to fix theoretical problems as well :) I think, instead, that Jeff's approach of yielding the queue when a better knowledge is present is good, and this set of patches is not intended as a replacement. It is intended just to fix some regressions introduced by a previous commit, and I hope it could work together with Jeff's patch. Clearly, if RQ_NOIDLE is used only in the places that Jeff is already handling, then it is better to completely remove RQ_NOIDLE handling, so my patch set becomes obsolete. Thanks, Corrado > > Thanks > Vivek > -- __________________________________________________________________________ dott. Corrado Zoccolo mailto:czoccolo(a)gmail.com PhD - Department of Computer Science - University of Pisa, Italy -------------------------------------------------------------------------- The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness. Tales of Power - C. Castaneda -- 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: Corrado Zoccolo on 9 Jul 2010 06:40
On Wed, Jul 7, 2010 at 10:06 PM, Jeff Moyer <jmoyer(a)redhat.com> wrote: > Corrado Zoccolo <czoccolo(a)gmail.com> writes: > >> On Wed, Jul 7, 2010 at 7:03 PM, Jeff Moyer <jmoyer(a)redhat.com> wrote: >>> Corrado Zoccolo <czoccolo(a)gmail.com> writes: >>> >>>> Hi Jens, >>>> patch 8e55063 "cfq-iosched: fix corner cases in idling logic", is >>>> suspected for some regressions on high end hardware. >>>> The two patches from this series: >>>> - [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle >>>> - [PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD >>>> fix two issues that I have identified, related to how RQ_NOIDLE is >>>> used by the upper layers. >>>> First patch makes sure that a RQ_NOIDLE coming after a sequence of >>>> possibly idling requests from the same queue on the no-idle tree will >>>> clear the noidle_tree_requires_idle flag. >>>> Second patch enables RQ_NOIDLE for queues in the idling tree, >>>> restoring the behaviour pre-8e55063 patch. >>> >>> Hi, Corrado, >>> >>> I ran your kernel through my tests.  Here are the results, up against >>> vanilla, deadline, and the blk_yield patch set: >>> >> Hi Jeff, >> can you also add cfq with 8e55063 reverted to the testing mix? > > Sure, the results now look like this: > >         just   just >         fs_mark  fio     mixed > -------------------------------+-------------- > deadline     529.44  151.4 | 450.0   78.2 > vanilla cfq   107.88  164.4 |  6.6  137.2 > blk_yield cfq  530.82  158.7 | 113.2   78.6 > corrado cfq   110.16  220.6 |  7.0  159.8 > 8e55063 revert  559.66  198.9 |  16.1  153.3 > > I had accidentally run your patch set (corrado cfq) on ext3, so the > numbers were a bit off (everything else was run against ext4).  The > corrected numbers above reflect the performance on ext4, which is much > better for the sequential reader, but still not great for the fs_mark > run.  Reverting 8e55063 definitely gets us into better shape.  However, > if we care about the mixed workload, then it won't be enough. Wondering why deadline performs so well in the fs_mark workload. Is it because it doesn't distinguish between sync and async writes? Maybe we can achieve something similar by putting all sync writes (that are marked as REQ_NOIDLE) in the noidle tree? This, coupled with making jbd(2) perform sync writes, should make the yield automatic, since they all live in the same tree for which we don't idle between queues, and should be able to provide fairness compared to a sequential reader (that lives in the other tree). Can you test the attached patch, where I also added your changes to make jbd(2) to perform sync writes? Thanks, Corrado > > It's worth noting that I can't explain that jump from 151MB/s for > deadline vs 220MB/s for corrado cfq.  I'm not sure how you can vary > driving a single queue depth sequential read at all.  Those are the > averages of 5 runs and this storage should be solely accessible by me, > so I am at a loss. > > Cheers, > Jeff > -- __________________________________________________________________________ dott. Corrado Zoccolo             mailto:czoccolo(a)gmail.com PhD - Department of Computer Science - University of Pisa, Italy -------------------------------------------------------------------------- The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness.                 Tales of Power - C. Castaneda |