Add check for dirty_writeback_interval in bdi_wakeup_thread_delayed
Subsystems (2)
PATCH 1/1 Add check for dirty_writeback_interval in bdi_wakeup_thread_delayed
In the function bdi_wakeup_thread_delayed, no checks are performed on dirty_writeback_interval unlike other places and timeout is being set to zero as result, thus defeating the purpose. So, I have changed it to be passed default value of interval which is 500 centiseconds, when it is set to zero.
I have also verified this and tested it.
--- mm/backing-dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
mm/backing-dev.c +4 -1
@@ -336,7 +336,10 @@ void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) {unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +if (dirty_writeback_interval) + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +else + timeout =msecs_to_jiffies (5000 );mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); }
Signature · 12 lines
1.7.4.4 -------------------------- Raghavendra Prabhu GPG Id : D72BE977 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 17 lines · Raghavendra D Prabhu
In the function
bdi_wakeup_thread_delayed, no checks are performed ondirty_writeback_intervalunlike other places and timeout is being set to zero as result, thus defeating the purpose. So, I have changed it to be passed default value of interval which is 500 centiseconds, when it is set to zero.
I have also verified this and tested it.Signed-off-by Raghavendra D Prabhu <[email protected]>--- mm/backing-dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)mm/backing-dev.c +0
-0
mm/backing-dev.c void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) · line 343
Quoted 9 lines
mm/backing-dev.c +4
-1@@ -336,7 +336,10 @@ void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) {unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +if (dirty_writeback_interval) + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +else + timeout =msecs_to_jiffies (5000 );mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); }
Isn't the problem that the sysctl handler does not have a min/max valid value set? I.e. to prevent invalid values from being set in the first place?
Cheers,
Dave.
Signature · 9 lines
Dave Chinner [email protected] -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 32 lines · * On Mon, Apr 18, 2011 at 10:02:04AM +1000, Dave Chinner <[email protected]>…
Quoted 17 lines · Raghavendra D Prabhu
In the function
bdi_wakeup_thread_delayed, no checks are performed ondirty_writeback_intervalunlike other places and timeout is being set to zero as result, thus defeating the purpose. So, I have changed it to be passed default value of interval which is 500 centiseconds, when it is set to zero.
I have also verified this and tested it.Signed-off-by Raghavendra D Prabhu <[email protected]>--- mm/backing-dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)mm/backing-dev.c +0
-0mm/backing-dev.c void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) · line 343
Quoted 9 lines
mm/backing-dev.c +4
-1@@ -336,7 +336,10 @@ void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) {unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +if (dirty_writeback_interval) + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +else + timeout =msecs_to_jiffies (5000 );mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); }Isn't the problem that the sysctl handler does not have a min/max valid value set? I.e. to prevent invalid values from being set in the first place?
Quoted 3 lines
Cheers,
Dave.
0 is a valid value for dirty_writeback_interval which according to the definition/documentation is disabled when set to 0. In other places, a constraint check is done on that value except here.
Quoted 8 lines · Raghavendra D Prabhu
In the function
bdi_wakeup_thread_delayed, no checks are performed ondirty_writeback_intervalunlike other places and timeout is being set to zero as result, thus defeating the purpose. So, I have changed it to be passed default value of interval which is 500 centiseconds, when it is set to zero.
I have also verified this and tested it.Signed-off-by Raghavendra D Prabhu <[email protected]>
If dirty_writeback_interval then the periodic write-back has to be disabled. Which means we should rather do something like this:
mm/backing-dev.c +5 -3
@@ -334,10 +334,12 @@ static void wakeup_timer_fn(unsigned long data)*/ void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi) { -unsigned long timeout; +if (dirty_writeback_interval) { +unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); -mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); + } }
I do not see why you use 500 centisecs instead - I think this is wrong.
Quoted 20 lines
--- mm/backing-dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)mm/backing-dev.c +4
-1@@ -336,7 +336,10 @@ void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) {unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +if (dirty_writeback_interval) + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +else + timeout =msecs_to_jiffies (5000 );mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); }
Signature · 9 lines
Best Regards, Artem Bityutskiy (D?N?N?N?D 1/4 D?D,N?N?N?DoD,D1) -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 46 lines · * On Mon, Apr 18, 2011 at 10:19:12AM +0300, Artem Bityutskiy…
Quoted 8 lines · Raghavendra D Prabhu
In the function
bdi_wakeup_thread_delayed, no checks are performed ondirty_writeback_intervalunlike other places and timeout is being set to zero as result, thus defeating the purpose. So, I have changed it to be passed default value of interval which is 500 centiseconds, when it is set to zero.
I have also verified this and tested it.Signed-off-by Raghavendra D Prabhu <[email protected]>If
dirty_writeback_intervalthen the periodic write-back has to be disabled. Which means we should rather do something like this:mm/backing-dev.c +5
-3@@ -334,10 +334,12 @@ static void wakeup_timer_fn(unsigned long data)*/ void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi) { -unsigned long timeout; +if (dirty_writeback_interval) { +unsigned long timeout; - timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); -mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); + } }I do not see why you use 500 centisecs instead - I think this is wrong.
Quoted 11 lines
--- mm/backing-dev.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)mm/backing-dev.c +0
-0@@ -336,7 +336,10 @@ void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) {unsigned long timeout;
Quoted 7 lines
mm/backing-dev.c void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) · line 343
Quoted 7 lines
mm/backing-dev.c +4
-1- timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +if (dirty_writeback_interval) + timeout =msecs_to_jiffies (dirty_writeback_interval *10 ); +else + timeout =msecs_to_jiffies (5000 );mod_timer (&bdi->wb.wakeup_timer, jiffies + timeout); }
Hi,
I have set it to 500 centisecs as that is the default value of dirty_writeback_interval. I used this logic for following reason: the purpose for which dirty_writeback_interval is set to 0 is to disable periodic writeback
(http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/fs/fs-writeback.c#L818)
, whereas here (in bdi_wakeup_thread_delayed) it is being used for a different purpose -- to delay the bdi wakeup in order to reduce context switches for dirty inode writeback.
Regarding the change you made: in
that case won't it end up disabling the timer altogether ? which shouldn't happen given the original purpose of defining dirty_writeback_interval to zero.
Quoted 8 lines · Raghavendra D Prabhu
I have set it to 500 centisecs as that is the default value of
dirty_writeback_interval. I used this logic for following reason: the purpose for whichdirty_writeback_intervalis set to 0 is to disable periodic writeback
(http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/fs/fs-writeback.c#L818)
, whereas here (inbdi_wakeup_thread_delayed) it is being used for a different purpose -- to delay the bdi wakeup in order to reduce context switches for dirty inode writeback.
But why it wakes up the bdi thread? Exactly to make sure the periodic write-back happen.
Signature · 9 lines
Best Regards, Artem Bityutskiy (D?N?N?N?D 1/4 D?D,N?N?N?DoD,D1) -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 10 lines · * On Mon, Apr 18, 2011 at 03:26:29PM +0300, Artem Bityutskiy…
Quoted 8 lines · Raghavendra D Prabhu
I have set it to 500 centisecs as that is the default value of
dirty_writeback_interval. I used this logic for following reason: the purpose for whichdirty_writeback_intervalis set to 0 is to disable periodic writeback
(http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/fs/fs-writeback.c#L818)
, whereas here (inbdi_wakeup_thread_delayed) it is being used for a different purpose -- to delay the bdi wakeup in order to reduce context switches for dirty inode writeback.
Quoted 2 lines
But why it wakes up the bdi thread? Exactly to make sure the periodic write-back happen.
I checked the callgraph of bdi_wakeup_thread_delayed and found out that even though it may be called in the aftermath of wb_do_writeback(), it is certainly called in the call-chain of sync. So effectively making that function do nothing when dirty_writeback_interval is unset will also make sync do nothing. On the other hand, not applying the original change at all will make it run instantly (jiffies + 0, 0 being the writeback interval in this case ) thus reversing the benefits of d7dd01adc098eadc5d5fb07a7d2bf942d09b15df.