Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: [RFC] Is it correctly that the usage for spin_{lock|unlock}_irq in
 clear_page_dirty_for_io
References: <157ed606-4a61-508b-d26a-2f5d638f39bb@meituan.com>
To:     tj@kernel.org, hannes@cmpxchg.org
Cc:     gthelen@google.com, npiggin@suse.de, akpm@osdl.org,
        linux-kernel@vger.kernel.org, wanglong19@meituan.com
From:   Wang Long <wanglong19@meituan.com>
Message-ID: <d5f73e91-6fd5-12ab-d005-93a72e146464@meituan.com>
Date:   Mon, 2 Apr 2018 19:50:50 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0)
 Gecko/20100101 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <157ed606-4a61-508b-d26a-2f5d638f39bb@meituan.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Hi,=C2=A0 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){
...
...
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 memcg =3D mem_cgroup_begin_page_stat(page); ----------=
(a)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 wb =3D unlocked_inode_to_wb_begin(inode, &locked); ---=
------(b)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 if (TestClearPageDirty(page)) {
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mem_cg=
roup_dec_page_stat(memcg, MEM_CGROUP_STAT_DIRTY);
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dec_zo=
ne_page_state(page, NR_FILE_DIRTY);
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dec_wb=
_stat(wb, WB_RECLAIMABLE);
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D=
1;
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 }
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 unlocked_inode_to_wb_end(inode, locked); -----------(c=
)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 mem_cgroup_end_page_stat(memcg); -------------(d)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 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)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_lock_irq(&inode->i_mappi=
ng->tree_lock); ------------(b)
 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 spin_unlock_irq(&inode->i_map=
ping->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.=C2=A0 But it really exists.

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

Thanks:-)