[PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

After disable periodic writeback by writing 0 to
dirty_writeback_centisecs, the handler wb_workfn() will not be
entered again until the dirty background limit reaches or
sync syscall is executed or no enough free memory available or
vmscan is triggered.
So the periodic writeback can't be enabled by writing a non-zero
value to dirty_writeback_centisecs
As it can be disabled by sysctl, it should be able to enable by 
sysctl as well.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 mm/page-writeback.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 0b9c5cb..e202f37 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
 int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
 	void __user *buffer, size_t *length, loff_t *ppos)
 {
-	proc_dointvec(table, write, buffer, length, ppos);
+	unsigned int old_interval = dirty_writeback_interval;
+	int ret;
+
+	ret = proc_dointvec(table, write, buffer, length, ppos);
+	if (!ret && !old_interval && dirty_writeback_interval)
+		wakeup_flusher_threads(0, WB_REASON_PERIODIC);
+
 	return 0;
 }
 
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Jan Kara]

On Sat 07-10-17 06:58:04, Yafang Shao wrote:
> After disable periodic writeback by writing 0 to
> dirty_writeback_centisecs, the handler wb_workfn() will not be
> entered again until the dirty background limit reaches or
> sync syscall is executed or no enough free memory available or
> vmscan is triggered.
> So the periodic writeback can't be enabled by writing a non-zero
> value to dirty_writeback_centisecs
> As it can be disabled by sysctl, it should be able to enable by 
> sysctl as well.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  mm/page-writeback.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 0b9c5cb..e202f37 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>  	void __user *buffer, size_t *length, loff_t *ppos)
>  {
> -	proc_dointvec(table, write, buffer, length, ppos);
> +	unsigned int old_interval = dirty_writeback_interval;
> +	int ret;
> +
> +	ret = proc_dointvec(table, write, buffer, length, ppos);
> +	if (!ret && !old_interval && dirty_writeback_interval)
> +		wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> +

I agree it is good to schedule some writeback. However Jens has some
changes queued in linux-block tree in this area so your change won't apply.
So please base your changes on his tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Andrew Morton]

On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:

> After disable periodic writeback by writing 0 to
> dirty_writeback_centisecs, the handler wb_workfn() will not be
> entered again until the dirty background limit reaches or
> sync syscall is executed or no enough free memory available or
> vmscan is triggered.
> So the periodic writeback can't be enabled by writing a non-zero
> value to dirty_writeback_centisecs
> As it can be disabled by sysctl, it should be able to enable by 
> sysctl as well.
> 
> ...
>
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>  	void __user *buffer, size_t *length, loff_t *ppos)
>  {
> -	proc_dointvec(table, write, buffer, length, ppos);
> +	unsigned int old_interval = dirty_writeback_interval;
> +	int ret;
> +
> +	ret = proc_dointvec(table, write, buffer, length, ppos);
> +	if (!ret && !old_interval && dirty_writeback_interval)
> +		wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> +
>  	return 0;

We could do with a code comment here, explaining why this code exists.

And...  I'm not sure it works correctly?  For example, if a device
doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
will skip it and the periodic writeback still won't be started?

(why does the dirty_writeback_interval==0 special case exist, btw? 
Seems to be a strange thing to do).

(and what happens if the interval was set to 1 hour and the user
rewrites that to 1 second?  Does that change take 1 hour to take
effect?)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

2017-10-10 6:42 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> After disable periodic writeback by writing 0 to
>> dirty_writeback_centisecs, the handler wb_workfn() will not be
>> entered again until the dirty background limit reaches or
>> sync syscall is executed or no enough free memory available or
>> vmscan is triggered.
>> So the periodic writeback can't be enabled by writing a non-zero
>> value to dirty_writeback_centisecs
>> As it can be disabled by sysctl, it should be able to enable by
>> sysctl as well.
>>
>> ...
>>
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>>       void __user *buffer, size_t *length, loff_t *ppos)
>>  {
>> -     proc_dointvec(table, write, buffer, length, ppos);
>> +     unsigned int old_interval = dirty_writeback_interval;
>> +     int ret;
>> +
>> +     ret = proc_dointvec(table, write, buffer, length, ppos);
>> +     if (!ret && !old_interval && dirty_writeback_interval)
>> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
>> +
>>       return 0;
>
> We could do with a code comment here, explaining why this code exists.
>

OK. I will comment here.

> And...  I'm not sure it works correctly?  For example, if a device
> doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
> will skip it and the periodic writeback still won't be started?
>

That's an issue.
The periodic writeback won't be started.

Maybe we'd better call  wb_wakeup_delayed(wb) here to bypass the
bdi_has_dirty_io() check ?
But then I find another issue exisit in the periodic writeback, in
function wb_workfn().

    } else if (wb_has_dirty_io(wb) && dirty_writeback_interval) {
        wb_wakeup_delayed(wb);
    }

