| Message ID | 1505850787-18311-6-git-send-email-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 19, 2017 at 01:53:06PM -0600, Jens Axboe wrote: > Now that we have no external callers of wb_start_writeback(), > we can move the nr_pages == 0 logic into that function. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
On Tue 19-09-17 13:53:06, Jens Axboe wrote: > Now that we have no external callers of wb_start_writeback(), > we can move the nr_pages == 0 logic into that function. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> ... > +static unsigned long get_nr_dirty_pages(void) > +{ > + return global_node_page_state(NR_FILE_DIRTY) + > + global_node_page_state(NR_UNSTABLE_NFS) + > + get_nr_dirty_inodes(); > +} > + > static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, > bool range_cyclic, enum wb_reason reason) > { > @@ -942,6 +953,12 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, > return; > > /* > + * If someone asked for zero pages, we write out the WORLD > + */ > + if (!nr_pages) > + nr_pages = get_nr_dirty_pages(); > + So for 'wb' we have a better estimate of the amount we should write - use wb_stat_sum(wb, WB_RECLAIMABLE) statistics - that is essentially dirty + unstable_nfs broken down to bdi_writeback. Honza
On 09/20/2017 08:41 AM, Jan Kara wrote: > On Tue 19-09-17 13:53:06, Jens Axboe wrote: >> Now that we have no external callers of wb_start_writeback(), >> we can move the nr_pages == 0 logic into that function. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > ... > >> +static unsigned long get_nr_dirty_pages(void) >> +{ >> + return global_node_page_state(NR_FILE_DIRTY) + >> + global_node_page_state(NR_UNSTABLE_NFS) + >> + get_nr_dirty_inodes(); >> +} >> + >> static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, >> bool range_cyclic, enum wb_reason reason) >> { >> @@ -942,6 +953,12 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, >> return; >> >> /* >> + * If someone asked for zero pages, we write out the WORLD >> + */ >> + if (!nr_pages) >> + nr_pages = get_nr_dirty_pages(); >> + > > So for 'wb' we have a better estimate of the amount we should write - use > wb_stat_sum(wb, WB_RECLAIMABLE) statistics - that is essentially dirty + > unstable_nfs broken down to bdi_writeback. I don't mind making that change, but I think that should be a separate patch. We're using get_nr_dirty_pages() in existing locations where we have the 'wb', like in wb_check_old_data_flush().
On Wed 20-09-17 09:05:51, Jens Axboe wrote: > On 09/20/2017 08:41 AM, Jan Kara wrote: > > On Tue 19-09-17 13:53:06, Jens Axboe wrote: > >> Now that we have no external callers of wb_start_writeback(), > >> we can move the nr_pages == 0 logic into that function. > >> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > > ... > > > >> +static unsigned long get_nr_dirty_pages(void) > >> +{ > >> + return global_node_page_state(NR_FILE_DIRTY) + > >> + global_node_page_state(NR_UNSTABLE_NFS) + > >> + get_nr_dirty_inodes(); > >> +} > >> + > >> static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, > >> bool range_cyclic, enum wb_reason reason) > >> { > >> @@ -942,6 +953,12 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, > >> return; > >> > >> /* > >> + * If someone asked for zero pages, we write out the WORLD > >> + */ > >> + if (!nr_pages) > >> + nr_pages = get_nr_dirty_pages(); > >> + > > > > So for 'wb' we have a better estimate of the amount we should write - use > > wb_stat_sum(wb, WB_RECLAIMABLE) statistics - that is essentially dirty + > > unstable_nfs broken down to bdi_writeback. > > I don't mind making that change, but I think that should be a separate > patch. We're using get_nr_dirty_pages() in existing locations where > we have the 'wb', like in wb_check_old_data_flush(). Good point and fully agreed. So you can add: Reviewed-by: Jan Kara <jack@suse.cz> for this patch. Honza
On 09/20/2017 09:36 AM, Jan Kara wrote: > On Wed 20-09-17 09:05:51, Jens Axboe wrote: >> On 09/20/2017 08:41 AM, Jan Kara wrote: >>> On Tue 19-09-17 13:53:06, Jens Axboe wrote: >>>> Now that we have no external callers of wb_start_writeback(), >>>> we can move the nr_pages == 0 logic into that function. >>>> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >>> >>> ... >>> >>>> +static unsigned long get_nr_dirty_pages(void) >>>> +{ >>>> + return global_node_page_state(NR_FILE_DIRTY) + >>>> + global_node_page_state(NR_UNSTABLE_NFS) + >>>> + get_nr_dirty_inodes(); >>>> +} >>>> + >>>> static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, >>>> bool range_cyclic, enum wb_reason reason) >>>> { >>>> @@ -942,6 +953,12 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, >>>> return; >>>> >>>> /* >>>> + * If someone asked for zero pages, we write out the WORLD >>>> + */ >>>> + if (!nr_pages) >>>> + nr_pages = get_nr_dirty_pages(); >>>> + >>> >>> So for 'wb' we have a better estimate of the amount we should write - use >>> wb_stat_sum(wb, WB_RECLAIMABLE) statistics - that is essentially dirty + >>> unstable_nfs broken down to bdi_writeback. >> >> I don't mind making that change, but I think that should be a separate >> patch. We're using get_nr_dirty_pages() in existing locations where >> we have the 'wb', like in wb_check_old_data_flush(). > > Good point and fully agreed. So you can add: > > Reviewed-by: Jan Kara <jack@suse.cz> Thanks Jan, added. I just sent out the new version, mainly because the removal or 'nr_pages' changes the later patches a bit. All for the better, in my opinion.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 7564347914f8..a9a86644cb9f 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -933,6 +933,17 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, #endif /* CONFIG_CGROUP_WRITEBACK */ +/* + * Add in the number of potentially dirty inodes, because each inode + * write can dirty pagecache in the underlying blockdev. + */ +static unsigned long get_nr_dirty_pages(void) +{ + return global_node_page_state(NR_FILE_DIRTY) + + global_node_page_state(NR_UNSTABLE_NFS) + + get_nr_dirty_inodes(); +} + static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, bool range_cyclic, enum wb_reason reason) { @@ -942,6 +953,12 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, return; /* + * If someone asked for zero pages, we write out the WORLD + */ + if (!nr_pages) + nr_pages = get_nr_dirty_pages(); + + /* * This is WB_SYNC_NONE writeback, so if allocation fails just * wakeup the thread for old dirty data writeback */ @@ -1814,17 +1831,6 @@ static struct wb_writeback_work *get_next_work_item(struct bdi_writeback *wb) return work; } -/* - * Add in the number of potentially dirty inodes, because each inode - * write can dirty pagecache in the underlying blockdev. - */ -static unsigned long get_nr_dirty_pages(void) -{ - return global_node_page_state(NR_FILE_DIRTY) + - global_node_page_state(NR_UNSTABLE_NFS) + - get_nr_dirty_inodes(); -} - static long wb_check_background_flush(struct bdi_writeback *wb) { if (wb_over_bg_thresh(wb)) { @@ -1966,9 +1972,6 @@ static void __wakeup_flusher_threads_bdi(struct backing_dev_info *bdi, void wakeup_flusher_threads_bdi(struct backing_dev_info *bdi, long nr_pages, enum wb_reason reason) { - if (!nr_pages) - nr_pages = get_nr_dirty_pages(); - rcu_read_lock(); __wakeup_flusher_threads_bdi(bdi, nr_pages, reason); rcu_read_unlock(); @@ -1988,9 +1991,6 @@ void wakeup_flusher_threads(long nr_pages, enum wb_reason reason) if (blk_needs_flush_plug(current)) blk_schedule_flush_plug(current); - if (!nr_pages) - nr_pages = get_nr_dirty_pages(); - rcu_read_lock(); list_for_each_entry_rcu(bdi, &bdi_list, bdi_list) __wakeup_flusher_threads_bdi(bdi, nr_pages, reason);
Now that we have no external callers of wb_start_writeback(), we can move the nr_pages == 0 logic into that function. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/fs-writeback.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)