More graceful flusher thread memory reclaim wakeup
Subsystems (6)
v2 cover More graceful flusher thread memory reclaim wakeup
We've had some issues with writeback in presence of memory reclaim at Facebook, and this patch set attempts to fix it up. The real functional change is the last patch in the series, the first 5 are prep and cleanup patches.
The basic idea is that we have callers that call wakeup_flusher_threads() with nr_pages == 0. This means 'writeback everything'. For memory reclaim situations, we can end up queuing a TON of these kinds of writeback units. This can cause softlockups and further memory issues, since we allocate huge amounts of struct wb_writeback_work to handle this writeback. Handle this situation more gracefully.
Changes since v1:
- Rename WB_zero_pages to WB_start_all (Amir).
- Remove a test_bit() for a condition where we always expect the bit to be set.
- Remove 'nr_pages' from the wakeup flusher threads helpers, since everybody now passes in zero. Enables further cleanups in later patches too (Jan).
- Fix a case where I forgot to clear WB_start_all if 'work' allocation failed.
- Get rid of cond_resched() in the wb_do_writeback() loop.
Signature · 8 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 1/7 buffer: cleanup free_more_memory() flusher wakeup
This whole function is... interesting. Change the wakeup call to the flusher threads to pass in nr_pages == 0, instead of some random number of pages. This matches more closely what similar cases do for memory shortage/reclaim.
--- fs/buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/buffer.c +1 -1
@@ -260,7 +260,7 @@ static void free_more_memory(void)struct zoneref *z;int nid; -wakeup_flusher_threads (1024 ,WB_REASON_FREE_MORE_MEM ); +wakeup_flusher_threads (0 ,WB_REASON_FREE_MORE_MEM );yield ();for_each_online_node (nid) {
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/7] buffer: cleanup free_more_memory() flusher wakeup
Looks fine,
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 2/7 fs: kill 'nr_pages' argument from wakeup_flusher_threads()
In reply to Jens AxboeEverybody is passing in 0 now, let's get rid of the argument.
--- fs/buffer.c | 2 +- fs/fs-writeback.c | 9 ++++----- fs/sync.c | 2 +- include/linux/writeback.h | 2 +- mm/vmscan.c | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-)
fs/buffer.c +1 -1
@@ -260,7 +260,7 @@ static void free_more_memory(void)struct zoneref *z;int nid; -wakeup_flusher_threads (0 ,WB_REASON_FREE_MORE_MEM ); +wakeup_flusher_threads (WB_REASON_FREE_MORE_MEM );yield ();for_each_online_node (nid) {
fs/fs-writeback.c +4 -5
@@ -1947,12 +1947,12 @@ void wb_workfn(struct work_struct *work) }/* -* Start writeback of `nr_pages' pages. If `nr_pages' is zero, write back -* the whole world. +* Wakeup the flusher threads to start writeback of all currently dirty pages */ -void wakeup_flusher_threads (long nr_pages,enum wb_reason reason) +void wakeup_flusher_threads (enum wb_reason reason) {struct backing_dev_info *bdi; +long nr_pages;/* * If we are expecting writeback progress we must submit plugged IO.
@@ -1960,8 +1960,7 @@ 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 (); + nr_pages =get_nr_dirty_pages ();rcu_read_lock ();list_for_each_entry_rcu (bdi, &bdi_list, bdi_list) {
fs/sync.c +1 -1
@@ -108,7 +108,7 @@ SYSCALL_DEFINE0(sync) {int nowait =0 , wait =1 ; -wakeup_flusher_threads (0 ,WB_REASON_SYNC ); +wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL );iterate_supers (sync_fs_one_sb, &nowait);iterate_supers (sync_fs_one_sb, &wait);
include/linux/writeback.h +1 -1
@@ -189,7 +189,7 @@ bool try_to_writeback_inodes_sb(struct super_block *, enum wb_reason reason);bool try_to_writeback_inodes_sb_nr (struct super_block *,unsigned long nr,enum wb_reason reason);void sync_inodes_sb (struct super_block *); -void wakeup_flusher_threads (long nr_pages,enum wb_reason reason); +void wakeup_flusher_threads (enum wb_reason reason);void inode_wait_for_writeback (struct inode *inode);/* writeback.h requires fs.h; it, too, is not included from here. */
mm/vmscan.c +1 -1
@@ -1867,7 +1867,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,* also allow kswapd to start writing pages during reclaim. */ if (stat.nr_unqueued_dirty == nr_taken) { -wakeup_flusher_threads (0 ,WB_REASON_VMSCAN ); +wakeup_flusher_threads (WB_REASON_VMSCAN );set_bit (PGDAT_DIRTY , &pgdat->flags); }
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/7] fs: kill 'nr_pages' argument from wakeup_flusher_threads()
Looks fine,
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/7] fs: kill 'nr_pages' argument from wakeup_flusher_threads()
In reply to Jens AxboeQuoted 3 lines · Jens Axboe
Everybody is passing in 0 now, let's get rid of the argument.
Signed-off-by Jens Axboe <[email protected]>
Looks good. You can add:
Honza
Quoted 93 lines
--- fs/buffer.c | 2 +- fs/fs-writeback.c | 9 ++++----- fs/sync.c | 2 +- include/linux/writeback.h | 2 +- mm/vmscan.c | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-)fs/buffer.c +1
-1@@ -260,7 +260,7 @@ static void free_more_memory(void)struct zoneref *z;int nid; -wakeup_flusher_threads (0 ,WB_REASON_FREE_MORE_MEM ); +wakeup_flusher_threads (WB_REASON_FREE_MORE_MEM );yield ();for_each_online_node (nid) {fs/fs-writeback.c +4
-5@@ -1947,12 +1947,12 @@ void wb_workfn(struct work_struct *work) }/* -* Start writeback of `nr_pages' pages. If `nr_pages' is zero, write back -* the whole world. +* Wakeup the flusher threads to start writeback of all currently dirty pages */ -void wakeup_flusher_threads (long nr_pages,enum wb_reason reason) +void wakeup_flusher_threads (enum wb_reason reason) {struct backing_dev_info *bdi; +long nr_pages;/* * If we are expecting writeback progress we must submit plugged IO. @@ -1960,8 +1960,7 @@ 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 (); + nr_pages =get_nr_dirty_pages ();rcu_read_lock ();list_for_each_entry_rcu (bdi, &bdi_list, bdi_list) {fs/sync.c +1
-1@@ -108,7 +108,7 @@ SYSCALL_DEFINE0(sync) {int nowait =0 , wait =1 ; -wakeup_flusher_threads (0 ,WB_REASON_SYNC ); +wakeup_flusher_threads (WB_REASON_SYNC );iterate_supers (sync_inodes_one_sb,NULL );iterate_supers (sync_fs_one_sb, &nowait);iterate_supers (sync_fs_one_sb, &wait);include/linux/writeback.h +1
-1@@ -189,7 +189,7 @@ bool try_to_writeback_inodes_sb(struct super_block *, enum wb_reason reason);bool try_to_writeback_inodes_sb_nr (struct super_block *,unsigned long nr,enum wb_reason reason);void sync_inodes_sb (struct super_block *); -void wakeup_flusher_threads (long nr_pages,enum wb_reason reason); +void wakeup_flusher_threads (enum wb_reason reason);void inode_wait_for_writeback (struct inode *inode);/* writeback.h requires fs.h; it, too, is not included from here. */ mm/vmscan.c +1
-1@@ -1867,7 +1867,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,* also allow kswapd to start writing pages during reclaim. */ if (stat.nr_unqueued_dirty == nr_taken) { -wakeup_flusher_threads (0 ,WB_REASON_VMSCAN ); +wakeup_flusher_threads (WB_REASON_VMSCAN );set_bit (PGDAT_DIRTY , &pgdat->flags); }Signature · 1 line
2.7.4
Signature · 8 lines
Jan Kara <[email protected]> SUSE Labs, CR -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 3/7 fs-writeback: provide a wakeup_flusher_threads_bdi()
In reply to Jens AxboeSimilar to wakeup_flusher_threads(), except that we only wake up the flusher threads on the specified backing device.
No functional changes in this patch.
--- fs/fs-writeback.c | 39 +++++++++++++++++++++++++++++---------- include/linux/writeback.h | 2 ++ 2 files changed, 31 insertions(+), 10 deletions(-)
fs/fs-writeback.c +29 -10
@@ -1947,6 +1947,33 @@ void wb_workfn(struct work_struct *work) }/* +* Start writeback of `nr_pages' pages on this bdi. If `nr_pages' is zero, +* write back the whole world. +*/ +static void __wakeup_flusher_threads_bdi (struct backing_dev_info *bdi, +long nr_pages,enum wb_reason reason) +{ +struct bdi_writeback *wb; + +if (!bdi_has_dirty_io (bdi)) +return ; + +list_for_each_entry_rcu (wb, &bdi->wb_list, bdi_node) +wb_start_writeback (wb,wb_split_bdi_pages (wb, nr_pages), + false, reason); +} + +void wakeup_flusher_threads_bdi (struct backing_dev_info *bdi, +enum wb_reason reason) +{ +long nr_pages =get_nr_dirty_pages (); + +rcu_read_lock (); +__wakeup_flusher_threads_bdi (bdi, nr_pages, reason); +rcu_read_unlock (); +} + +/* * Wakeup the flusher threads to start writeback of all currently dirty pages */ void wakeup_flusher_threads (enum wb_reason reason)
@@ -1963,16 +1990,8 @@ void wakeup_flusher_threads(enum wb_reason reason) nr_pages =get_nr_dirty_pages ();rcu_read_lock (); -list_for_each_entry_rcu (bdi, &bdi_list, bdi_list) { -struct bdi_writeback *wb; - -if (!bdi_has_dirty_io (bdi)) -continue ; - -list_for_each_entry_rcu (wb, &bdi->wb_list, bdi_node) -wb_start_writeback (wb,wb_split_bdi_pages (wb, nr_pages), - false, reason); - } +list_for_each_entry_rcu (bdi, &bdi_list, bdi_list) +__wakeup_flusher_threads_bdi (bdi, nr_pages, reason);rcu_read_unlock (); }
include/linux/writeback.h +2 -0
@@ -190,6 +190,8 @@ bool try_to_writeback_inodes_sb_nr(struct super_block *, unsigned long nr, enum wb_reason reason);void sync_inodes_sb (struct super_block *);void wakeup_flusher_threads (enum wb_reason reason); +void wakeup_flusher_threads_bdi (struct backing_dev_info *bdi, +enum wb_reason reason);void inode_wait_for_writeback (struct inode *inode);/* writeback.h requires fs.h; it, too, is not included from here. */
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 3/7] fs-writeback: provide a wakeup_flusher_threads_bdi()
Looks fine,
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 4/7 page-writeback: pass in '0' for nr_pages writeback in laptop mode
In reply to Jens AxboeLaptop mode really wants to writeback the number of dirty pages and inodes. Instead of calculating this in the caller, just pass in 0 and let wakeup_flusher_threads() handle it.
Use the new wakeup_flusher_threads_bdi() instead of rolling our own. This changes the writeback to not be range cyclic, but that should not matter for laptop mode flush-all semantics.
--- mm/page-writeback.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-)
mm/page-writeback.c +1 -16
@@ -1980,23 +1980,8 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,void laptop_mode_timer_fn (unsigned long data) {struct request_queue *q = (struct request_queue *)data; -int nr_pages =global_node_page_state (NR_FILE_DIRTY ) + -global_node_page_state (NR_UNSTABLE_NFS ); -struct bdi_writeback *wb; -/* -* We want to write everything out, not just down to the dirty -* threshold -*/ -if (!bdi_has_dirty_io (q->backing_dev_info)) -return ; - -rcu_read_lock (); -list_for_each_entry_rcu (wb, &q->backing_dev_info->wb_list, bdi_node) -if (wb_has_dirty_io (wb)) -wb_start_writeback (wb, nr_pages, true, -WB_REASON_LAPTOP_TIMER ); -rcu_read_unlock (); +wakeup_flusher_threads_bdi (q->backing_dev_info,WB_REASON_LAPTOP_TIMER ); }/*
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 4/7] page-writeback: pass in '0' for nr_pages writeback in laptop mode
Quoted 8 lines · Jens Axboe
Laptop mode really wants to writeback the number of dirty pages and inodes. Instead of calculating this in the caller, just pass in 0 and let
wakeup_flusher_threads()handle it.Use the new
wakeup_flusher_threads_bdi()instead of rolling our own. This changes the writeback to not be range cyclic, but that should not matter for laptop mode flush-all semantics.
Looks good,
While we're at sorting out the laptop_mode_wb_timer mess:
can we move initializing and deleting it from the block code to the backing-dev code given that it now doesn't assume anything about block devices any more?
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 4/7] page-writeback: pass in '0' for nr_pages writeback in laptop mode
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · Jens Axboe
Laptop mode really wants to writeback the number of dirty pages and inodes. Instead of calculating this in the caller, just pass in 0 and let
wakeup_flusher_threads()handle it.Use the new
wakeup_flusher_threads_bdi()instead of rolling our own. This changes the writeback to not be range cyclic, but that should not matter for laptop mode flush-all semantics.
Quoted 8 lines
Looks good,
Reviewed-by Christoph Hellwig <[email protected]>While we're at sorting out the
laptop_mode_wb_timermess:
can we move initializing and deleting it from the block code to the backing-dev code given that it now doesn't assume anything about block devices any more?
Good point, I'll include that in a followup for 4.15.
Signature · 7 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 4/7] page-writeback: pass in '0' for nr_pages writeback in laptop mode
In reply to Jens AxboeQuoted 4 lines · Jens Axboe
Use the new
wakeup_flusher_threads_bdi()instead of rolling our own. This changes the writeback to not be range cyclic, but that should not matter for laptop mode flush-all semantics.
Oh btw - I think this actually is the more important change and should probably be in the subject line.
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Jan Kara
maintainerRe: [PATCH 4/7] page-writeback: pass in '0' for nr_pages writeback in laptop mode
In reply to Jens AxboeQuoted 9 lines · Jens Axboe
Laptop mode really wants to writeback the number of dirty pages and inodes. Instead of calculating this in the caller, just pass in 0 and let
wakeup_flusher_threads()handle it.Use the new
wakeup_flusher_threads_bdi()instead of rolling our own. This changes the writeback to not be range cyclic, but that should not matter for laptop mode flush-all semantics.
Quoted 5 lines
Acked-by Johannes Weiner <[email protected]>Tested-by Chris Mason <[email protected]>Signed-off-by Jens Axboe <[email protected]>Reviewed-by Jan Kara <[email protected]>Signed-off-by Jens Axboe <[email protected]>
The subject of the patch looks stale.
Honza
Signature · 8 lines
Jan Kara <[email protected]> SUSE Labs, CR -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 5/7 fs-writeback: make wb_start_writeback() static
In reply to Jens AxboeWe don't have any callers outside of fs-writeback.c anymore, make it private.
--- fs/fs-writeback.c | 4 ++-- include/linux/backing-dev.h | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-)
fs/fs-writeback.c +2 -2
@@ -933,8 +933,8 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,#endif /* CONFIG_CGROUP_WRITEBACK */ -void wb_start_writeback (struct bdi_writeback *wb,long nr_pages, -bool range_cyclic,enum wb_reason reason) +static void wb_start_writeback (struct bdi_writeback *wb,long nr_pages, +bool range_cyclic,enum wb_reason reason) {struct wb_writeback_work *work;
include/linux/backing-dev.h +0 -2
@@ -38,8 +38,6 @@ static inline struct backing_dev_info *bdi_alloc(gfp_t gfp_mask)return bdi_alloc_node (gfp_mask,NUMA_NO_NODE ); } -void wb_start_writeback (struct bdi_writeback *wb,long nr_pages, -bool range_cyclic,enum wb_reason reason);void wb_start_background_writeback (struct bdi_writeback *wb);void wb_workfn (struct work_struct *work);void wb_wakeup_delayed (struct bdi_writeback *wb);
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 5/7] fs-writeback: make wb_start_writeback() static
Looks fine,
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 6/7 fs-writeback: move nr_pages == 0 logic to one location
In reply to Jens AxboeNow that we have no external callers of wb_start_writeback(), we can shuffle the passing in of 'nr_pages'. Everybody passes in 0 at this point, so just kill the argument and move the dirty count retrieval to that function.
--- fs/fs-writeback.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)
fs/fs-writeback.c +18 -24
@@ -933,8 +933,19 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,#endif /* CONFIG_CGROUP_WRITEBACK */ -static void wb_start_writeback (struct bdi_writeback *wb,long nr_pages, -bool range_cyclic,enum wb_reason reason) +/* +* 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,bool range_cyclic, +enum wb_reason reason) {struct wb_writeback_work *work;
@@ -954,7 +965,7 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, } work->sync_mode =WB_SYNC_NONE ; - work->nr_pages = nr_pages; + work->nr_pages =wb_split_bdi_pages (wb,get_nr_dirty_pages ()); work->range_cyclic = range_cyclic; work->reason = reason; work->auto_free =1 ;
@@ -1814,17 +1825,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)) {
@@ -1951,7 +1951,7 @@ void wb_workfn(struct work_struct *work)* write back the whole world. */ static void __wakeup_flusher_threads_bdi (struct backing_dev_info *bdi, -long nr_pages,enum wb_reason reason) +enum wb_reason reason) {struct bdi_writeback *wb;
@@ -1959,17 +1959,14 @@ static void __wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,return ;list_for_each_entry_rcu (wb, &bdi->wb_list, bdi_node) -wb_start_writeback (wb,wb_split_bdi_pages (wb, nr_pages), - false, reason); +wb_start_writeback (wb, false, reason); }void wakeup_flusher_threads_bdi (struct backing_dev_info *bdi,enum wb_reason reason) { -long nr_pages =get_nr_dirty_pages (); -rcu_read_lock (); -__wakeup_flusher_threads_bdi (bdi, nr_pages, reason); +__wakeup_flusher_threads_bdi (bdi, reason);rcu_read_unlock (); }
@@ -1979,7 +1976,6 @@ void wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,void wakeup_flusher_threads (enum wb_reason reason) {struct backing_dev_info *bdi; -long nr_pages;/* * If we are expecting writeback progress we must submit plugged IO.
@@ -1987,11 +1983,9 @@ void wakeup_flusher_threads(enum wb_reason reason)if (blk_needs_flush_plug (current))blk_schedule_flush_plug (current); - 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); +__wakeup_flusher_threads_bdi (bdi, reason);rcu_read_unlock (); }
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 6/7] fs-writeback: move nr_pages == 0 logic to one location
Looks fine,
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 6/7] fs-writeback: move nr_pages == 0 logic to one location
In reply to Jens AxboeQuoted 8 lines · Jens Axboe
Now that we have no external callers of
wb_start_writeback(), we can shuffle the passing in of 'nr_pages'. Everybody passes in 0 at this point, so just kill the argument and move the dirty count retrieval to that function.Acked-by Johannes Weiner <[email protected]>Tested-by Chris Mason <[email protected]>Signed-off-by Jens Axboe <[email protected]>
Looks good. You can add:
Honza
Quoted 110 lines
--- fs/fs-writeback.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)fs/fs-writeback.c +18
-24@@ -933,8 +933,19 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,#endif /* CONFIG_CGROUP_WRITEBACK */ -static void wb_start_writeback (struct bdi_writeback *wb,long nr_pages, -bool range_cyclic,enum wb_reason reason) +/* +* 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,bool range_cyclic, +enum wb_reason reason) {struct wb_writeback_work *work;@@ -954,7 +965,7 @@ static void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, } work->sync_mode =WB_SYNC_NONE ; - work->nr_pages = nr_pages; + work->nr_pages =wb_split_bdi_pages (wb,get_nr_dirty_pages ()); work->range_cyclic = range_cyclic; work->reason = reason; work->auto_free =1 ;@@ -1814,17 +1825,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)) {@@ -1951,7 +1951,7 @@ void wb_workfn(struct work_struct *work)* write back the whole world. */ static void __wakeup_flusher_threads_bdi (struct backing_dev_info *bdi, -long nr_pages,enum wb_reason reason) +enum wb_reason reason) {struct bdi_writeback *wb;@@ -1959,17 +1959,14 @@ static void __wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,return ;list_for_each_entry_rcu (wb, &bdi->wb_list, bdi_node) -wb_start_writeback (wb,wb_split_bdi_pages (wb, nr_pages), - false, reason); +wb_start_writeback (wb, false, reason); }void wakeup_flusher_threads_bdi (struct backing_dev_info *bdi,enum wb_reason reason) { -long nr_pages =get_nr_dirty_pages (); -rcu_read_lock (); -__wakeup_flusher_threads_bdi (bdi, nr_pages, reason); +__wakeup_flusher_threads_bdi (bdi, reason);rcu_read_unlock (); }@@ -1979,7 +1976,6 @@ void wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,void wakeup_flusher_threads (enum wb_reason reason) {struct backing_dev_info *bdi; -long nr_pages;/* * If we are expecting writeback progress we must submit plugged IO. @@ -1987,11 +1983,9 @@ void wakeup_flusher_threads(enum wb_reason reason)if (blk_needs_flush_plug (current))blk_schedule_flush_plug (current); - 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); +__wakeup_flusher_threads_bdi (bdi, reason);rcu_read_unlock (); }Signature · 1 line
2.7.4
Signature · 8 lines
Jan Kara <[email protected]> SUSE Labs, CR -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH v2 7/7 fs-writeback: only allow one inflight and pending full flush
In reply to Jens AxboeWhen someone calls wakeup_flusher_threads() or wakeup_flusher_threads_bdi(), they schedule writeback of all dirty pages in the system (or on that bdi). If we are tight on memory, we can get tons of these queued from kswapd/vmscan. This causes (at least) two problems:
1) We consume a ton of memory just allocating writeback work items.
2) We spend so much time processing these work items, that we introduce a softlockup in writeback processing.
Fix this by adding a 'start_all' bit to the writeback structure, and set that when someone attempts to flush all dirty page. The bit is cleared when we start writeback on that work item. If the bit is already set when we attempt to queue !nr_pages writeback, then we simply ignore it.
This provides us one full flush in flight, with one pending as well, and makes for more efficient handling of this type of writeback.
--- fs/fs-writeback.c | 24 ++++++++++++++++++++++++ include/linux/backing-dev-defs.h | 1 + 2 files changed, 25 insertions(+)
fs/fs-writeback.c +24 -0
@@ -53,6 +53,7 @@ struct wb_writeback_work { unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ unsigned int auto_free:1; /* free on completion */ + unsigned int start_all:1; /* nr_pages == 0 (all) writeback */enum wb_reason reason;/* why was writeback initiated? */ struct list_head list;/* pending work list */
@@ -953,12 +954,26 @@ static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic,return ;/* +* All callers of this function want to start writeback of all +* dirty pages. Places like vmscan can call this at a very +* high frequency, causing pointless allocations of tons of +* work items and keeping the flusher threads busy retrieving +* that work. Ensure that we only allow one of them pending and +* inflight at the time +*/ +if (test_bit (WB_start_all, &wb->state)) +return ; + +set_bit (WB_start_all, &wb->state); + +/* * This is WB_SYNC_NONE writeback, so if allocation fails just * wakeup the thread for old dirty data writeback */ work =kzalloc (sizeof (*work),GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN);if (!work) { +clear_bit (WB_start_all, &wb->state);trace_writeback_nowork (wb);wb_wakeup (wb);return ;
@@ -969,6 +984,7 @@ static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic, work->range_cyclic = range_cyclic; work->reason = reason; work->auto_free =1 ; + work->start_all =1 ;wb_queue_work (wb, work); }
@@ -1822,6 +1838,14 @@ static struct wb_writeback_work *get_next_work_item(struct bdi_writeback *wb)list_del_init (&work->list); }spin_unlock_bh (&wb->work_lock); + +/* +* Once we start processing a work item that had !nr_pages, +* clear the wb state bit for that so we can allow more. +*/ +if (work && work->start_all) +clear_bit (WB_start_all, &wb->state); +return work; }
include/linux/backing-dev-defs.h +1 -0
@@ -24,6 +24,7 @@ enum wb_state { WB_shutting_down,/* wb_shutdown() in progress */ WB_writeback_running,/* Writeback is in progress */ WB_has_dirty_io,/* Dirty inodes on ->b_{dirty|io|more_io} */ + WB_start_all, /* nr_pages == 0 (all) work pending */ }; enum wb_congested_state {
Signature · 7 lines
2.7.4 -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 45 lines · Jens Axboe
When someone calls
wakeup_flusher_threads()orwakeup_flusher_threads_bdi(), they schedule writeback of all dirty pages in the system (or on that bdi). If we are tight on memory, we can get tons of these queued from kswapd/vmscan. This causes (at least) two problems:1) We consume a ton of memory just allocating writeback work items.
2) We spend so much time processing these work items, that we introduce a softlockup in writeback processing.Fix this by adding a '
start_all' bit to the writeback structure, and set that when someone attempts to flush all dirty page. The bit is cleared when we start writeback on that work item. If the bit is already set when we attempt to queue !nr_pages writeback, then we simply ignore it.This provides us one full flush in flight, with one pending as well, and makes for more efficient handling of this type of writeback.
Acked-by Johannes Weiner <[email protected]>Tested-by Chris Mason <[email protected]>Reviewed-by Jan Kara <[email protected]>Signed-off-by Jens Axboe <[email protected]>--- fs/fs-writeback.c | 24 ++++++++++++++++++++++++ include/linux/backing-dev-defs.h | 1 + 2 files changed, 25 insertions(+)fs/fs-writeback.c +2
-0@@ -53,6 +53,7 @@ struct wb_writeback_work { unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ unsigned int auto_free:1; /* free on completion */ + unsigned int start_all:1; /* nr_pages == 0 (all) writeback */enum wb_reason reason;/* why was writeback initiated? */ struct list_head list;/* pending work list */ @@ -953,12 +954,26 @@ static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic,return ;/* +* All callers of this function want to start writeback of all
fs/fs-writeback.c static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic, · line 967
Quoted 10 lines
fs/fs-writeback.c +10
-0+* dirty pages. Places like vmscan can call this at a very +* high frequency, causing pointless allocations of tons of +* work items and keeping the flusher threads busy retrieving +* that work. Ensure that we only allow one of them pending and +* inflight at the time +*/ +if (test_bit (WB_start_all, &wb->state)) +return ; + +set_bit (WB_start_all, &wb->state);
This should be test_and_set_bit here..
But more importantly once we are not guaranteed that we only have a single global wb_writeback_work per bdi_writeback we should just embedd that into struct bdi_writeback instead of dynamically allocating it.
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 44 lines · Christoph Hellwig
Quoted 43 lines · Jens Axboe
When someone calls
wakeup_flusher_threads()orwakeup_flusher_threads_bdi(), they schedule writeback of all dirty pages in the system (or on that bdi). If we are tight on memory, we can get tons of these queued from kswapd/vmscan. This causes (at least) two problems:1) We consume a ton of memory just allocating writeback work items.
2) We spend so much time processing these work items, that we introduce a softlockup in writeback processing.Fix this by adding a '
start_all' bit to the writeback structure, and set that when someone attempts to flush all dirty page. The bit is cleared when we start writeback on that work item. If the bit is already set when we attempt to queue !nr_pages writeback, then we simply ignore it.This provides us one full flush in flight, with one pending as well, and makes for more efficient handling of this type of writeback.
Acked-by Johannes Weiner <[email protected]>Tested-by Chris Mason <[email protected]>Reviewed-by Jan Kara <[email protected]>Signed-off-by Jens Axboe <[email protected]>--- fs/fs-writeback.c | 24 ++++++++++++++++++++++++ include/linux/backing-dev-defs.h | 1 + 2 files changed, 25 insertions(+)fs/fs-writeback.c +1
-0@@ -53,6 +53,7 @@ struct wb_writeback_work { unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ unsigned int auto_free:1; /* free on completion */ + unsigned int start_all:1; /* nr_pages == 0 (all) writeback */enum wb_reason reason;/* why was writeback initiated? */ struct list_head list;/* pending work list */ @@ -953,12 +954,26 @@ static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic,return ;
Quoted 14 lines
fs/fs-writeback.c static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic, · line 967
Quoted 12 lines
fs/fs-writeback.c +11
-0/* +* All callers of this function want to start writeback of all +* dirty pages. Places like vmscan can call this at a very +* high frequency, causing pointless allocations of tons of +* work items and keeping the flusher threads busy retrieving +* that work. Ensure that we only allow one of them pending and +* inflight at the time +*/ +if (test_bit (WB_start_all, &wb->state)) +return ; + +set_bit (WB_start_all, &wb->state);This should be
test_and_set_bithere..
That's on purpose, doesn't matter if we race here, and if we're being hammered with flusher thread wakeups, then we don't want to turn that unlocked test into a locked instruction.
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Signature · 7 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 7 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the bdi_writeback structure.
fs/fs-writeback.c +9 -50
@@ -40,27 +40,6 @@ struct wb_completion { };/* -* Passed into wb_writeback(), essentially a subset of writeback_control -*/ -struct wb_writeback_work { -long nr_pages; -struct super_block *sb; -unsigned long *older_than_this; -enum writeback_sync_modes sync_mode; -unsigned int tagged_writepages:1 ; -unsigned int for_kupdate:1 ; -unsigned int range_cyclic:1 ; -unsigned int for_background:1 ; -unsigned int for_sync:1 ;/* sync(2) WB_SYNC_ALL writeback */ -unsigned int auto_free:1 ;/* free on completion */ -unsigned int start_all:1 ;/* nr_pages == 0 (all) writeback */ -enum wb_reason reason;/* why was writeback initiated? */ - -struct list_head list;/* pending work list */ -struct wb_completion *done;/* set if the caller waits */ -}; - -/* * If one wants to wait for one or more wb_writeback_works, each work's * ->done should be set to a wb_completion defined using the following * macro. Once all work items are issued with wb_queue_work(), the caller
@@ -181,6 +160,8 @@ static void finish_writeback_work(struct bdi_writeback *wb,if (work->auto_free)kfree (work); +else if (work->start_all) +clear_bit (WB_start_all, &wb->state);if (done &&atomic_dec_and_test (&done->cnt))wake_up_all (&wb->bdi->wb_waitq); }
@@ -945,8 +926,7 @@ static unsigned long get_nr_dirty_pages(void)get_nr_dirty_inodes (); } -static void wb_start_writeback (struct bdi_writeback *wb,bool range_cyclic, -enum wb_reason reason) +static void wb_start_writeback (struct bdi_writeback *wb,enum wb_reason reason) {struct wb_writeback_work *work;
@@ -961,29 +941,16 @@ static void wb_start_writeback(struct bdi_writeback *wb, bool range_cyclic,* that work. Ensure that we only allow one of them pending and * inflight at the time */ -if (test_bit (WB_start_all, &wb->state)) -return ; - -set_bit (WB_start_all, &wb->state); - -/* -* This is WB_SYNC_NONE writeback, so if allocation fails just -* wakeup the thread for old dirty data writeback -*/ - work =kzalloc (sizeof (*work), -GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN); -if (!work) { -clear_bit (WB_start_all, &wb->state); -trace_writeback_nowork (wb); -wb_wakeup (wb); +if (test_bit (WB_start_all, &wb->state) || +test_and_set_bit (WB_start_all, &wb->state))return ; - } + work = &wb->wb_all_work; +memset (work,0 ,sizeof (*work)); work->sync_mode =WB_SYNC_NONE ; work->nr_pages =wb_split_bdi_pages (wb,get_nr_dirty_pages ()); - work->range_cyclic = range_cyclic; + work->range_cyclic = false; work->reason = reason; - work->auto_free =1 ; work->start_all =1 ;wb_queue_work (wb, work);
@@ -1838,14 +1805,6 @@ static struct wb_writeback_work *get_next_work_item(struct bdi_writeback *wb)list_del_init (&work->list); }spin_unlock_bh (&wb->work_lock); - -/* -* Once we start processing a work item that had !nr_pages, -* clear the wb state bit for that so we can allow more. -*/ -if (work && work->start_all) -clear_bit (WB_start_all, &wb->state); -return work; }
@@ -1983,7 +1942,7 @@ static void __wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,return ;list_for_each_entry_rcu (wb, &bdi->wb_list, bdi_node) -wb_start_writeback (wb, false, reason); +wb_start_writeback (wb, reason); } void wakeup_flusher_threads_bdi(struct backing_dev_info *bdi,
include/linux/backing-dev-defs.h +55 -1
74 lines not on this page · raw message
include/linux/writeback.h +0 -30
@@ -34,36 +34,6 @@ DECLARE_PER_CPU(int, dirty_throttle_leaks);struct backing_dev_info ;/* -* fs/fs-writeback.c -*/ -enum writeback_sync_modes { -WB_SYNC_NONE ,/* Don't wait on anything */ -WB_SYNC_ALL ,/* Wait on every mapping */ -}; - -/* -* why some writeback work was initiated -*/ -enum wb_reason { -WB_REASON_BACKGROUND , -WB_REASON_VMSCAN , -WB_REASON_SYNC , -WB_REASON_PERIODIC , -WB_REASON_LAPTOP_TIMER , -WB_REASON_FREE_MORE_MEM , -WB_REASON_FS_FREE_SPACE , -/* -* There is no bdi forker thread any more and works are done -* by emergency worker, however, this is TPs userland visible -* and we'll be exposing exactly the same information, -* so it has a mismatch name. -*/ -WB_REASON_FORKER_THREAD , - -WB_REASON_MAX , -}; - -/* * A control structure which tells the writeback code what to do. These are * always on the stack, and hence need no locking. They are always initialised * in a manner such that unspecified fields are set to zero.
Signature · 1 line
Jens Axboe
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 3 lines · Jens Axboe
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.
Something like that. Although if we still kalloc the global wb we wouldn't have to expose all the details in the header.
--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Jan Kara
maintainerRe: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
In reply to Jens AxboeQuoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.
Quoted 6 lines
Quoted 2 lines
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.
Hum, so when we consider stuff like embedded work item, I would somewhat prefer to handle this like we do for for_background and for_kupdate style writeback so that we don't have another special case. For these don't queue any item, we just queue writeback work into the workqueue (via wb_wakeup()). When flusher work gets processed wb_do_writeback() checks (after processing all normal writeback requests) whether conditions for these special writeback styles are met and if yes, it creates on-stack work item and processes it (see wb_check_old_data_flush() and wb_check_background_flush()).
So in this case we would just set some flag in bdi_writeback when memory reclaim needs help and wb_do_writeback() would check for this flag and create and process writeback-all style writeback work. Granted this does not preserve ordering of requests (basically any specific request gets priority over writeback-whole-world request) but memory gets cleaned in either case so flusher should be doing what is needed.
Honza
Signature · 8 lines
Jan Kara <[email protected]> SUSE Labs, CR -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 14 lines · Jan Kara
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.Quoted 6 lines
Quoted 2 lines
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.
Quoted 9 lines
Hum, so when we consider stuff like embedded work item, I would somewhat prefer to handle this like we do for
for_backgroundandfor_kupdatestyle writeback so that we don't have another special case. For these don't queue any item, we just queue writeback work into the workqueue (viawb_wakeup()). When flusher work gets processedwb_do_writeback()checks (after processing all normal writeback requests) whether conditions for these special writeback styles are met and if yes, it creates on-stack work item and processes it (seewb_check_old_data_flush()andwb_check_background_flush()).
Thanks Jan, I think that's a really good suggestion and kills the special case completely. I'll rework the patch as a small series for 4.15.
Quoted 6 lines
So in this case we would just set some flag in
bdi_writebackwhen memory reclaim needs help andwb_do_writeback()would check for this flag and create and process writeback-all style writeback work. Granted this does not preserve ordering of requests (basically any specific request gets priority over writeback-whole-world request) but memory gets cleaned in either case so flusher should be doing what is needed.
I don't think that matters, and we're already mostly there since we reject a request if one is pending. And at this point they are all identical "start all writeback" requests.
Signature · 7 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
In reply to Jan KaraQuoted 24 lines · Jan Kara
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.Quoted 6 lines
Quoted 2 lines
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.Hum, so when we consider stuff like embedded work item, I would somewhat prefer to handle this like we do for
for_backgroundandfor_kupdatestyle writeback so that we don't have another special case. For these don't queue any item, we just queue writeback work into the workqueue (viawb_wakeup()). When flusher work gets processedwb_do_writeback()checks (after processing all normal writeback requests) whether conditions for these special writeback styles are met and if yes, it creates on-stack work item and processes it (seewb_check_old_data_flush()andwb_check_background_flush()).
Quoted 6 lines
So in this case we would just set some flag in
bdi_writebackwhen memory reclaim needs help andwb_do_writeback()would check for this flag and create and process writeback-all style writeback work. Granted this does not preserve ordering of requests (basically any specific request gets priority over writeback-whole-world request) but memory gets cleaned in either case so flusher should be doing what is needed.
How about something like the below? It's on top of the latest series, which is in my wb-start-all branch. It handles start_all like the background/kupdate style writeback, reusing the WB_start_all bit for that.
On a plane, so untested, but it seems pretty straight forward. It changes the logic a little bit, as the WB_start_all bit isn't cleared until after we're done with a flush-all request. At this point it's truly on inflight at any point in time, not one inflight and one potentially queued.
fs/fs-writeback.c +34 -37
@@ -53,7 +53,6 @@ struct wb_writeback_work { unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ unsigned int auto_free:1; /* free on completion */ - unsigned int start_all:1; /* nr_pages == 0 (all) writeback */enum wb_reason reason;/* why was writeback initiated? */ struct list_head list;/* pending work list */
@@ -947,8 +946,6 @@ static unsigned long get_nr_dirty_pages(void)static void wb_start_writeback (struct bdi_writeback *wb,enum wb_reason reason) { -struct wb_writeback_work *work; -if (!wb_has_dirty_io (wb))return ;
@@ -958,35 +955,14 @@ static void wb_start_writeback(struct bdi_writeback *wb, enum wb_reason reason)* high frequency, causing pointless allocations of tons of * work items and keeping the flusher threads busy retrieving * that work. Ensure that we only allow one of them pending and -* inflight at the time. It doesn't matter if we race a little -* bit on this, so use the faster separate test/set bit variants. +* inflight at the time. */ -if (test_bit (WB_start_all, &wb->state)) +if (test_bit (WB_start_all, &wb->state) || +test_and_set_bit (WB_start_all, &wb->state))return ; -set_bit (WB_start_all, &wb->state); - -/* -* This is WB_SYNC_NONE writeback, so if allocation fails just -* wakeup the thread for old dirty data writeback -*/ - work =kzalloc (sizeof (*work), -GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN); -if (!work) { -clear_bit (WB_start_all, &wb->state); -trace_writeback_nowork (wb); -wb_wakeup (wb); -return ; - } - - work->sync_mode =WB_SYNC_NONE ; - work->nr_pages =wb_split_bdi_pages (wb,get_nr_dirty_pages ()); - work->range_cyclic =1 ; - work->reason = reason; - work->auto_free =1 ; - work->start_all =1 ; - -wb_queue_work (wb, work); + wb->start_all_reason = reason; +wb_wakeup (wb); }/**
@@ -1838,14 +1814,6 @@ static struct wb_writeback_work *get_next_work_item(struct bdi_writeback *wb)list_del_init (&work->list); }spin_unlock_bh (&wb->work_lock); - -/* -* Once we start processing a work item that had !nr_pages, -* clear the wb state bit for that so we can allow more. -*/ -if (work && work->start_all) -clear_bit (WB_start_all, &wb->state); -return work; }
@@ -1901,6 +1869,30 @@ static long wb_check_old_data_flush(struct bdi_writeback *wb)return 0 ; } +static long wb_check_start_all (struct bdi_writeback *wb) +{ +long nr_pages; + +if (!test_bit (WB_start_all, &wb->state)) +return 0 ; + + nr_pages =get_nr_dirty_pages (); +if (nr_pages) { +struct wb_writeback_work work = { + .nr_pages =wb_split_bdi_pages (wb, nr_pages), + .sync_mode =WB_SYNC_NONE , + .range_cyclic =1 , + .reason = wb->start_all_reason, + }; + + nr_pages =wb_writeback (wb, &work); + } + +clear_bit (WB_start_all, &wb->state); +return nr_pages; +} + +/* * Retrieve work items and do the writeback they describe */
@@ -1917,6 +1909,11 @@ static long wb_do_writeback(struct bdi_writeback *wb) }/* +* Check for a flush-everything request +*/ + wrote +=wb_check_start_all (wb); + +/* * Check for periodic writeback, kupdated() style */ wrote +=wb_check_old_data_flush (wb);
include/linux/backing-dev-defs.h +1 -0
@@ -116,6 +116,7 @@ struct bdi_writeback {struct fprop_local_percpu completions;int dirty_exceeded; +int start_all_reason;spinlock_t work_lock;/* protects work_list & dwork scheduling */ struct list_head work_list;
include/trace/events/writeback.h +0 -1
@@ -286,7 +286,6 @@ DEFINE_EVENT(writeback_class, name, \ TP_PROTO(struct bdi_writeback *wb), \ TP_ARGS(wb)) -DEFINE_WRITEBACK_EVENT(writeback_nowork); DEFINE_WRITEBACK_EVENT(writeback_wake_background); TRACE_EVENT(writeback_bdi_register,
Signature · 7 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
Quoted 37 lines · Jens Axboe
Quoted 24 lines · Jan Kara
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.Quoted 6 lines
Quoted 2 lines
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.Hum, so when we consider stuff like embedded work item, I would somewhat prefer to handle this like we do for
for_backgroundandfor_kupdatestyle writeback so that we don't have another special case. For these don't queue any item, we just queue writeback work into the workqueue (viawb_wakeup()). When flusher work gets processedwb_do_writeback()checks (after processing all normal writeback requests) whether conditions for these special writeback styles are met and if yes, it creates on-stack work item and processes it (seewb_check_old_data_flush()andwb_check_background_flush()).Quoted 6 lines
So in this case we would just set some flag in
bdi_writebackwhen memory reclaim needs help andwb_do_writeback()would check for this flag and create and process writeback-all style writeback work. Granted this does not preserve ordering of requests (basically any specific request gets priority over writeback-whole-world request) but memory gets cleaned in either case so flusher should be doing what is needed.How about something like the below? It's on top of the latest series, which is in my wb-start-all branch. It handles
start_alllike the background/kupdate style writeback, reusing theWB_start_allbit for that.
Quoted 5 lines
On a plane, so untested, but it seems pretty straight forward. It changes the logic a little bit, as the
WB_start_allbit isn't cleared until after we're done with a flush-all request. At this point it's truly on inflight at any point in time, not one inflight and one potentially queued.
I tested it, with adding a patch that actually enables laptop completion triggers on blk-mq (not there before, an oversight, will send that out separately). It works fine for me, verified with tracing that we do trigger flushes with completions from laptop mode.
Signature · 7 lines
Jens Axboe -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Jan Kara
maintainerRe: [PATCH 7/7] fs-writeback: only allow one inflight and pending full flush
In reply to Jens AxboeQuoted 37 lines · Jens Axboe
Quoted 24 lines · Jan Kara
Quoted 6 lines · Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines
But more importantly once we are not guaranteed that we only have a single global
wb_writeback_workperbdi_writebackwe should just embedd that into structbdi_writebackinstead of dynamically allocating it.Quoted 6 lines
Quoted 2 lines
We could do this as a followup. But right now the logic is that we can have on started (inflight), and still have one new queued.
Something like the below would fit on top to do that. Gets rid of the allocation and embeds the work item for global start-all in the
bdi_writebackstructure.Hum, so when we consider stuff like embedded work item, I would somewhat prefer to handle this like we do for
for_backgroundandfor_kupdatestyle writeback so that we don't have another special case. For these don't queue any item, we just queue writeback work into the workqueue (viawb_wakeup()). When flusher work gets processedwb_do_writeback()checks (after processing all normal writeback requests) whether conditions for these special writeback styles are met and if yes, it creates on-stack work item and processes it (seewb_check_old_data_flush()andwb_check_background_flush()).Quoted 6 lines
So in this case we would just set some flag in
bdi_writebackwhen memory reclaim needs help andwb_do_writeback()would check for this flag and create and process writeback-all style writeback work. Granted this does not preserve ordering of requests (basically any specific request gets priority over writeback-whole-world request) but memory gets cleaned in either case so flusher should be doing what is needed.How about something like the below? It's on top of the latest series, which is in my wb-start-all branch. It handles
start_alllike the background/kupdate style writeback, reusing theWB_start_allbit for that.
Quoted 5 lines
On a plane, so untested, but it seems pretty straight forward. It changes the logic a little bit, as the
WB_start_allbit isn't cleared until after we're done with a flush-all request. At this point it's truly on inflight at any point in time, not one inflight and one potentially queued.
Thanks for looking into this! I like the change.
Honza
Quoted 149 lines
fs/fs-writeback.c +34
-37@@ -53,7 +53,6 @@ struct wb_writeback_work { unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ unsigned int auto_free:1; /* free on completion */ - unsigned int start_all:1; /* nr_pages == 0 (all) writeback */enum wb_reason reason;/* why was writeback initiated? */ struct list_head list;/* pending work list */ @@ -947,8 +946,6 @@ static unsigned long get_nr_dirty_pages(void)static void wb_start_writeback (struct bdi_writeback *wb,enum wb_reason reason) { -struct wb_writeback_work *work; -if (!wb_has_dirty_io (wb))return ;@@ -958,35 +955,14 @@ static void wb_start_writeback(struct bdi_writeback *wb, enum wb_reason reason)* high frequency, causing pointless allocations of tons of * work items and keeping the flusher threads busy retrieving * that work. Ensure that we only allow one of them pending and -* inflight at the time. It doesn't matter if we race a little -* bit on this, so use the faster separate test/set bit variants. +* inflight at the time. */ -if (test_bit (WB_start_all, &wb->state)) +if (test_bit (WB_start_all, &wb->state) || +test_and_set_bit (WB_start_all, &wb->state))return ; -set_bit (WB_start_all, &wb->state); - -/* -* This is WB_SYNC_NONE writeback, so if allocation fails just -* wakeup the thread for old dirty data writeback -*/ - work =kzalloc (sizeof (*work), -GFP_NOWAIT | __GFP_NOMEMALLOC | __GFP_NOWARN); -if (!work) { -clear_bit (WB_start_all, &wb->state); -trace_writeback_nowork (wb); -wb_wakeup (wb); -return ; - } - - work->sync_mode =WB_SYNC_NONE ; - work->nr_pages =wb_split_bdi_pages (wb,get_nr_dirty_pages ()); - work->range_cyclic =1 ; - work->reason = reason; - work->auto_free =1 ; - work->start_all =1 ; - -wb_queue_work (wb, work); + wb->start_all_reason = reason; +wb_wakeup (wb); }/** @@ -1838,14 +1814,6 @@ static struct wb_writeback_work *get_next_work_item(struct bdi_writeback *wb)list_del_init (&work->list); }spin_unlock_bh (&wb->work_lock); - -/* -* Once we start processing a work item that had !nr_pages, -* clear the wb state bit for that so we can allow more. -*/ -if (work && work->start_all) -clear_bit (WB_start_all, &wb->state); -return work; }@@ -1901,6 +1869,30 @@ static long wb_check_old_data_flush(struct bdi_writeback *wb)return 0 ; } +static long wb_check_start_all (struct bdi_writeback *wb) +{ +long nr_pages; + +if (!test_bit (WB_start_all, &wb->state)) +return 0 ; + + nr_pages =get_nr_dirty_pages (); +if (nr_pages) { +struct wb_writeback_work work = { + .nr_pages =wb_split_bdi_pages (wb, nr_pages), + .sync_mode =WB_SYNC_NONE , + .range_cyclic =1 , + .reason = wb->start_all_reason, + }; + + nr_pages =wb_writeback (wb, &work); + } + +clear_bit (WB_start_all, &wb->state); +return nr_pages; +} + +/* * Retrieve work items and do the writeback they describe */ @@ -1917,6 +1909,11 @@ static long wb_do_writeback(struct bdi_writeback *wb) }/* +* Check for a flush-everything request +*/ + wrote +=wb_check_start_all (wb); + +/* * Check for periodic writeback, kupdated() style */ wrote +=wb_check_old_data_flush (wb);include/linux/backing-dev-defs.h +1
-0@@ -116,6 +116,7 @@ struct bdi_writeback {struct fprop_local_percpu completions;int dirty_exceeded; +int start_all_reason;spinlock_t work_lock;/* protects work_list & dwork scheduling */ struct list_head work_list;include/trace/events/writeback.h +0
-1@@ -286,7 +286,6 @@ DEFINE_EVENT(writeback_class, name, \ TP_PROTO(struct bdi_writeback *wb), \ TP_ARGS(wb)) -DEFINE_WRITEBACK_EVENT(writeback_nowork); DEFINE_WRITEBACK_EVENT(writeback_wake_background); TRACE_EVENT(writeback_bdi_register,Signature · 2 lines
-- Jens Axboe
Signature · 8 lines
Jan Kara <[email protected]> SUSE Labs, CR -- 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/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>