>From the above code, we can find that if wb_has_dirty_io return false,
then bdi_writeback will not be wakeup until some other conditions
happen.
Seems we have to check periodically no matther whether there's dirty
IO or not ?

But then, introduce another issue,
If there's no dirty IO but we wakeup the bdi_writeback periodically or
do some other periodic check, there will be  performance hit .

Per my understanding, maybe the periodic writeback needs reimplement.

> (why does the dirty_writeback_interval==0 special case exist, btw?
> Seems to be a strange thing to do).
>

I agree with you.
we'd better impelment as bellow?
    if (!ret && write && dirty_writeback_interval &&
dirty_writeback_interval != old_interva)
        do_something();

> (and what happens if the interval was set to 1 hour and the user
> rewrites that to 1 second?  Does that change take 1 hour to take
> effect?)
>

If we rewirte it as above.
It will wakeup the bdi_writeback immdiately, see bellow:
    wakeup_flusher_threads
        mod_delayed_work(bdi_wq, &wb->dwork, 0);   <<< here's 0.
Next time, it will run periodically.

But is this a good implementation ?
Should we wakeup the bdi_writeback after the interval that we set?
That means, using  wb_wakeup_delayed() instead of
wakeup_flusher_threads(), that's I prefer to.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Jan Kara]

On Tue 10-10-17 16:00:29, Yafang Shao wrote:
> 2017-10-10 6:42 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> > On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> >> After disable periodic writeback by writing 0 to
> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
> >> entered again until the dirty background limit reaches or
> >> sync syscall is executed or no enough free memory available or
> >> vmscan is triggered.
> >> So the periodic writeback can't be enabled by writing a non-zero
> >> value to dirty_writeback_centisecs
> >> As it can be disabled by sysctl, it should be able to enable by
> >> sysctl as well.
> >>
> >> ...
> >>
> >> --- a/mm/page-writeback.c
> >> +++ b/mm/page-writeback.c
> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
> >>       void __user *buffer, size_t *length, loff_t *ppos)
> >>  {
> >> -     proc_dointvec(table, write, buffer, length, ppos);
> >> +     unsigned int old_interval = dirty_writeback_interval;
> >> +     int ret;
> >> +
> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
> >> +     if (!ret && !old_interval && dirty_writeback_interval)
> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> >> +
> >>       return 0;
> >
> > We could do with a code comment here, explaining why this code exists.
> >
> 
> OK. I will comment here.
> 
> > And...  I'm not sure it works correctly?  For example, if a device
> > doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
> > will skip it and the periodic writeback still won't be started?
> >
> 
> That's an issue.
> The periodic writeback won't be started.
> 
> Maybe we'd better call  wb_wakeup_delayed(wb) here to bypass the
> bdi_has_dirty_io() check ?

Well, wb_wakeup_delayed() would be more appropriate but you'd then have to
iterate over all bdis and wbs to be able to call it which IMO isn't worth
the pain for a special case like this. But the decision is worth mentioning
in the comment. Also wakeup_flusher_threads() does in principle what you
need - see my reply to Andrew for details.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

2017-10-10 16:48 GMT+08:00 Jan Kara <jack@suse.cz>:
> On Tue 10-10-17 16:00:29, Yafang Shao wrote:
>> 2017-10-10 6:42 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
>> > On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>> >
>> >> After disable periodic writeback by writing 0 to
>> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
>> >> entered again until the dirty background limit reaches or
>> >> sync syscall is executed or no enough free memory available or
>> >> vmscan is triggered.
>> >> So the periodic writeback can't be enabled by writing a non-zero
>> >> value to dirty_writeback_centisecs
>> >> As it can be disabled by sysctl, it should be able to enable by
>> >> sysctl as well.
>> >>
>> >> ...
>> >>
>> >> --- a/mm/page-writeback.c
>> >> +++ b/mm/page-writeback.c
>> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>> >>       void __user *buffer, size_t *length, loff_t *ppos)
>> >>  {
>> >> -     proc_dointvec(table, write, buffer, length, ppos);
>> >> +     unsigned int old_interval = dirty_writeback_interval;
>> >> +     int ret;
>> >> +
>> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
>> >> +     if (!ret && !old_interval && dirty_writeback_interval)
>> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
>> >> +
>> >>       return 0;
>> >
>> > We could do with a code comment here, explaining why this code exists.
>> >
>>
>> OK. I will comment here.
>>
>> > And...  I'm not sure it works correctly?  For example, if a device
>> > doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
>> > will skip it and the periodic writeback still won't be started?
>> >
>>
>> That's an issue.
>> The periodic writeback won't be started.
>>
>> Maybe we'd better call  wb_wakeup_delayed(wb) here to bypass the
>> bdi_has_dirty_io() check ?
>
> Well, wb_wakeup_delayed() would be more appropriate but you'd then have to
> iterate over all bdis and wbs to be able to call it which IMO isn't worth
> the pain for a special case like this. But the decision is worth mentioning
> in the comment. Also wakeup_flusher_threads() does in principle what you
> need - see my reply to Andrew for details.
>
>                                                                 Honza

Thanks for your explaination. I understood.
I will mention it in the comment.

Should we do the wakeup whenever dirty_writeback_interval changes ?
If we still use wakeup_flusher_threads(), it will wakeup the flusher
threads immediately after we make the change.

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Jan Kara]

