lkml.org 
[lkml]   [2018]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Subject[RFC] Is it correctly that the usage for spin_{lock|unlock}_irq in clear_page_dirty_for_io
From
Date

Hi,  Johannes Weiner and Tejun Heo

I use linux-4.4.y to test the new cgroup controller io and the current
stable kernel linux-4.4.y has the follow logic


int clear_page_dirty_for_io(struct page *page){
...
...
                memcg = mem_cgroup_begin_page_stat(page); ----------(a)
                wb = unlocked_inode_to_wb_begin(inode, &locked); ---------(b)
                if (TestClearPageDirty(page)) {
                        mem_cgroup_dec_page_stat(memcg, MEM_CGROUP_STAT_DIRTY);
                        dec_zone_page_state(page, NR_FILE_DIRTY);
                        dec_wb_stat(wb, WB_RECLAIMABLE);
                        ret =1;
                }
                unlocked_inode_to_wb_end(inode, locked); -----------(c)
                mem_cgroup_end_page_stat(memcg); -------------(d)
                return ret;
...
...
}


when memcg is moving, and I_WB_SWITCH flags for inode is set. the logic
is the following:


spin_lock_irqsave(&memcg->move_lock, flags); -------------(a)
        spin_lock_irq(&inode->i_mapping->tree_lock); ------------(b)
        spin_unlock_irq(&inode->i_mapping->tree_lock); -----------(c)
spin_unlock_irqrestore(&memcg->move_lock, flags); -----------(d)


after (c) , the local irq is enabled. I think it is not correct.

We get a deadlock backtrace after (c), the cpu get an softirq and in the
irq it also call mem_cgroup_begin_page_stat to lock the same
memcg->move_lock.

Since the conditions are too harsh, this scenario is difficult to
reproduce.  But it really exists.

So how about change (b) (c) to spin_lock_irqsave/spin_lock_irqrestore?

Thanks:-)

\
 
 \ /
  Last update: 2018-04-02 13:59    [W:0.067 / U:3.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site