On Sun, Apr 22, 2018 at 12:42:57PM +0200, Greg Kroah-Hartman wrote: > On Sun, Apr 22, 2018 at 03:36:32AM -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> > > I tried a "simple" backport like this as well, and it too blew up when > building :( > > This patch dies with: > > $ make M=mm > CC mm/filemap.o > In file included from ./include/linux/kernel.h:13:0, > from ./include/linux/list.h:9, > from ./include/linux/wait.h:7, > from ./include/linux/wait_bit.h:8, > from ./include/linux/fs.h:6, > from ./include/linux/dax.h:5, > from mm/filemap.c:14: > ./include/linux/backing-dev.h: In function ‘unlocked_inode_to_wb_begin’: > ./include/linux/backing-dev.h:373:52: error: ‘flags’ undeclared (first use in this function); did you mean ‘class’? > spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags); > ^ > ./include/linux/typecheck.h:11:9: note: in definition of macro ‘typecheck’ > typeof(x) __dummy2; \ > ^ > ./include/linux/spinlock.h:340:2: note: in expansion of macro ‘raw_spin_lock_irqsave’ > raw_spin_lock_irqsave(spinlock_check(lock), flags); \ > ^~~~~~~~~~~~~~~~~~~~~ > ./include/linux/backing-dev.h:373:3: note: in expansion of macro ‘spin_lock_irqsave’ > spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags); > ^~~~~~~~~~~~~~~~~ > ./include/linux/backing-dev.h:373:52: note: each undeclared identifier is reported only once for each function it appears in > spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags); > ^ > ./include/linux/typecheck.h:11:9: note: in definition of macro ‘typecheck’ > typeof(x) __dummy2; \ > ^ > ./include/linux/spinlock.h:340:2: note: in expansion of macro ‘raw_spin_lock_irqsave’ > raw_spin_lock_irqsave(spinlock_check(lock), flags); \ > ^~~~~~~~~~~~~~~~~~~~~ > ./include/linux/backing-dev.h:373:3: note: in expansion of macro ‘spin_lock_irqsave’ > spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags); > ^~~~~~~~~~~~~~~~~ > ./include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast > (void)(&__dummy == &__dummy2); \ > ^ > ./include/linux/spinlock.h:221:3: note: in expansion of macro ‘typecheck’ > typecheck(unsigned long, flags); \ > ^~~~~~~~~ > ./include/linux/spinlock.h:340:2: note: in expansion of macro ‘raw_spin_lock_irqsave’ > raw_spin_lock_irqsave(spinlock_check(lock), flags); \ > ^~~~~~~~~~~~~~~~~~~~~ > ./include/linux/backing-dev.h:373:3: note: in expansion of macro ‘spin_lock_irqsave’ > spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags); > ^~~~~~~~~~~~~~~~~ > In file included from mm/filemap.c:29:0: > ./include/linux/backing-dev.h: In function ‘unlocked_inode_to_wb_end’: > ./include/linux/backing-dev.h:391:56: error: ‘flags’ undeclared (first use in this function); did you mean ‘class’? > spin_unlock_irqrestore(&inode->i_mapping->tree_lock, flags); > ^~~~~ > class > make[1]: *** [scripts/Makefile.build:325: mm/filemap.o] Error 1 > make: *** [Makefile:1561: _module_mm] Error 2 > > > Can you test-build your patches? :) > > thanks, > > greg k-h Sigh I don't know what happened but I was looking at an old version of the patch for reference :/ I just send v2 and I have verified everything builds properly just using the x86_64_defconfig. Again really sorry about all the noice! Nathan