Hi.
shrink_page_list passes gfp_mask to try_to_release_page.
When shrink_page_list is called from kswapd or buddy system, gfp_mask is set
and (gfp_mask & __GFP_WAIT) and (gfp_mask & __GFP_FS) check is positive.
releasepage of jbd/jbd2(ext3/4, ocfs2) and XFS use this parameter.
If try_to_free_page fails due to bh busy in jbd/jbd2, jbd/jbd2 lets a thread wait for
committing transaction. I think this has big performance impacts for vmscan.
So I modified shrink_page_list not to pass gfp_mask to try_to_release_page
in ordered to improve vmscan performance.
Thanks.
Signed-off-by: Hisashi Hifumi <[email protected]>
diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c linux-2.6.27-rc2.vmscan/mm/vmscan.c
--- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
+++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
@@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
* Otherwise, leave the page on the LRU so it is swappable.
*/
if (PagePrivate(page)) {
- if (!try_to_release_page(page, sc->gfp_mask))
+ if (!try_to_release_page(page, 0))
goto activate_locked;
if (!mapping && page_count(page) == 1) {
unlock_page(page);
On Wed, 13 Aug 2008 11:21:16 +0900 Hisashi Hifumi <[email protected]> wrote:
> Hi.
>
> shrink_page_list passes gfp_mask to try_to_release_page.
> When shrink_page_list is called from kswapd or buddy system, gfp_mask is set
> and (gfp_mask & __GFP_WAIT) and (gfp_mask & __GFP_FS) check is positive.
> releasepage of jbd/jbd2(ext3/4, ocfs2) and XFS use this parameter.
> If try_to_free_page fails due to bh busy in jbd/jbd2, jbd/jbd2 lets a thread wait for
> committing transaction. I think this has big performance impacts for vmscan.
> So I modified shrink_page_list not to pass gfp_mask to try_to_release_page
> in ordered to improve vmscan performance.
>
> Thanks.
>
> Signed-off-by: Hisashi Hifumi <[email protected]>
>
> diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c linux-2.6.27-rc2.vmscan/mm/vmscan.c
> --- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
> +++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
> @@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
> * Otherwise, leave the page on the LRU so it is swappable.
> */
> if (PagePrivate(page)) {
> - if (!try_to_release_page(page, sc->gfp_mask))
> + if (!try_to_release_page(page, 0))
> goto activate_locked;
> if (!mapping && page_count(page) == 1) {
> unlock_page(page);
I think the change makes sense.
Has this change been shown to improve any workloads? If so, please
provide full information for the changelog. If not, please mention
this and explain why benefits were not demonstrable. This information
should _always_ be present in a "performance" patch's changelog!
Probably a better fix would be to explicitly tell
journal_try_to_free_buffers() when it need to block on journal commit,
rather than (mis)interpreting the gfp_t in this fashion. I assume the
only caller who really cares is direct-io. That would be quite a bit
of churn, and the asynchronous behaviour perhaps makes sense _anyway_
when called from page reclaim.
otoh, there is a risk that this change will cause page reclaim to sit
there burning huge amounts of CPU time and not achieving anything,
because all it is doing is scanning over busy pages. In that case,
blocking behind a commit which would make those pages reclaimable is
correct behaviour. But given that the offending code in
journal_try_to_free_buffers() has only been there for a few weeks, I
guess this isn't a concern.
Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
race between free buffer and commit transaction") was an unpleasant
hack which had undesirable and unexpected side-effects. I think - that
depends upon your as-yet-undisclosed testing results?
Perhaps we should revert 3f31fddf and have another think about how to
fix the direct-io -EIO problem. One option would be to hold our noses
and add a new gfp_t flag for this specific purpose?
At 12:21 08/08/13, Andrew Morton wrote:
>On Wed, 13 Aug 2008 11:21:16 +0900 Hisashi Hifumi
><[email protected]> wrote:
>
>> Hi.
>>
>> shrink_page_list passes gfp_mask to try_to_release_page.
>> When shrink_page_list is called from kswapd or buddy system, gfp_mask is set
>> and (gfp_mask & __GFP_WAIT) and (gfp_mask & __GFP_FS) check is positive.
>> releasepage of jbd/jbd2(ext3/4, ocfs2) and XFS use this parameter.
>> If try_to_free_page fails due to bh busy in jbd/jbd2, jbd/jbd2 lets a
>thread wait for
>> committing transaction. I think this has big performance impacts for vmscan.
>> So I modified shrink_page_list not to pass gfp_mask to try_to_release_page
>> in ordered to improve vmscan performance.
>>
>> Thanks.
>>
>> Signed-off-by: Hisashi Hifumi <[email protected]>
>>
>> diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c linux-2.6.27-rc2.vmscan/mm/vmscan.c
>> --- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
>> +++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
>> @@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
>> * Otherwise, leave the page on the LRU so it is swappable.
>> */
>> if (PagePrivate(page)) {
>> - if (!try_to_release_page(page, sc->gfp_mask))
>> + if (!try_to_release_page(page, 0))
>> goto activate_locked;
>> if (!mapping && page_count(page) == 1) {
>> unlock_page(page);
>
>I think the change makes sense.
>
>Has this change been shown to improve any workloads? If so, please
>provide full information for the changelog. If not, please mention
>this and explain why benefits were not demonstrable. This information
>should _always_ be present in a "performance" patch's changelog!
Sorry, I do not have performance number yet. I'll try this.
>
>Probably a better fix would be to explicitly tell
>journal_try_to_free_buffers() when it need to block on journal commit,
>rather than (mis)interpreting the gfp_t in this fashion. I assume the
>only caller who really cares is direct-io. That would be quite a bit
>of churn, and the asynchronous behaviour perhaps makes sense _anyway_
>when called from page reclaim.
>
>otoh, there is a risk that this change will cause page reclaim to sit
>there burning huge amounts of CPU time and not achieving anything,
>because all it is doing is scanning over busy pages. In that case,
>blocking behind a commit which would make those pages reclaimable is
>correct behaviour. But given that the offending code in
>journal_try_to_free_buffers() has only been there for a few weeks, I
>guess this isn't a concern.
>
>
>Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
>race between free buffer and commit transaction") was an unpleasant
>hack which had undesirable and unexpected side-effects. I think - that
>depends upon your as-yet-undisclosed testing results?
>
>Perhaps we should revert 3f31fddf and have another think about how to
>fix the direct-io -EIO problem. One option would be to hold our noses
>and add a new gfp_t flag for this specific purpose?
Currently, we are discussing about direct-io -EIO problem because the patch
("jbd: fix race between free buffer and commit transaction") was not
enough to fix the issue.
The ML subject of this discussion is
"[PATCH] jbd jbd2: fix diowritereturningEIOwhentry_to_release_page fails".
On Wed, 13 Aug 2008 15:24:40 +0900
Hisashi Hifumi <[email protected]> wrote:
> At 12:21 08/08/13, Andrew Morton wrote:
> >On Wed, 13 Aug 2008 11:21:16 +0900 Hisashi Hifumi
> ><[email protected]> wrote:
> >
> >> Hi.
> >>
> >> shrink_page_list passes gfp_mask to try_to_release_page.
> >> When shrink_page_list is called from kswapd or buddy system, gfp_mask is set
> >> and (gfp_mask & __GFP_WAIT) and (gfp_mask & __GFP_FS) check is positive.
> >> releasepage of jbd/jbd2(ext3/4, ocfs2) and XFS use this parameter.
> >> If try_to_free_page fails due to bh busy in jbd/jbd2, jbd/jbd2 lets a
> >thread wait for
> >> committing transaction. I think this has big performance impacts for vmscan.
> >> So I modified shrink_page_list not to pass gfp_mask to try_to_release_page
> >> in ordered to improve vmscan performance.
> >>
> >> Thanks.
> >>
> >> Signed-off-by: Hisashi Hifumi <[email protected]>
> >>
> >> diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c linux-2.6.27-rc2.vmscan/mm/vmscan.c
> >> --- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
> >> +++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
> >> @@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
> >> * Otherwise, leave the page on the LRU so it is swappable.
> >> */
> >> if (PagePrivate(page)) {
> >> - if (!try_to_release_page(page, sc->gfp_mask))
> >> + if (!try_to_release_page(page, 0))
> >> goto activate_locked;
> >> if (!mapping && page_count(page) == 1) {
> >> unlock_page(page);
> >
> >I think the change makes sense.
> >
> >Has this change been shown to improve any workloads? If so, please
> >provide full information for the changelog. If not, please mention
> >this and explain why benefits were not demonstrable. This information
> >should _always_ be present in a "performance" patch's changelog!
>
> Sorry, I do not have performance number yet. I'll try this.
>
This patch remains in a stalled state...
And then there's this:
: Probably a better fix would be to explicitly tell
: journal_try_to_free_buffers() when it need to block on journal commit,
: rather than (mis)interpreting the gfp_t in this fashion. I assume the
: only caller who really cares is direct-io. That would be quite a bit
: of churn, and the asynchronous behaviour perhaps makes sense _anyway_
: when called from page reclaim.
:
: otoh, there is a risk that this change will cause page reclaim to sit
: there burning huge amounts of CPU time and not achieving anything,
: because all it is doing is scanning over busy pages. In that case,
: blocking behind a commit which would make those pages reclaimable is
: correct behaviour. But given that the offending code in
: journal_try_to_free_buffers() has only been there for a few weeks, I
: guess this isn't a concern.
:
:
: Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
: race between free buffer and commit transaction") was an unpleasant
: hack which had undesirable and unexpected side-effects. I think - that
: depends upon your as-yet-undisclosed testing results?
:
: Perhaps we should revert 3f31fddf and have another think about how to
: fix the direct-io -EIO problem. One option would be to hold our noses
: and add a new gfp_t flag for this specific purpose?
:
Hi Andrew.
>Hisashi Hifumi <[email protected]> wrote:
>
>> At 12:21 08/08/13, Andrew Morton wrote:
>> >On Wed, 13 Aug 2008 11:21:16 +0900 Hisashi Hifumi
>> ><[email protected]> wrote:
>> >> Signed-off-by: Hisashi Hifumi <[email protected]>
>> >>
>> >> diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c
>linux-2.6.27-rc2.vmscan/mm/vmscan.c
>> >> --- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
>> >> +++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
>> >> @@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
>> >> * Otherwise, leave the page on the LRU so it is swappable.
>> >> */
>> >> if (PagePrivate(page)) {
>> >> - if (!try_to_release_page(page, sc->gfp_mask))
>> >> + if (!try_to_release_page(page, 0))
>> >> goto activate_locked;
>> >> if (!mapping && page_count(page) == 1) {
>> >> unlock_page(page);
>> >
>> >I think the change makes sense.
>> >
>> >Has this change been shown to improve any workloads? If so, please
>> >provide full information for the changelog. If not, please mention
>> >this and explain why benefits were not demonstrable. This information
>> >should _always_ be present in a "performance" patch's changelog!
>>
>> Sorry, I do not have performance number yet. I'll try this.
>>
>
Unfortunately, I did not succeed to get good performance number that
prove this patch had some benefit.
>This patch remains in a stalled state...
>
>And then there's this:
>
>:
>: Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
>: race between free buffer and commit transaction") was an unpleasant
>: hack which had undesirable and unexpected side-effects. I think - that
>: depends upon your as-yet-undisclosed testing results?
>:
>: Perhaps we should revert 3f31fddf and have another think about how to
>: fix the direct-io -EIO problem. One option would be to hold our noses
>: and add a new gfp_t flag for this specific purpose?
>:
direct-io -EIO problem was already fixed by following patch.
commit 6ccfa806a9cfbbf1cd43d5b6aa47ef2c0eb518fd
Author: Hisashi Hifumi <[email protected]>
Date: Tue Sep 2 14:35:40 2008 -0700
VFS: fix dio write returning EIO when try_to_release_page fails
Dio falls back to buffered write when dio write gets EIO due to failure of try_to_release_page
by above patch. So I think just reverting the patch 3f31fddf ("jbd: fix race between
free buffer and commit transaction") is good approach.
On Thu, 16 Oct 2008 11:44:39 +0900 Hisashi Hifumi <[email protected]> wrote:
> Hi Andrew.
>
> >Hisashi Hifumi <[email protected]> wrote:
> >
> >> At 12:21 08/08/13, Andrew Morton wrote:
> >> >On Wed, 13 Aug 2008 11:21:16 +0900 Hisashi Hifumi
> >> ><[email protected]> wrote:
>
> >> >> Signed-off-by: Hisashi Hifumi <[email protected]>
> >> >>
> >> >> diff -Nrup linux-2.6.27-rc2.org/mm/vmscan.c
> >linux-2.6.27-rc2.vmscan/mm/vmscan.c
> >> >> --- linux-2.6.27-rc2.org/mm/vmscan.c 2008-08-11 14:33:24.000000000 +0900
> >> >> +++ linux-2.6.27-rc2.vmscan/mm/vmscan.c 2008-08-12 18:57:05.000000000 +0900
> >> >> @@ -614,7 +614,7 @@ static unsigned long shrink_page_list(st
> >> >> * Otherwise, leave the page on the LRU so it is swappable.
> >> >> */
> >> >> if (PagePrivate(page)) {
> >> >> - if (!try_to_release_page(page, sc->gfp_mask))
> >> >> + if (!try_to_release_page(page, 0))
> >> >> goto activate_locked;
> >> >> if (!mapping && page_count(page) == 1) {
> >> >> unlock_page(page);
> >> >
> >> >I think the change makes sense.
> >> >
> >> >Has this change been shown to improve any workloads? If so, please
> >> >provide full information for the changelog. If not, please mention
> >> >this and explain why benefits were not demonstrable. This information
> >> >should _always_ be present in a "performance" patch's changelog!
> >>
> >> Sorry, I do not have performance number yet. I'll try this.
> >>
> >
>
> Unfortunately, I did not succeed to get good performance number that
> prove this patch had some benefit.
OK, thanks, I dropped it.
> >This patch remains in a stalled state...
> >
> >And then there's this:
> >
>
> >:
> >: Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
> >: race between free buffer and commit transaction") was an unpleasant
> >: hack which had undesirable and unexpected side-effects. I think - that
> >: depends upon your as-yet-undisclosed testing results?
> >:
> >: Perhaps we should revert 3f31fddf and have another think about how to
> >: fix the direct-io -EIO problem. One option would be to hold our noses
> >: and add a new gfp_t flag for this specific purpose?
> >:
>
> direct-io -EIO problem was already fixed by following patch.
>
> commit 6ccfa806a9cfbbf1cd43d5b6aa47ef2c0eb518fd
> Author: Hisashi Hifumi <[email protected]>
> Date: Tue Sep 2 14:35:40 2008 -0700
>
> VFS: fix dio write returning EIO when try_to_release_page fails
>
> Dio falls back to buffered write when dio write gets EIO due to failure of try_to_release_page
> by above patch. So I think just reverting the patch 3f31fddf ("jbd: fix race between
> free buffer and commit transaction") is good approach.
Fair enough. Could I ask that you (or someone) send a suitable patch
sometime?
I could generate the patch, but I'd never get around to testing it.
Too busy fixing rejects and compile errors :(
At 11:54 08/10/16, Andrew Morton wrote:
>On Thu, 16 Oct 2008 11:44:39 +0900 Hisashi Hifumi
><[email protected]> wrote:
>
>>
>> Unfortunately, I did not succeed to get good performance number that
>> prove this patch had some benefit.
>
>OK, thanks, I dropped it.
>
>> >This patch remains in a stalled state...
>> >
>> >And then there's this:
>> >
>>
>> >:
>> >: Really, I think what this patch tells us is that 3f31fddf ("jbd: fix
>> >: race between free buffer and commit transaction") was an unpleasant
>> >: hack which had undesirable and unexpected side-effects. I think - that
>> >: depends upon your as-yet-undisclosed testing results?
>> >:
>> >: Perhaps we should revert 3f31fddf and have another think about how to
>> >: fix the direct-io -EIO problem. One option would be to hold our noses
>> >: and add a new gfp_t flag for this specific purpose?
>> >:
>>
>> direct-io -EIO problem was already fixed by following patch.
>>
>> commit 6ccfa806a9cfbbf1cd43d5b6aa47ef2c0eb518fd
>> Author: Hisashi Hifumi <[email protected]>
>> Date: Tue Sep 2 14:35:40 2008 -0700
>>
>> VFS: fix dio write returning EIO when try_to_release_page fails
>>
>> Dio falls back to buffered write when dio write gets EIO due to failure
>of try_to_release_page
>> by above patch. So I think just reverting the patch 3f31fddf ("jbd: fix
>race between
>> free buffer and commit transaction") is good approach.
>
>Fair enough. Could I ask that you (or someone) send a suitable patch
>sometime?
Yes, sometimes I send you some bug fixing or performance improvement
patch.
>
>I could generate the patch, but I'd never get around to testing it.
>Too busy fixing rejects and compile errors :(