From: Fengguang Wu <wfg@...>
Subject: [PATCH] writeback: remove unnecessary wait in throttle_vm_writeout()
Date: Sep 26, 9:50 pm 2007
We don't want to introduce pointless delays in throttle_vm_writeout()
when the writeback limits are not yet exceeded, do we?
Cc: Nick Piggin
Cc: OGAWA Hirofumi
Cc: Kumar Gala
Cc: Pete Zaitcev
Cc: Greg KH
Signed-off-by: Fengguang Wu
---
mm/page-writeback.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
--- linux-2.6.23-rc8-mm1.orig/mm/page-writeback.c
+++ linux-2.6.23-rc8-mm1/mm/page-writeback.c
@@ -507,16 +507,6 @@ void throttle_vm_writeout(gfp_t gfp_mask
long background_thresh;
long dirty_thresh;
- if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO)) {
- /*
- * The caller might hold locks which can prevent IO completion
- * or progress in the filesystem. So we cannot just sit here
- * waiting for IO to complete.
- */
- congestion_wait(WRITE, HZ/10);
- return;
- }
-
for ( ; ; ) {
get_dirty_limits(&background_thresh, &dirty_thresh, NULL, NULL);
@@ -530,6 +520,14 @@ void throttle_vm_writeout(gfp_t gfp_mask
global_page_state(NR_WRITEBACK) <= dirty_thresh)
break;
congestion_wait(WRITE, HZ/10);
+
+ /*
+ * The caller might hold locks which can prevent IO completion
+ * or progress in the filesystem. So we cannot just sit here
+ * waiting for IO to complete.
+ */
+ if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO))
+ break;
}
}
-
From: Andrew Morton <akpm@...>
Subject: Re: [PATCH] writeback: remove unnecessary wait in throttle_vm_writeout()
Date: Sep 27, 4:47 pm 2007
On Thu, 27 Sep 2007 09:50:16 +0800
Fengguang Wu wrote:
> We don't want to introduce pointless delays in throttle_vm_writeout()
> when the writeback limits are not yet exceeded, do we?
>
> Cc: Nick Piggin
> Cc: OGAWA Hirofumi
> Cc: Kumar Gala
> Cc: Pete Zaitcev
> Cc: Greg KH
> Signed-off-by: Fengguang Wu
> ---
> mm/page-writeback.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> --- linux-2.6.23-rc8-mm1.orig/mm/page-writeback.c
> +++ linux-2.6.23-rc8-mm1/mm/page-writeback.c
> @@ -507,16 +507,6 @@ void throttle_vm_writeout(gfp_t gfp_mask
> long background_thresh;
> long dirty_thresh;
>
> - if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO)) {
> - /*
> - * The caller might hold locks which can prevent IO completion
> - * or progress in the filesystem. So we cannot just sit here
> - * waiting for IO to complete.
> - */
> - congestion_wait(WRITE, HZ/10);
> - return;
> - }
> -
> for ( ; ; ) {
> get_dirty_limits(&background_thresh, &dirty_thresh, NULL, NULL);
>
> @@ -530,6 +520,14 @@ void throttle_vm_writeout(gfp_t gfp_mask
> global_page_state(NR_WRITEBACK) <= dirty_thresh)
> break;
> congestion_wait(WRITE, HZ/10);
> +
> + /*
> + * The caller might hold locks which can prevent IO completion
> + * or progress in the filesystem. So we cannot just sit here
> + * waiting for IO to complete.
> + */
> + if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO))
> + break;
> }
> }
>
This is a pretty major bugfix.
GFP_NOIO and GFP_NOFS callers should have been spending really large
amounts of time stuck in that sleep.
I wonder why nobody noticed this happening. Either a) it turns out that
kswapd is doing a good job and such callers don't do direct reclaim much or
b) nobody is doing any in-depth kernel instrumentation.
Now, how _would_ one notice this problem? We don't have very good tools,
really. Booting with "profile=sleep" and looking at the profile data would
be one way. Repeatedly doing sysrq-T is another. Perhaps the new
lockstat-via-lockdep code would allow this to be observed in some fashion,
dunno.
Anyway, this patch has the potential to significantly alter the dynamics of
the VM behaviour under particular workloads. It might turn up other
stuff...
-
Bad style
A for(;;) loop without a safety net would not make it through review in many compliance environments (FAA, FDA, ...). I know looping on register values until they change state is a common technique in embedded programming but it is not robust. It is better to at least log a message after an unreasonable number of iterations
Read the code
The for loop just writes out data, sleeps while the IO device is congested, writes out more data, etc. until enough data has been written out (dirty threshold passed) and then just breaks out of the loop. What's wrong with that?
I see it, its still bad
If had ever worked in embedded programming and have had to debug some yahoo's infinite loop that "had to terminate" you'd know. Go back to your Java.
I won't buy
Quipping something to the effect of "just take my word for it" and proceeding to insult the parent commenter is never a convincing way to express yourself.
So what are the alternatives to for(;;) { if(condition) break; }?
You could just as easily write a legitimate-seeming while(condition) loop that never terminates. The only difference between them is that the condition must be located in a different place. How exactly does that avoid bugs? To the contrary, rewriting your code just to move the condition to the appropriate place often increases complexity and produces hacks instead of improving anything.
So, come again?
This is really bad advice.
This is really bad advice. You only need arbitrary "safety nets" if you don't adequately understand the problem you're working on. If that's the case, you shouldn't be working on that problem at all.