On Tue 10-10-17 17:14:48, Yafang Shao wrote:
> 2017-10-10 16:48 GMT+08:00 Jan Kara <jack@suse.cz>:
> > On Tue 10-10-17 16:00:29, Yafang Shao wrote:
> >> 2017-10-10 6:42 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
> >> > On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> >> >
> >> >> After disable periodic writeback by writing 0 to
> >> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
> >> >> entered again until the dirty background limit reaches or
> >> >> sync syscall is executed or no enough free memory available or
> >> >> vmscan is triggered.
> >> >> So the periodic writeback can't be enabled by writing a non-zero
> >> >> value to dirty_writeback_centisecs
> >> >> As it can be disabled by sysctl, it should be able to enable by
> >> >> sysctl as well.
> >> >>
> >> >> ...
> >> >>
> >> >> --- a/mm/page-writeback.c
> >> >> +++ b/mm/page-writeback.c
> >> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
> >> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
> >> >>       void __user *buffer, size_t *length, loff_t *ppos)
> >> >>  {
> >> >> -     proc_dointvec(table, write, buffer, length, ppos);
> >> >> +     unsigned int old_interval = dirty_writeback_interval;
> >> >> +     int ret;
> >> >> +
> >> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
> >> >> +     if (!ret && !old_interval && dirty_writeback_interval)
> >> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> >> >> +
> >> >>       return 0;
> >> >
> >> > We could do with a code comment here, explaining why this code exists.
> >> >
> >>
> >> OK. I will comment here.
> >>
> >> > And...  I'm not sure it works correctly?  For example, if a device
> >> > doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
> >> > will skip it and the periodic writeback still won't be started?
> >> >
> >>
> >> That's an issue.
> >> The periodic writeback won't be started.
> >>
> >> Maybe we'd better call  wb_wakeup_delayed(wb) here to bypass the
> >> bdi_has_dirty_io() check ?
> >
> > Well, wb_wakeup_delayed() would be more appropriate but you'd then have to
> > iterate over all bdis and wbs to be able to call it which IMO isn't worth
> > the pain for a special case like this. But the decision is worth mentioning
> > in the comment. Also wakeup_flusher_threads() does in principle what you
> > need - see my reply to Andrew for details.
> >
> >                                                                 Honza
> 
> Thanks for your explaination. I understood.
> I will mention it in the comment.
> 
> Should we do the wakeup whenever dirty_writeback_interval changes ?
> If we still use wakeup_flusher_threads(), it will wakeup the flusher
> threads immediately after we make the change.

Yes, I think we should wakeup for every change of dirty_writeback_interval.
And immediate wakeup is not a problem IMO.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

2017-10-10 17:33 GMT+08:00 Jan Kara <jack@suse.cz>:
> On Tue 10-10-17 17:14:48, Yafang Shao wrote:
>> 2017-10-10 16:48 GMT+08:00 Jan Kara <jack@suse.cz>:
>> > On Tue 10-10-17 16:00:29, Yafang Shao wrote:
>> >> 2017-10-10 6:42 GMT+08:00 Andrew Morton <akpm@linux-foundation.org>:
>> >> > On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>> >> >
>> >> >> After disable periodic writeback by writing 0 to
>> >> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
>> >> >> entered again until the dirty background limit reaches or
>> >> >> sync syscall is executed or no enough free memory available or
>> >> >> vmscan is triggered.
>> >> >> So the periodic writeback can't be enabled by writing a non-zero
>> >> >> value to dirty_writeback_centisecs
>> >> >> As it can be disabled by sysctl, it should be able to enable by
>> >> >> sysctl as well.
>> >> >>
>> >> >> ...
>> >> >>
>> >> >> --- a/mm/page-writeback.c
>> >> >> +++ b/mm/page-writeback.c
>> >> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>> >> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>> >> >>       void __user *buffer, size_t *length, loff_t *ppos)
>> >> >>  {
>> >> >> -     proc_dointvec(table, write, buffer, length, ppos);
>> >> >> +     unsigned int old_interval = dirty_writeback_interval;
>> >> >> +     int ret;
>> >> >> +
>> >> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
>> >> >> +     if (!ret && !old_interval && dirty_writeback_interval)
>> >> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
>> >> >> +
>> >> >>       return 0;
>> >> >
>> >> > We could do with a code comment here, explaining why this code exists.
>> >> >
>> >>
>> >> OK. I will comment here.
>> >>
>> >> > And...  I'm not sure it works correctly?  For example, if a device
>> >> > doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
>> >> > will skip it and the periodic writeback still won't be started?
>> >> >
>> >>
>> >> That's an issue.
>> >> The periodic writeback won't be started.
>> >>
>> >> Maybe we'd better call  wb_wakeup_delayed(wb) here to bypass the
>> >> bdi_has_dirty_io() check ?
>> >
>> > Well, wb_wakeup_delayed() would be more appropriate but you'd then have to
>> > iterate over all bdis and wbs to be able to call it which IMO isn't worth
>> > the pain for a special case like this. But the decision is worth mentioning
>> > in the comment. Also wakeup_flusher_threads() does in principle what you
>> > need - see my reply to Andrew for details.
>> >
>> >                                                                 Honza
>>
>> Thanks for your explaination. I understood.
>> I will mention it in the comment.
>>
>> Should we do the wakeup whenever dirty_writeback_interval changes ?
>> If we still use wakeup_flusher_threads(), it will wakeup the flusher
>> threads immediately after we make the change.
>
> Yes, I think we should wakeup for every change of dirty_writeback_interval.
> And immediate wakeup is not a problem IMO.
>

Got it!


Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Jan Kara]

