split inode_wb_list_lock into bdi_writeback.list_lock
Subsystems (5)
PATCH 1/2 split inode_wb_list_lock into bdi_writeback.list_lock
From: Christoph Hellwig <[email protected]>
Split the global inode_wb_list_lock into a per-bdi_writeback list_lock, as it's currently the most contended lock in the system for metadata heavy workloads. I won't help for single-filesystem workloads for which we'll need the I/O-less balance_dirty_pages, but at least we can dedicate a cpu to spinning on each bdi now for larger systems.
Based on earlier patches from Nick Piggin and Dave Chinner.
It reduces lock contentions to 1/4 in this test case:
10 HDD JBOD, 100 dd on each disk, XFS, 6GB ram.
lock_stat version 0.3
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
class name con-bounces contentions waittime-min waittime-max waittime-total acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
vanilla 2.6.39-rc3:
inode_wb_list_lock: 42590 44433 0.12 147.74 144127.35 252274 886792 0.08 121.34 917211.23
------------------
inode_wb_list_lock 2 [<ffffffff81165da5>] bdev_inode_switch_bdi+0x29/0x85
inode_wb_list_lock 34 [<ffffffff8115bd0b>] inode_wb_list_del+0x22/0x49
inode_wb_list_lock 12893 [<ffffffff8115bb53>] __mark_inode_dirty+0x170/0x1d0
inode_wb_list_lock 10702 [<ffffffff8115afef>] writeback_single_inode+0x16d/0x20a
------------------
inode_wb_list_lock 2 [<ffffffff81165da5>] bdev_inode_switch_bdi+0x29/0x85
inode_wb_list_lock 19 [<ffffffff8115bd0b>] inode_wb_list_del+0x22/0x49
inode_wb_list_lock 5550 [<ffffffff8115bb53>] __mark_inode_dirty+0x170/0x1d0
inode_wb_list_lock 8511 [<ffffffff8115b4ad>] writeback_sb_inodes+0x10f/0x1572.6.39-rc3 + patch:
&(&wb->list_lock)->rlock: 11383 11657 0.14 151.69 40429.51 90825 527918 0.11 145.90 556843.37
------------------------
&(&wb->list_lock)->rlock 10 [<ffffffff8115b189>] inode_wb_list_del+0x5f/0x86
&(&wb->list_lock)->rlock 1493 [<ffffffff8115b1ed>] writeback_inodes_wb+0x3d/0x150
&(&wb->list_lock)->rlock 3652 [<ffffffff8115a8e9>] writeback_sb_inodes+0x123/0x16f
&(&wb->list_lock)->rlock 1412 [<ffffffff8115a38e>] writeback_single_inode+0x17f/0x223
------------------------
&(&wb->list_lock)->rlock 3 [<ffffffff8110b5af>] bdi_lock_two+0x46/0x4b
&(&wb->list_lock)->rlock 6 [<ffffffff8115b189>] inode_wb_list_del+0x5f/0x86
&(&wb->list_lock)->rlock 2061 [<ffffffff8115af97>] __mark_inode_dirty+0x173/0x1cf
&(&wb->list_lock)->rlock 2629 [<ffffffff8115a8e9>] writeback_sb_inodes+0x123/0x16f--- fs/block_dev.c | 7 +- fs/fs-writeback.c | 97 +++++++++++++++++----------------- fs/inode.c | 5 - include/linux/backing-dev.h | 2 include/linux/writeback.h | 2 mm/backing-dev.c | 21 +++++-- mm/filemap.c | 6 +- mm/rmap.c | 4 - 8 files changed, 80 insertions(+), 64 deletions(-)
fs/block_dev.c +5 -2
@@ -55,13 +55,16 @@ EXPORT_SYMBOL(I_BDEV);static void bdev_inode_switch_bdi (struct inode *inode,struct backing_dev_info *dst) { -spin_lock (&inode_wb_list_lock); +struct backing_dev_info *old = inode->i_data.backing_dev_info; + +bdi_lock_two (&old->wb, &dst->wb);spin_lock (&inode->i_lock); inode->i_data.backing_dev_info = dst;if (inode->i_state &I_DIRTY )list_move (&inode->i_wb_list, &dst->wb.b_dirty);spin_unlock (&inode->i_lock); -spin_unlock (&inode_wb_list_lock); +spin_unlock (&old->wb.list_lock); +spin_unlock (&dst->wb.list_lock); }static sector_t max_block (struct block_device *bdev)
fs/fs-writeback.c +49 -48
301 lines not on this page · raw message
fs/inode.c +2 -3
@@ -37,7 +37,7 @@* inode_lru, inode->i_lru * inode_sb_list_lock protects: * sb->s_inodes, inode->i_sb_list -* inode_wb_list_lock protects: +* bdi->wb.list_lock protects: * bdi->wb.b_{dirty,io,more_io}, inode->i_wb_list * inode_hash_lock protects: * inode_hashtable, inode->i_hash
@@ -48,7 +48,7 @@* inode->i_lock * inode_lru_lock * -* inode_wb_list_lock +* bdi->wb.list_lock * inode->i_lock * * inode_hash_lock
@@ -111,7 +111,6 @@ static LIST_HEAD(inode_lru);static DEFINE_SPINLOCK (inode_lru_lock);__cacheline_aligned_in_smp DEFINE_SPINLOCK (inode_sb_list_lock ); -__cacheline_aligned_in_smp DEFINE_SPINLOCK (inode_wb_list_lock );/* * iprune_sem provides exclusion between the icache shrinking and the
include/linux/backing-dev.h +2 -0
@@ -57,6 +57,7 @@ struct bdi_writeback {struct list_head b_dirty;/* dirty inodes */ struct list_head b_io;/* parked for writeback */ struct list_head b_more_io;/* parked for more writeback */ +spinlock_t list_lock;/* protects the b_* lists */ };struct backing_dev_info {
@@ -106,6 +107,7 @@ int bdi_writeback_thread(void *data);int bdi_has_dirty_io (struct backing_dev_info *bdi);void bdi_arm_supers_timer (void );void bdi_wakeup_thread_delayed (struct backing_dev_info *bdi); +void bdi_lock_two (struct bdi_writeback *wb1,struct bdi_writeback *wb2);extern spinlock_t bdi_lock;extern struct list_head bdi_list;
include/linux/writeback.h +0 -2
@@ -9,8 +9,6 @@struct backing_dev_info ; -extern spinlock_t inode_wb_list_lock; -/* * fs/fs-writeback.c */
mm/backing-dev.c +17 -4
@@ -45,6 +45,17 @@ static struct timer_list sync_supers_timstatic int bdi_sync_supers (void *);static void sync_supers_timer_fn (unsigned long ); +void bdi_lock_two (struct bdi_writeback *wb1,struct bdi_writeback *wb2) +{ +if (wb1 < wb2) { +spin_lock (&wb1->list_lock); +spin_lock_nested (&wb2->list_lock,1 ); + }else { +spin_lock (&wb2->list_lock); +spin_lock_nested (&wb1->list_lock,1 ); + } +} +#ifdef CONFIG_DEBUG_FS #include <linux/debugfs.h> #include <linux/seq_file.h>
@@ -67,14 +78,14 @@ static int bdi_debug_stats_show(struct sstruct inode *inode; nr_wb = nr_dirty = nr_io = nr_more_io =0 ; -spin_lock (&inode_wb_list_lock); +spin_lock (&wb->list_lock);list_for_each_entry (inode, &wb->b_dirty, i_wb_list) nr_dirty++;list_for_each_entry (inode, &wb->b_io, i_wb_list) nr_io++;list_for_each_entry (inode, &wb->b_more_io, i_wb_list) nr_more_io++; -spin_unlock (&inode_wb_list_lock); +spin_unlock (&wb->list_lock);global_dirty_limits (&background_thresh, &dirty_thresh); bdi_thresh =bdi_dirty_limit (bdi, dirty_thresh);
@@ -628,6 +639,7 @@ static void bdi_wb_init(struct bdi_writeINIT_LIST_HEAD (&wb->b_dirty);INIT_LIST_HEAD (&wb->b_io);INIT_LIST_HEAD (&wb->b_more_io); +spin_lock_init (&wb->list_lock);setup_timer (&wb->wakeup_timer, wakeup_timer_fn, (unsigned long )bdi); }
@@ -676,11 +688,12 @@ void bdi_destroy(struct backing_dev_infoif (bdi_has_dirty_io (bdi)) {struct bdi_writeback *dst = &default_backing_dev_info.wb; -spin_lock (&inode_wb_list_lock); +bdi_lock_two (&bdi->wb, dst);list_splice (&bdi->wb.b_dirty, &dst->b_dirty);list_splice (&bdi->wb.b_io, &dst->b_io);list_splice (&bdi->wb.b_more_io, &dst->b_more_io); -spin_unlock (&inode_wb_list_lock); +spin_unlock (&bdi->wb.list_lock); +spin_unlock (&dst->list_lock); }bdi_unregister (bdi);
mm/filemap.c +3 -3
@@ -80,7 +80,7 @@* ->i_mutex * ->i_alloc_sem (various) * -* inode_wb_list_lock +* bdi->wb.list_lock * sb_lock (fs/fs-writeback.c) * ->mapping->tree_lock (__sync_single_inode) *
@@ -98,9 +98,9 @@* ->zone.lru_lock (check_pte_range->isolate_lru_page) * ->private_lock (page_remove_rmap->set_page_dirty) * ->tree_lock (page_remove_rmap->set_page_dirty) -* inode_wb_list_lock (page_remove_rmap->set_page_dirty) +* bdi.wb->list_lock (page_remove_rmap->set_page_dirty) * ->inode->i_lock (page_remove_rmap->set_page_dirty) -* inode_wb_list_lock (zap_pte_range->set_page_dirty) +* bdi.wb->list_lock (zap_pte_range->set_page_dirty) * ->inode->i_lock (zap_pte_range->set_page_dirty) * ->private_lock (zap_pte_range->__set_page_dirty_buffers) *
mm/rmap.c +2 -2
@@ -32,11 +32,11 @@* mmlist_lock (in mmput, drain_mmlist and others) * mapping->private_lock (in __set_page_dirty_buffers) * inode->i_lock (in set_page_dirty's __mark_inode_dirty) -* inode_wb_list_lock (in set_page_dirty's __mark_inode_dirty) +* bdi.wb->list_lock (in set_page_dirty's __mark_inode_dirty) * sb_lock (within inode_lock in fs/fs-writeback.c) * mapping->tree_lock (widely used, in set_page_dirty, * in arch-dependent flush_dcache_mmap_lock, -* within inode_wb_list_lock in __sync_single_inode) +* within bdi.wb->list_lock in __sync_single_inode) * * (code doesn't rely on that order so it could be switched around) * ->tasklist_lock
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH 2/2 writeback: elevate queue_io() into wb_writeback()
Code refactor for more logical code layout.
No behavior change.
- remove the mis-named __writeback_inodes_sb()
- wb_writeback()/writeback_inodes_wb() will decide when to queue_io() before calling __writeback_inodes_wb()
--- fs/fs-writeback.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
fs/fs-writeback.c +12 -15
@@ -570,17 +570,13 @@ static int writeback_sb_inodes(struct sureturn 1 ; } -void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +static void __writeback_inodes_wb (struct bdi_writeback *wb, +struct writeback_control *wbc) {int ret =0 ;if (!wbc->wb_start) wbc->wb_start = jiffies;/* livelock avoidance */ -spin_lock (&wb->list_lock); - -if (list_empty (&wb->b_io)) -queue_io (wb, wbc);while (!list_empty (&wb->b_io)) {struct inode *inode =wb_inode (wb->b_io.prev);
@@ -596,19 +592,16 @@ void writeback_inodes_wb(struct bdi_writif (ret)break ; } -spin_unlock (&wb->list_lock);/* Leave any unwritten inodes on b_io */ } -static void __writeback_inodes_sb (struct super_block *sb, -struct bdi_writeback *wb,struct writeback_control *wbc) +void writeback_inodes_wb (struct bdi_writeback *wb, +struct writeback_control *wbc) { -WARN_ON (!rwsem_is_locked (&sb->s_umount)); -spin_lock (&wb->list_lock);if (list_empty (&wb->b_io))queue_io (wb, wbc); -writeback_sb_inodes (sb, wb, wbc, true); +__writeback_inodes_wb (wb, wbc);spin_unlock (&wb->list_lock); }
@@ -674,7 +667,7 @@ static long wb_writeback(struct bdi_writ* The intended call sequence for WB_SYNC_ALL writeback is: * * wb_writeback() -* __writeback_inodes_sb() <== called only once +* writeback_sb_inodes() <== called only once * write_cache_pages() <== called once for each inode * (quickly) tag currently dirty pages * (maybe slowly) sync all tagged pages
@@ -722,10 +715,14 @@ static long wb_writeback(struct bdi_writ retry:trace_wbc_writeback_start (&wbc, wb->bdi); +spin_lock (&wb->list_lock); +if (list_empty (&wb->b_io)) +queue_io (wb, &wbc);if (work->sb) -__writeback_inodes_sb (work->sb, wb, &wbc); +writeback_sb_inodes (work->sb, wb, &wbc, true);else -writeback_inodes_wb (wb, &wbc); +__writeback_inodes_wb (wb, &wbc); +spin_unlock (&wb->list_lock);trace_wbc_writeback_written (&wbc, wb->bdi); work->nr_pages -= write_chunk - wbc.nr_to_write;
Re: [PATCH 2/2] writeback: elevate queue_io() into wb_writeback()
fs/fs-writeback.c static int writeback_sb_inodes(struct su · line 579
Quoted 12 lines
fs/fs-writeback.c +2
-2return 1 ; } -void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +static void __writeback_inodes_wb (struct bdi_writeback *wb, +struct writeback_control *wbc) {int ret =0 ;if (!wbc->wb_start) wbc->wb_start = jiffies;/* livelock avoidance */
This should move to writeback_inodes_wb and be unconditional as wb_writeback already always initializes 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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] writeback: elevate queue_io() into wb_writeback()
Quoted 10 lines · Christoph Hellwig
Quoted 10 lines
fs/fs-writeback.c +2
-2return 1 ; } -void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +static void __writeback_inodes_wb (struct bdi_writeback *wb, +struct writeback_control *wbc) {int ret =0 ;
Quoted 5 lines
fs/fs-writeback.c static int writeback_sb_inodes(struct su · line 579
Quoted 2 lines
fs/fs-writeback.c +0
-0if (!wbc->wb_start) wbc->wb_start = jiffies;/* livelock avoidance */ This should move to
writeback_inodes_wband be unconditional aswb_writebackalready always initializes it.
Never mind :) wbc->wb_start has been killed in a later patch named "writeback: avoid extra sync work at enqueue time".
Thanks,
Fengguang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 2/2] writeback: elevate queue_io() into wb_writeback()
Quoted 5 lines · Wu Fengguang
Quoted 2 lines
This should move to
writeback_inodes_wband be unconditional aswb_writebackalready always initializes it.Never mind :)
wbc->wb_starthas been killed in a later patch named "writeback: avoid extra sync work at enqueue time".
Even better. I was already wondering why we'd need two different jiffies and jiffies + offset value in the writeback code.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Andrew Morton
maintainerQuoted 7 lines · On Tue, 26 Apr 2011 22:42:19 +0800 Wu Fengguang <[email protected]> wrote:
fs/block_dev.c +2
-1@@ -55,13 +55,16 @@ EXPORT_SYMBOL(I_BDEV);static void bdev_inode_switch_bdi (struct inode *inode,struct backing_dev_info *dst) { -spin_lock (&inode_wb_list_lock); +struct backing_dev_info *old = inode->i_data.backing_dev_info; +
fs/block_dev.c EXPORT_SYMBOL(I_BDEV); · line 68
Quoted 10 lines
fs/block_dev.c +3
-1+bdi_lock_two (&old->wb, &dst->wb);spin_lock (&inode->i_lock); inode->i_data.backing_dev_info = dst;if (inode->i_state &I_DIRTY )list_move (&inode->i_wb_list, &dst->wb.b_dirty);spin_unlock (&inode->i_lock); -spin_unlock (&inode_wb_list_lock); +spin_unlock (&old->wb.list_lock); +spin_unlock (&dst->wb.list_lock); }
Has this patch been well tested under lockdep?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 14 lines · Andrew Morton
Quoted 11 lines · On Tue, 26 Apr 2011 22:42:19 +0800 Wu Fengguang <[email protected]> wrote:
fs/block_dev.c +3
-1@@ -55,13 +55,16 @@ EXPORT_SYMBOL(I_BDEV);static void bdev_inode_switch_bdi (struct inode *inode,struct backing_dev_info *dst) { -spin_lock (&inode_wb_list_lock); +struct backing_dev_info *old = inode->i_data.backing_dev_info; + +bdi_lock_two (&old->wb, &dst->wb);spin_lock (&inode->i_lock); inode->i_data.backing_dev_info = dst;if (inode->i_state &I_DIRTY )
Quoted 8 lines
fs/block_dev.c EXPORT_SYMBOL(I_BDEV); · line 68
Quoted 6 lines
fs/block_dev.c +2
-1list_move (&inode->i_wb_list, &dst->wb.b_dirty);spin_unlock (&inode->i_lock); -spin_unlock (&inode_wb_list_lock); +spin_unlock (&old->wb.list_lock); +spin_unlock (&dst->wb.list_lock); }Has this patch been well tested under lockdep?
Yes, it runs OK on concurrent dd and dd+tar workloads over all major filesystems, including NFS. Lockdep is always enabled in my kernels, no warnings are found in dmesg.
Thanks,
Fengguang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 1 line · Andrew Morton
Has this patch been well tested under lockdep?
Yes. There's only two places locking two bdis, and both use the bdi_lock_two helper which imposes an ordering, and tells lockdep about 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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>