Re: [PATCH 4.4] writeback: safer lock nesting

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Apr 22, 2018 at 03:07:59AM -0700, Nathan Chancellor wrote:
> From: Greg Thelen <gthelen@google.com>
> 
> 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@google.com: v4]
>   Link: http://lkml.kernel.org/r/20180411084653.254724-1-gthelen@google.com
> [akpm@linux-foundation.org: comment tweaks, struct initialization simplification]
> Change-Id: Ibb773e8045852978f6207074491d262f1b3fb613
> Link: http://lkml.kernel.org/r/20180410005908.167976-1-gthelen@google.com
> Fixes: 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb transaction and use it for stat updates")
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Reported-by: Wang Long <wanglong19@meituan.com>
> Acked-by: Wang Long <wanglong19@meituan.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: <stable@vger.kernel.org>	[v4.2+]
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [natechancellor: Applied to 4.4 based on Greg's backport on lkml.org]

I need a working patch for 4.9.y, 4.14.y and 4.16.y first before I can
apply this one :(

thanks,

greg k-h