* [PATCH 4.14/4.16] writeback: safer lock nesting
2018-04-22 10:36 Backport of 2e898e4c0a38 for 4.4, 4.9, 4.14, and 4.16 Nathan Chancellor
@ 2018-04-22 10:36 ` Nathan Chancellor
2018-04-22 10:42 ` Greg Kroah-Hartman
2018-04-22 10:36 ` [PATCH 4.4] " Nathan Chancellor
2018-04-22 10:36 ` [PATCH 4.9] " Nathan Chancellor
2 siblings, 1 reply; 7+ messages in thread
From: Nathan Chancellor @ 2018-04-22 10:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, stable
Cc: Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds, Nathan Chancellor
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: Adjust context due to lack of b93b016313b3b]
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
fs/fs-writeback.c | 7 ++++---
include/linux/backing-dev-defs.h | 5 +++++
include/linux/backing-dev.h | 30 ++++++++++++++++--------------
mm/page-writeback.c | 18 +++++++++---------
4 files changed, 34 insertions(+), 26 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index d4d04fee568a..40c34a0ef58a 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -745,11 +745,12 @@ int inode_congested(struct inode *inode, int cong_bits)
*/
if (inode && inode_to_wb_is_valid(inode)) {
struct bdi_writeback *wb;
- bool locked, congested;
+ struct wb_lock_cookie lock_cookie = {};
+ bool congested;
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &lock_cookie);
congested = wb_congested(wb, cong_bits);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &lock_cookie);
return congested;
}
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index bfe86b54f6c1..0bd432a4d7bd 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -223,6 +223,11 @@ static inline void set_bdi_congested(struct backing_dev_info *bdi, int sync)
set_wb_congested(bdi->wb.congested, sync);
}
+struct wb_lock_cookie {
+ bool locked;
+ unsigned long flags;
+};
+
#ifdef CONFIG_CGROUP_WRITEBACK
/**
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 3e4ce54d84ab..43e5aef01930 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -346,7 +346,7 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
/**
* unlocked_inode_to_wb_begin - begin unlocked inode wb access transaction
* @inode: target inode
- * @lockedp: temp bool output param, to be passed to the end function
+ * @cookie: output param, to be passed to the end function
*
* The caller wants to access the wb associated with @inode but isn't
* holding inode->i_lock, mapping->tree_lock or wb->list_lock. This
@@ -354,12 +354,12 @@ static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
* association doesn't change until the transaction is finished with
* unlocked_inode_to_wb_end().
*
- * The caller must call unlocked_inode_to_wb_end() with *@lockdep
- * afterwards and can't sleep during transaction. IRQ may or may not be
- * disabled on return.
+ * The caller must call unlocked_inode_to_wb_end() with *@cookie afterwards and
+ * can't sleep during the transaction. IRQs may or may not be disabled on
+ * return.
*/
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
rcu_read_lock();
@@ -367,10 +367,10 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
* Paired with store_release in inode_switch_wb_work_fn() and
* ensures that we see the new wb if we see cleared I_WB_SWITCH.
*/
- *lockedp = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
+ cookie->locked = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
- if (unlikely(*lockedp))
- spin_lock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags);
/*
* Protected by either !I_WB_SWITCH + rcu_read_lock() or tree_lock.
@@ -382,12 +382,13 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
/**
* unlocked_inode_to_wb_end - end inode wb access transaction
* @inode: target inode
- * @locked: *@lockedp from unlocked_inode_to_wb_begin()
+ * @cookie: @cookie from unlocked_inode_to_wb_begin()
*/
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
- if (unlikely(locked))
- spin_unlock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_unlock_irqrestore(&inode->i_mapping->tree_lock, flags);
rcu_read_unlock();
}
@@ -434,12 +435,13 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
}
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
return inode_to_wb(inode);
}
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
}
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 586f31261c83..8369572e1f7d 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2501,13 +2501,13 @@ void account_page_redirty(struct page *page)
if (mapping && mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
current->nr_dirtied--;
dec_node_page_state(page, NR_DIRTIED);
dec_wb_stat(wb, WB_DIRTIED);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
}
}
EXPORT_SYMBOL(account_page_redirty);
@@ -2613,15 +2613,15 @@ void __cancel_dirty_page(struct page *page)
if (mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
lock_page_memcg(page);
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
if (TestClearPageDirty(page))
account_page_cleaned(page, mapping, wb);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
unlock_page_memcg(page);
} else {
ClearPageDirty(page);
@@ -2653,7 +2653,7 @@ int clear_page_dirty_for_io(struct page *page)
if (mapping && mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
/*
* Yes, Virginia, this is indeed insane.
@@ -2690,14 +2690,14 @@ int clear_page_dirty_for_io(struct page *page)
* always locked coming in here, so we get the desired
* exclusion.
*/
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
if (TestClearPageDirty(page)) {
dec_lruvec_page_state(page, NR_FILE_DIRTY);
dec_zone_page_state(page, NR_ZONE_WRITE_PENDING);
dec_wb_stat(wb, WB_RECLAIMABLE);
ret = 1;
}
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
return ret;
}
return TestClearPageDirty(page);
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 4.14/4.16] writeback: safer lock nesting
2018-04-22 10:36 ` [PATCH 4.14/4.16] writeback: safer lock nesting Nathan Chancellor
@ 2018-04-22 10:42 ` Greg Kroah-Hartman
2018-04-22 10:47 ` Nathan Chancellor
2018-04-22 11:17 ` Nathan Chancellor
0 siblings, 2 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-22 10:42 UTC (permalink / raw)
To: Nathan Chancellor
Cc: stable, Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds
On Sun, Apr 22, 2018 at 03:36:32AM -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: Adjust context due to lack of b93b016313b3b]
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 4.14/4.16] writeback: safer lock nesting
2018-04-22 10:42 ` Greg Kroah-Hartman
@ 2018-04-22 10:47 ` Nathan Chancellor
2018-04-22 11:17 ` Nathan Chancellor
1 sibling, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2018-04-22 10:47 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: stable, Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds
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@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: Adjust context due to lack of b93b016313b3b]
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>
> 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
Yes, sorry, I normally do but it seemed like an easy resolution :(
I'll fix this up and resend, sorry for the noise!
Nathan
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 4.14/4.16] writeback: safer lock nesting
2018-04-22 10:42 ` Greg Kroah-Hartman
2018-04-22 10:47 ` Nathan Chancellor
@ 2018-04-22 11:17 ` Nathan Chancellor
1 sibling, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2018-04-22 11:17 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: stable, Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds
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@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: Adjust context due to lack of b93b016313b3b]
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4.4] writeback: safer lock nesting
2018-04-22 10:36 Backport of 2e898e4c0a38 for 4.4, 4.9, 4.14, and 4.16 Nathan Chancellor
2018-04-22 10:36 ` [PATCH 4.14/4.16] writeback: safer lock nesting Nathan Chancellor
@ 2018-04-22 10:36 ` Nathan Chancellor
2018-04-22 10:36 ` [PATCH 4.9] " Nathan Chancellor
2 siblings, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2018-04-22 10:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, stable
Cc: Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds, Nathan Chancellor
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]
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
fs/fs-writeback.c | 7 ++++---
include/linux/backing-dev-defs.h | 5 +++++
include/linux/backing-dev.h | 31 +++++++++++++++++--------------
mm/page-writeback.c | 18 +++++++++---------
4 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 22b30249fbcb..0fe667875852 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -747,11 +747,12 @@ int inode_congested(struct inode *inode, int cong_bits)
*/
if (inode && inode_to_wb_is_valid(inode)) {
struct bdi_writeback *wb;
- bool locked, congested;
+ struct wb_lock_cookie lock_cookie = {};
+ bool congested;
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &lock_cookie);
congested = wb_congested(wb, cong_bits);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &lock_cookie);
return congested;
}
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 140c29635069..a307c37c2e6c 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -191,6 +191,11 @@ static inline void set_bdi_congested(struct backing_dev_info *bdi, int sync)
set_wb_congested(bdi->wb.congested, sync);
}
+struct wb_lock_cookie {
+ bool locked;
+ unsigned long flags;
+};
+
#ifdef CONFIG_CGROUP_WRITEBACK
/**
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 89d3de3e096b..361274ce5815 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -366,7 +366,7 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
/**
* unlocked_inode_to_wb_begin - begin unlocked inode wb access transaction
* @inode: target inode
- * @lockedp: temp bool output param, to be passed to the end function
+ * @cookie: output param, to be passed to the end function
*
* The caller wants to access the wb associated with @inode but isn't
* holding inode->i_lock, mapping->tree_lock or wb->list_lock. This
@@ -374,12 +374,12 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
* association doesn't change until the transaction is finished with
* unlocked_inode_to_wb_end().
*
- * The caller must call unlocked_inode_to_wb_end() with *@lockdep
- * afterwards and can't sleep during transaction. IRQ may or may not be
- * disabled on return.
+ * The caller must call unlocked_inode_to_wb_end() with *@cookie afterwards and
+ * can't sleep during the transaction. IRQs may or may not be disabled on
+ * return.
*/
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
rcu_read_lock();
@@ -387,10 +387,10 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
* Paired with store_release in inode_switch_wb_work_fn() and
* ensures that we see the new wb if we see cleared I_WB_SWITCH.
*/
- *lockedp = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
+ cookie->locked = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
- if (unlikely(*lockedp))
- spin_lock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_lock_irqsave(&inode->i_mapping->tree_lock, cookie->flags);
/*
* Protected by either !I_WB_SWITCH + rcu_read_lock() or tree_lock.
@@ -402,12 +402,14 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
/**
* unlocked_inode_to_wb_end - end inode wb access transaction
* @inode: target inode
- * @locked: *@lockedp from unlocked_inode_to_wb_begin()
+ * @cookie: @cookie from unlocked_inode_to_wb_begin()
*/
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
- if (unlikely(locked))
- spin_unlock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_unlock_irqrestore(&inode->i_mapping->tree_lock,
+ cookie->flags);
rcu_read_unlock();
}
@@ -454,12 +456,13 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
}
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
return inode_to_wb(inode);
}
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
}
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 6d0dbde4503b..3309dbda7ffa 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2510,13 +2510,13 @@ void account_page_redirty(struct page *page)
if (mapping && mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
current->nr_dirtied--;
dec_zone_page_state(page, NR_DIRTIED);
dec_wb_stat(wb, WB_DIRTIED);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
}
}
EXPORT_SYMBOL(account_page_redirty);
@@ -2622,15 +2622,15 @@ void cancel_dirty_page(struct page *page)
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
struct mem_cgroup *memcg;
- bool locked;
+ struct wb_lock_cookie cookie = {};
memcg = mem_cgroup_begin_page_stat(page);
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
if (TestClearPageDirty(page))
account_page_cleaned(page, mapping, memcg, wb);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
mem_cgroup_end_page_stat(memcg);
} else {
ClearPageDirty(page);
@@ -2663,7 +2663,7 @@ int clear_page_dirty_for_io(struct page *page)
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
struct mem_cgroup *memcg;
- bool locked;
+ struct wb_lock_cookie cookie = {};
/*
* Yes, Virginia, this is indeed insane.
@@ -2701,14 +2701,14 @@ int clear_page_dirty_for_io(struct page *page)
* exclusion.
*/
memcg = mem_cgroup_begin_page_stat(page);
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
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);
+ unlocked_inode_to_wb_end(inode, &cookie);
mem_cgroup_end_page_stat(memcg);
return ret;
}
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4.9] writeback: safer lock nesting
2018-04-22 10:36 Backport of 2e898e4c0a38 for 4.4, 4.9, 4.14, and 4.16 Nathan Chancellor
2018-04-22 10:36 ` [PATCH 4.14/4.16] writeback: safer lock nesting Nathan Chancellor
2018-04-22 10:36 ` [PATCH 4.4] " Nathan Chancellor
@ 2018-04-22 10:36 ` Nathan Chancellor
2 siblings, 0 replies; 7+ messages in thread
From: Nathan Chancellor @ 2018-04-22 10:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, stable
Cc: Greg Thelen, Johannes Weiner, Tejun Heo, Nicholas Piggin,
Andrew Morton, Linus Torvalds, Nathan Chancellor
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: Adjust context due to lack of b93b016313b3b]
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
fs/fs-writeback.c | 7 ++++---
include/linux/backing-dev-defs.h | 5 +++++
include/linux/backing-dev.h | 30 ++++++++++++++++--------------
mm/page-writeback.c | 18 +++++++++---------
4 files changed, 34 insertions(+), 26 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 0703a1179847..3d8b35f28a9b 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -745,11 +745,12 @@ int inode_congested(struct inode *inode, int cong_bits)
*/
if (inode && inode_to_wb_is_valid(inode)) {
struct bdi_writeback *wb;
- bool locked, congested;
+ struct wb_lock_cookie lock_cookie = {};
+ bool congested;
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &lock_cookie);
congested = wb_congested(wb, cong_bits);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &lock_cookie);
return congested;
}
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index c357f27d5483..32728ff8095c 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -191,6 +191,11 @@ static inline void set_bdi_congested(struct backing_dev_info *bdi, int sync)
set_wb_congested(bdi->wb.congested, sync);
}
+struct wb_lock_cookie {
+ bool locked;
+ unsigned long flags;
+};
+
#ifdef CONFIG_CGROUP_WRITEBACK
/**
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 43b93a947e61..608a82922f04 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -366,7 +366,7 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
/**
* unlocked_inode_to_wb_begin - begin unlocked inode wb access transaction
* @inode: target inode
- * @lockedp: temp bool output param, to be passed to the end function
+ * @cookie: output param, to be passed to the end function
*
* The caller wants to access the wb associated with @inode but isn't
* holding inode->i_lock, mapping->tree_lock or wb->list_lock. This
@@ -374,12 +374,12 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
* association doesn't change until the transaction is finished with
* unlocked_inode_to_wb_end().
*
- * The caller must call unlocked_inode_to_wb_end() with *@lockdep
- * afterwards and can't sleep during transaction. IRQ may or may not be
- * disabled on return.
+ * The caller must call unlocked_inode_to_wb_end() with *@cookie afterwards and
+ * can't sleep during the transaction. IRQs may or may not be disabled on
+ * return.
*/
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
rcu_read_lock();
@@ -387,10 +387,10 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
* Paired with store_release in inode_switch_wb_work_fn() and
* ensures that we see the new wb if we see cleared I_WB_SWITCH.
*/
- *lockedp = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
+ cookie->locked = smp_load_acquire(&inode->i_state) & I_WB_SWITCH;
- if (unlikely(*lockedp))
- spin_lock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_lock_irqsave(&inode->i_mapping->tree_lock, *flags);
/*
* Protected by either !I_WB_SWITCH + rcu_read_lock() or tree_lock.
@@ -402,12 +402,13 @@ unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
/**
* unlocked_inode_to_wb_end - end inode wb access transaction
* @inode: target inode
- * @locked: *@lockedp from unlocked_inode_to_wb_begin()
+ * @cookie: @cookie from unlocked_inode_to_wb_begin()
*/
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
- if (unlikely(locked))
- spin_unlock_irq(&inode->i_mapping->tree_lock);
+ if (unlikely(cookie->locked))
+ spin_unlock_irqrestore(&inode->i_mapping->tree_lock, flags);
rcu_read_unlock();
}
@@ -454,12 +455,13 @@ static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
}
static inline struct bdi_writeback *
-unlocked_inode_to_wb_begin(struct inode *inode, bool *lockedp)
+unlocked_inode_to_wb_begin(struct inode *inode, struct wb_lock_cookie *cookie)
{
return inode_to_wb(inode);
}
-static inline void unlocked_inode_to_wb_end(struct inode *inode, bool locked)
+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+ struct wb_lock_cookie *cookie)
{
}
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 439cc63ad903..807236aed275 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2506,13 +2506,13 @@ void account_page_redirty(struct page *page)
if (mapping && mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
current->nr_dirtied--;
dec_node_page_state(page, NR_DIRTIED);
dec_wb_stat(wb, WB_DIRTIED);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
}
}
EXPORT_SYMBOL(account_page_redirty);
@@ -2618,15 +2618,15 @@ void cancel_dirty_page(struct page *page)
if (mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
lock_page_memcg(page);
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
if (TestClearPageDirty(page))
account_page_cleaned(page, mapping, wb);
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
unlock_page_memcg(page);
} else {
ClearPageDirty(page);
@@ -2658,7 +2658,7 @@ int clear_page_dirty_for_io(struct page *page)
if (mapping && mapping_cap_account_dirty(mapping)) {
struct inode *inode = mapping->host;
struct bdi_writeback *wb;
- bool locked;
+ struct wb_lock_cookie cookie = {};
/*
* Yes, Virginia, this is indeed insane.
@@ -2695,7 +2695,7 @@ int clear_page_dirty_for_io(struct page *page)
* always locked coming in here, so we get the desired
* exclusion.
*/
- wb = unlocked_inode_to_wb_begin(inode, &locked);
+ wb = unlocked_inode_to_wb_begin(inode, &cookie);
if (TestClearPageDirty(page)) {
mem_cgroup_dec_page_stat(page, MEM_CGROUP_STAT_DIRTY);
dec_node_page_state(page, NR_FILE_DIRTY);
@@ -2703,7 +2703,7 @@ int clear_page_dirty_for_io(struct page *page)
dec_wb_stat(wb, WB_RECLAIMABLE);
ret = 1;
}
- unlocked_inode_to_wb_end(inode, locked);
+ unlocked_inode_to_wb_end(inode, &cookie);
return ret;
}
return TestClearPageDirty(page);
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread