On Mon, Jul 02, 2012 at 08:14:02PM +0900, KAMEZAWA Hiroyuki wrote:
> (2012/06/28 20:01), Sha Zhengju wrote:
> > From: Sha Zhengju <[email protected]>
> >
> > Commit a8e7d49a(Fix race in create_empty_buffers() vs
> > __set_page_dirty_buffers())
> > extracts TestSetPageDirty from __set_page_dirty and is far away from
> > account_page_dirtied.But it's better to make the two operations in one
> > single
> > function to keep modular.So in order to avoid the potential race mentioned
> > in
> > commit a8e7d49a, we can hold private_lock until __set_page_dirty completes.
> > I guess there's no deadlock between ->private_lock and ->tree_lock by quick
> > look.
> >
> > It's a prepare patch for following memcg dirty page accounting patches.
> >
> > Signed-off-by: Sha Zhengju <[email protected]>
>
> I think there is no problem with the lock order.
Me think so, too.
> My small concern is the impact on the performance. IIUC, lock contention here
> can be
> seen if multiple threads write to the same file in parallel.
> Do you have any numbers before/after the patch ?
That would be a worthwhile test. The patch moves ->tree_lock and
->i_lock into ->private_lock, these are often contented locks..
For example, in the below case of 12 hard disks, each running 1 dd
write, the ->tree_lock and ->private_lock have the top #1 and #2
contentions.
lkp-nex04/JBOD-12HDD-thresh=1000M/ext4-1dd-1-3.3.0/lock_stat
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
class name con-bounces contentions
waittime-min waittime-max waittime-total acq-bounces acquisitions
holdtime-min holdtime-max holdtime-total
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
&(&mapping->tree_lock)->rlock: 18629034 19138284
0.09 1029.32 24353812.07 49650988 482883410
0.11 186.88 260706119.09
-----------------------------
&(&mapping->tree_lock)->rlock 783
[<ffffffff81109267>] tag_pages_for_writeback+0x2b/0x9d
&(&mapping->tree_lock)->rlock 3195817
[<ffffffff81100d6c>] add_to_page_cache_locked+0xa3/0x119
&(&mapping->tree_lock)->rlock 3863710
[<ffffffff81108df7>] test_set_page_writeback+0x63/0x140
&(&mapping->tree_lock)->rlock 3311518
[<ffffffff81172ade>] __set_page_dirty+0x25/0xa5
-----------------------------
&(&mapping->tree_lock)->rlock 3450725
[<ffffffff81100d6c>] add_to_page_cache_locked+0xa3/0x119
&(&mapping->tree_lock)->rlock 3225542
[<ffffffff81172ade>] __set_page_dirty+0x25/0xa5
&(&mapping->tree_lock)->rlock 2241958
[<ffffffff81108df7>] test_set_page_writeback+0x63/0x140
&(&mapping->tree_lock)->rlock 7339603
[<ffffffff8110ac33>] test_clear_page_writeback+0x64/0x155
...............................................................................................................................................................................................
&(&mapping->private_lock)->rlock: 1165199 1191201
0.11 2843.25 1621608.38 13341420 152761848
0.10 3727.92 33559035.07
--------------------------------
&(&mapping->private_lock)->rlock 1
[<ffffffff81172913>] __find_get_block_slow+0x5a/0x135
&(&mapping->private_lock)->rlock 385576
[<ffffffff811735d6>] create_empty_buffers+0x48/0xbf
&(&mapping->private_lock)->rlock 805624
[<ffffffff8117346d>] try_to_free_buffers+0x57/0xaa
--------------------------------
&(&mapping->private_lock)->rlock 1
[<ffffffff811746dd>] __getblk+0x1b8/0x257
&(&mapping->private_lock)->rlock 952718
[<ffffffff8117346d>] try_to_free_buffers+0x57/0xaa
&(&mapping->private_lock)->rlock 238482
[<ffffffff811735d6>] create_empty_buffers+0x48/0xbf
Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