On Mon 09-10-17 15:42:12, Andrew Morton wrote:
> On Sat,  7 Oct 2017 06:58:04 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> > After disable periodic writeback by writing 0 to
> > dirty_writeback_centisecs, the handler wb_workfn() will not be
> > entered again until the dirty background limit reaches or
> > sync syscall is executed or no enough free memory available or
> > vmscan is triggered.
> > So the periodic writeback can't be enabled by writing a non-zero
> > value to dirty_writeback_centisecs
> > As it can be disabled by sysctl, it should be able to enable by 
> > sysctl as well.
> > 
> > ...
> >
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
> >  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
> >  	void __user *buffer, size_t *length, loff_t *ppos)
> >  {
> > -	proc_dointvec(table, write, buffer, length, ppos);
> > +	unsigned int old_interval = dirty_writeback_interval;
> > +	int ret;
> > +
> > +	ret = proc_dointvec(table, write, buffer, length, ppos);
> > +	if (!ret && !old_interval && dirty_writeback_interval)
> > +		wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> > +
> >  	return 0;
> 
> We could do with a code comment here, explaining why this code exists.
> 
> And...  I'm not sure it works correctly?  For example, if a device
> doesn't presently have bdi_has_dirty_io() then wakeup_flusher_threads()
> will skip it and the periodic writeback still won't be started?

This works correctly. For this case __mark_inode_dirty() has:

      if (bdi_cap_writeback_dirty(wb->bdi) && wakeup_bdi)
              wb_wakeup_delayed(wb);

So periodic writeback gets automatically started once first dirty inode
appears on a bdi.

> (why does the dirty_writeback_interval==0 special case exist, btw? 
> Seems to be a strange thing to do).

I guess to prevent busylooping? But I'm not sure...
 
> (and what happens if the interval was set to 1 hour and the user
> rewrites that to 1 second?  Does that change take 1 hour to take
> effect?)

That's a good point I didn't think about. So probably we should do the
wakeup whenever dirty_writeback_interval changes. 

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

2017-10-09 17:56 GMT+08:00 Jan Kara <jack@suse.cz>:
> On Sat 07-10-17 06:58:04, Yafang Shao wrote:
>> After disable periodic writeback by writing 0 to
>> dirty_writeback_centisecs, the handler wb_workfn() will not be
>> entered again until the dirty background limit reaches or
>> sync syscall is executed or no enough free memory available or
>> vmscan is triggered.
>> So the periodic writeback can't be enabled by writing a non-zero
>> value to dirty_writeback_centisecs
>> As it can be disabled by sysctl, it should be able to enable by
>> sysctl as well.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> ---
>>  mm/page-writeback.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> index 0b9c5cb..e202f37 100644
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>>       void __user *buffer, size_t *length, loff_t *ppos)
>>  {
>> -     proc_dointvec(table, write, buffer, length, ppos);
>> +     unsigned int old_interval = dirty_writeback_interval;
>> +     int ret;
>> +
>> +     ret = proc_dointvec(table, write, buffer, length, ppos);
>> +     if (!ret && !old_interval && dirty_writeback_interval)
>> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
>> +
>
> I agree it is good to schedule some writeback. However Jens has some
> changes queued in linux-block tree in this area so your change won't apply.
> So please base your changes on his tree.
>

Do you mean this tree
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git ?

I have checked his tree and find nothing need to change on my patch.

>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Jan Kara]

