On Sun, Apr 22, 2018 at 5:24 AM Greg Kroah-Hartman < gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > On Sun, Apr 22, 2018 at 04:15:12AM -0700, Nathan Chancellor wrote: > > From: Greg Thelen <gthelen@xxxxxxxxxx> > > > > commit 2e898e4c0a3897ccd434adac5abb8330194f527b upstream. > > > > lock_page_memcg()/unlock_page_memcg() use spin_lock_irqsave/restore() if > > the page's memcg is undergoing move accounting, which occurs when a > > process leaves its memcg for a new one that has > > memory.move_charge_at_immigrate set. > > > > unlocked_inode_to_wb_begin,end() use spin_lock_irq/spin_unlock_irq() if > > the given inode is switching writeback domains. Switches occur when > > enough writes are issued from a new domain. > > > > This existing pattern is thus suspicious: > > lock_page_memcg(page); > > unlocked_inode_to_wb_begin(inode, &locked); > > ... > > unlocked_inode_to_wb_end(inode, locked); > > unlock_page_memcg(page); > > > > If both inode switch and process memcg migration are both in-flight then > > unlocked_inode_to_wb_end() will unconditionally enable interrupts while > > still holding the lock_page_memcg() irq spinlock. This suggests the > > possibility of deadlock if an interrupt occurs before unlock_page_memcg(). > > > > truncate > > __cancel_dirty_page > > lock_page_memcg > > unlocked_inode_to_wb_begin > > unlocked_inode_to_wb_end > > <interrupts mistakenly enabled> > > <interrupt> > > end_page_writeback > > test_clear_page_writeback > > lock_page_memcg > > <deadlock> > > unlock_page_memcg > > > > Due to configuration limitations this deadlock is not currently possible > > because we don't mix cgroup writeback (a cgroupv2 feature) and > > memory.move_charge_at_immigrate (a cgroupv1 feature). > > > > If the kernel is hacked to always claim inode switching and memcg > > moving_account, then this script triggers lockup in less than a minute: > > > > cd /mnt/cgroup/memory > > mkdir a b > > echo 1 > a/memory.move_charge_at_immigrate > > echo 1 > b/memory.move_charge_at_immigrate > > ( > > echo $BASHPID > a/cgroup.procs > > while true; do > > dd if=/dev/zero of=/mnt/big bs=1M count=256 > > done > > ) & > > while true; do > > sync > > done & > > sleep 1h & > > SLEEP=$! > > while true; do > > echo $SLEEP > a/cgroup.procs > > echo $SLEEP > b/cgroup.procs > > done > > > > The deadlock does not seem possible, so it's debatable if there's any > > reason to modify the kernel. I suggest we should to prevent future > > surprises. And Wang Long said "this deadlock occurs three times in our > > environment", so there's more reason to apply this, even to stable. > > Stable 4.4 has minor conflicts applying this patch. For a clean 4.4 patch > > see "[PATCH for-4.4] writeback: safer lock nesting" > > https://lkml.org/lkml/2018/4/11/146 > > > > Wang Long said "this deadlock occurs three times in our environment" > > > > [gthelen@xxxxxxxxxx: v4] > > Link: http://lkml.kernel.org/r/20180411084653.254724-1-gthelen@xxxxxxxxxx > > [akpm@xxxxxxxxxxxxxxxxxxxx: comment tweaks, struct initialization simplification] > > Change-Id: Ibb773e8045852978f6207074491d262f1b3fb613 > > Link: http://lkml.kernel.org/r/20180410005908.167976-1-gthelen@xxxxxxxxxx > > Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates") > > Signed-off-by: Greg Thelen <gthelen@xxxxxxxxxx> > > Reported-by: Wang Long <wanglong19@xxxxxxxxxxx> > > Acked-by: Wang Long <wanglong19@xxxxxxxxxxx> > > Acked-by: Michal Hocko <mhocko@xxxxxxxx> > > Reviewed-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > Cc: Johannes Weiner <hannes@xxxxxxxxxxx> > > Cc: Tejun Heo <tj@xxxxxxxxxx> > > Cc: Nicholas Piggin <npiggin@xxxxxxxxx> > > Cc: <stable@xxxxxxxxxxxxxxx> [v4.2+] > > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > > [natechancellor: Adjust context due to lack of b93b016313b3b] > > Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx> > Thanks for all of these, now queued up. > greg k-h I reviewed the 4.4, 4.9, 4.14, 4.16 queued stable backports for ("writeback: safer lock nesting"). They all look good. Thanks. I don't know if it's customary to add an author's reviewed-by to non-trivial backports. If useful, here you go: Reviewed-by: Greg Thelen <gthelen@xxxxxxxxxx>