On Mon 09-10-17 18:44:23, Yafang Shao wrote:
> 2017-10-09 17:56 GMT+08:00 Jan Kara <jack@suse.cz>:
> > On Sat 07-10-17 06:58:04, Yafang Shao wrote:
> >> After disable periodic writeback by writing 0 to
> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
> >> entered again until the dirty background limit reaches or
> >> sync syscall is executed or no enough free memory available or
> >> vmscan is triggered.
> >> So the periodic writeback can't be enabled by writing a non-zero
> >> value to dirty_writeback_centisecs
> >> As it can be disabled by sysctl, it should be able to enable by
> >> sysctl as well.
> >>
> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >> ---
> >>  mm/page-writeback.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> >> index 0b9c5cb..e202f37 100644
> >> --- a/mm/page-writeback.c
> >> +++ b/mm/page-writeback.c
> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
> >>       void __user *buffer, size_t *length, loff_t *ppos)
> >>  {
> >> -     proc_dointvec(table, write, buffer, length, ppos);
> >> +     unsigned int old_interval = dirty_writeback_interval;
> >> +     int ret;
> >> +
> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
> >> +     if (!ret && !old_interval && dirty_writeback_interval)
> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
> >> +
> >
> > I agree it is good to schedule some writeback. However Jens has some
> > changes queued in linux-block tree in this area so your change won't apply.
> > So please base your changes on his tree.
> >
> 
> Do you mean this tree
> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git ?
> 
> I have checked his tree and find nothing need to change on my patch.

Yes, I mean that tree. Check the wb_start_all branch.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/page-writeback.c: fix bug caused by disable periodic writeback

[Yafang Shao]

2017-10-09 19:03 GMT+08:00 Jan Kara <jack@suse.cz>:
> On Mon 09-10-17 18:44:23, Yafang Shao wrote:
>> 2017-10-09 17:56 GMT+08:00 Jan Kara <jack@suse.cz>:
>> > On Sat 07-10-17 06:58:04, Yafang Shao wrote:
>> >> After disable periodic writeback by writing 0 to
>> >> dirty_writeback_centisecs, the handler wb_workfn() will not be
>> >> entered again until the dirty background limit reaches or
>> >> sync syscall is executed or no enough free memory available or
>> >> vmscan is triggered.
>> >> So the periodic writeback can't be enabled by writing a non-zero
>> >> value to dirty_writeback_centisecs
>> >> As it can be disabled by sysctl, it should be able to enable by
>> >> sysctl as well.
>> >>
>> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> >> ---
>> >>  mm/page-writeback.c | 8 +++++++-
>> >>  1 file changed, 7 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> >> index 0b9c5cb..e202f37 100644
>> >> --- a/mm/page-writeback.c
>> >> +++ b/mm/page-writeback.c
>> >> @@ -1972,7 +1972,13 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
>> >>  int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
>> >>       void __user *buffer, size_t *length, loff_t *ppos)
>> >>  {
>> >> -     proc_dointvec(table, write, buffer, length, ppos);
>> >> +     unsigned int old_interval = dirty_writeback_interval;
>> >> +     int ret;
>> >> +
>> >> +     ret = proc_dointvec(table, write, buffer, length, ppos);
>> >> +     if (!ret && !old_interval && dirty_writeback_interval)
>> >> +             wakeup_flusher_threads(0, WB_REASON_PERIODIC);
>> >> +
>> >
>> > I agree it is good to schedule some writeback. However Jens has some
>> > changes queued in linux-block tree in this area so your change won't apply.
>> > So please base your changes on his tree.
>> >
>>
>> Do you mean this tree
>> git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git ?
>>
>> I have checked his tree and find nothing need to change on my patch.
>
> Yes, I mean that tree. Check the wb_start_all branch.
>

Got it!
I will implement it base on this branch.

>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Thanks
Yafang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>