Is it correctly that the usage for spin_{lock|unlock}_irq in clear_page_dirty_for_io

Hi,  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){
...
...
                memcg = mem_cgroup_begin_page_stat(page); ----------(a) wb = unlocked_inode_to_wb_begin(inode, &locked); ---------(b) 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); -----------(c) mem_cgroup_end_page_stat(memcg); -------------(d) 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) spin_lock_irq(&inode->i_mapping->tree_lock); ------------(b) spin_unlock_irq(&inode->i_mapping->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.  But it really exists.

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

Quoted 7 lines · Wang Long

Hi, 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

Quoted 5 lines

int clear_page_dirty_for_io(struct page *page){
...
...

                memcg = mem_cgroup_begin_page_stat(page); ----------(a)
                wb = unlocked_inode_to_wb_begin(inode, &locked);

Quoted 2 lines

                if (TestClearPageDirty(page)) {
                        mem_cgroup_dec_page_stat(memcg,

Quoted 24 lines

                        dec_zone_page_state(page, NR_FILE_DIRTY);
                        dec_wb_stat(wb, WB_RECLAIMABLE);
                        ret =1;
                }
                unlocked_inode_to_wb_end(inode, locked); -----------(c)
                mem_cgroup_end_page_stat(memcg); -------------(d)
                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)

        spin_lock_irq(&inode->i_mapping->tree_lock); ------------(b)
        spin_unlock_irq(&inode->i_mapping->tree_lock); -----------(c)

spin_unlock_irqrestore(&memcg->move_lock, flags); -----------(d)

after (c) , the local irq is enabled. I think it is not correct.

Quoted 8 lines

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. But it really exists.

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

Yes, it seems we really need this even for the current tree. Please note that At least clear_page_dirty_for_io doesn't lock memcg anymore. __cancel_dirty_page still uses lock_page_memcg though (former mem_cgroup_begin_page_stat).
--
Michal Hocko
SUSE Labs

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Wang Long

Hi, 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

Quoted 5 lines

Quoted 5 lines

int clear_page_dirty_for_io(struct page *page){
...
...

                 memcg = mem_cgroup_begin_page_stat(page); ----------(a)
                 wb = unlocked_inode_to_wb_begin(inode, &locked);

---------(b)

Quoted 2 lines

Quoted 2 lines

                 if (TestClearPageDirty(page)) {
                         mem_cgroup_dec_page_stat(memcg,

MEM_CGROUP_STAT_DIRTY);

Quoted 31 lines

Quoted 31 lines

                         dec_zone_page_state(page, NR_FILE_DIRTY);
                         dec_wb_stat(wb, WB_RECLAIMABLE);
                         ret =1;
                 }
                 unlocked_inode_to_wb_end(inode, locked); -----------(c)
                 mem_cgroup_end_page_stat(memcg); -------------(d)
                 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)

         spin_lock_irq(&inode->i_mapping->tree_lock); ------------(b)
         spin_unlock_irq(&inode->i_mapping->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. But it really exists.

Quoted 8 lines

Quoted 1 line

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

Yes, it seems we really need this even for the current tree. Please note that At least clear_page_dirty_for_io doesn't lock memcg anymore. __cancel_dirty_page still uses lock_page_memcg though (former mem_cgroup_begin_page_stat).
--
Michal Hocko
SUSE Labs

I agree the issue looks real in 4.4 stable and upstream. It seems like unlocked_inode_to_wb_begin/_end should use spin_lock_irqsave/restore.

I'm testing a little patch now.

fs/fs-writeback.c +1 -1

 		congested = wb_congested(wb, cong_bits);
-		unlocked_inode_to_wb_end(inode, locked);
+		unlocked_inode_to_wb_end(inode, locked, flags);
 		return congested;
 	}

On Fri 06-04-18 01:03:24, Greg Thelen wrote:
[...]

fs/fs-writeback.c int inode_congested(struct inode *inode, int

Quoted 5 lines

fs/fs-writeback.c +0 -0

index d4d04fee568a..d51bae5a53e2 100644
@@ -746,10 +746,11 @@ int inode_congested(struct inode *inode, int

fs/fs-writeback.c int inode_congested(struct inode *inode, int cong_bits) · line 751

Quoted 7 lines

fs/fs-writeback.c +2 -1

      if (inode && inode_to_wb_is_valid(inode)) {
              struct bdi_writeback *wb;
              bool locked, congested;
+             unsigned long flags;
 
-             wb = unlocked_inode_to_wb_begin(inode, &locked);
+             wb = unlocked_inode_to_wb_begin(inode, &locked, &flags);

Wouldn't it be better to have a cookie (struct) rather than 2 parameters and let unlocked_inode_to_wb_end DTRT?

fs/fs-writeback.c int inode_congested(struct inode *inode, int cong_bits) · line 755

Quoted 5 lines

fs/fs-writeback.c +1 -1

              congested = wb_congested(wb, cong_bits);
-             unlocked_inode_to_wb_end(inode, locked);
+             unlocked_inode_to_wb_end(inode, locked, flags);
              return congested;
      }

--
Michal Hocko
SUSE Labs

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

Given the deadlock is not currently possible, it's debatable if there's any reason to modify the kernel. I suggest we should to prevent future surprises.

Reported-by Wang Long <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Change-Id Ibb773e8045852978f6207074491d262f1b3fb613

---
Changelog since v2:
- explicitly initialize wb_lock_cookie to silence compiler warnings.

Changelog since v1:
- add wb_lock_cookie to record lock context.

 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(-)

fs/fs-writeback.c +4 -3

index 1280f915079b..f4b2f6625913 100644
@@ -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;
 	}
 

include/linux/backing-dev-defs.h +5 -0

index bfe86b54f6c1..0bd432a4d7bd 100644
@@ -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
 
 /**

include/linux/backing-dev.h +16 -14

index 3e4ce54d84ab..1d744c61d996 100644
@@ -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,11 @@ 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 transaction.  IRQ 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 +366,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.

@@ -382,12 +381,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();
 }

@@ -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)
 {
 }
 

mm/page-writeback.c +9 -9

index 586f31261c83..bc38a2a7a597 100644
@@ -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 = {0};
 
-		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 = {0};
 
 		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 = {0};
 
 		/*
 		 * 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);

Signature · 1 line

2.17.0.484.g0c8726318c-goog

Quoted 3 lines

Reported-by Wang Long <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Change-Id Ibb773e8045852978f6207074491d262f1b3fb613

Not a stable material IMHO

Quoted 5 lines · On Tue, 10 Apr 2018 08:33:57 +0200 Michal Hocko <[email protected]> wrote:

Quoted 3 lines

Reported-by Wang Long <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Change-Id Ibb773e8045852978f6207074491d262f1b3fb613

Not a stable material IMHO

Why's that? Wang Long said he's observed the deadlock three times?

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

Given the deadlock is not currently possible, it's debatable if there's any reason to modify the kernel. I suggest we should to prevent future surprises.

Reported-by Wang Long <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Change-Id Ibb773e8045852978f6207074491d262f1b3fb613

---
Changelog since v2:
- explicitly initialize wb_lock_cookie to silence compiler warnings.

Changelog since v1:
- add wb_lock_cookie to record lock context.

  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(-)

fs/fs-writeback.c +4 -3

index 1280f915079b..f4b2f6625913 100644
@@ -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;
  	}
 

include/linux/backing-dev-defs.h +5 -0

index bfe86b54f6c1..0bd432a4d7bd 100644
@@ -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
 
  /**

include/linux/backing-dev.h +16 -14

index 3e4ce54d84ab..1d744c61d996 100644
@@ -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,11 @@ 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 transaction.  IRQ 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 +366,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.

@@ -382,12 +381,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();
  }

@@ -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)
  {
  }
 

mm/page-writeback.c +9 -9

index 586f31261c83..bc38a2a7a597 100644
@@ -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 = {0};
 
-		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 = {0};
 
  		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 = {0};
 
  		/*
  		 * 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);

Quoted 6 lines

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

Quoted 55 lines

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

Quoted 3 lines

Given the deadlock is not currently possible, it's debatable if there's any reason to modify the kernel. I suggest we should to prevent future surprises.

This deadlock occurs three times in our environment，

this deadlock occurs three times in our environment. It is better to cc

backport it.

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

Given the deadlock is not currently possible, it's debatable if there's any reason to modify the kernel. I suggest we should to prevent future surprises.

...

Changelog since v2:
- explicitly initialize wb_lock_cookie to silence compiler warnings.

...

include/linux/backing-dev.h +1 -1

@@ -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

Quoted 6 lines · On Mon, 9 Apr 2018 17:59:08 -0700 Greg Thelen <[email protected]> wrote:

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

Quoted 59 lines

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

Given the deadlock is not currently possible, it's debatable if there's any reason to modify the kernel. I suggest we should to prevent future surprises.

Quoted 4 lines

...

Changelog since v2:
- explicitly initialize wb_lock_cookie to silence compiler warnings.

But only in some places. What's up with that?

include/linux/backing-dev.h static inline struct bdi_writeback

Quoted 6 lines

...

include/linux/backing-dev.h +0 -0

@@ -346,7 +346,7 @@ static inline struct bdi_writeback

include/linux/backing-dev.h static inline struct bdi_writeback · line 347

Quoted 2 lines

include/linux/backing-dev.h +0 -0

 /**
  * unlocked_inode_to_wb_begin - begin unlocked inode wb access

include/linux/backing-dev.h static inline struct bdi_writeback

Quoted 7 lines

include/linux/backing-dev.h +1 -1

  * @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,11 @@ static inline struct bdi_writeback

include/linux/backing-dev.h static inline struct bdi_writeback · line 358

Quoted 5 lines

include/linux/backing-dev.h +0 -2

  * 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

include/linux/backing-dev.h static inline struct bdi_writeback · line 357

Quoted 2 lines

include/linux/backing-dev.h +1 -1

- * disabled on return.
+ * The caller must call unlocked_inode_to_wb_end() with *@cookie

include/linux/backing-dev.h static inline struct bdi_writeback · line 358

Quoted 1 line

include/linux/backing-dev.h +1 -0

+ * can't sleep during transaction.  IRQ may or may not be disabled on

include/linux/backing-dev.h static inline struct bdi_writeback · line 359

Quoted 1 line

include/linux/backing-dev.h +0 -0

  */

Grammar is a bit awkward here,

Quoted 3 lines

...

mm/page-writeback.c void account_page_redirty(struct page *page) · line 2504

Quoted 8 lines

mm/page-writeback.c +1 -1

@@ -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 = {0};

Trivia: it's better to use "= {}" here. That has the same effect and it doesn't assume that the first field is a scalar. And indeed, the first field is a bool so it should be {false}!

mm/page-writeback.c renamed from mm/page-writeback.c~writeback-safer-lock-nesting-fix +3 -3

@@ -2501,7 +2501,7 @@ void account_page_redirty(struct page *p
          if (mapping && mapping_cap_account_dirty(mapping)) {
                  struct inode *inode = mapping->host;
                  struct bdi_writeback *wb;
-               struct wb_lock_cookie cookie = {0};
+               struct wb_lock_cookie cookie = {};
 
                  wb = unlocked_inode_to_wb_begin(inode, &cookie);
                  current->nr_dirtied--;

@@ -2613,7 +2613,7 @@ void __cancel_dirty_page(struct page *pa
          if (mapping_cap_account_dirty(mapping)) {
                  struct inode *inode = mapping->host;
                  struct bdi_writeback *wb;
-               struct wb_lock_cookie cookie = {0};
+               struct wb_lock_cookie cookie = {};
 
                  lock_page_memcg(page);
                  wb = unlocked_inode_to_wb_begin(inode, &cookie);

@@ -2653,7 +2653,7 @@ int clear_page_dirty_for_io(struct page
          if (mapping && mapping_cap_account_dirty(mapping)) {
                  struct inode *inode = mapping->host;
                  struct bdi_writeback *wb;
-               struct wb_lock_cookie cookie = {0};
+               struct wb_lock_cookie cookie = {};
 
                  /*
                   * Yes, Virginia, this is indeed insane.

But I wonder about the remaining uninitialized wb_lock_cookies?

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 44.5575)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33, v4.9.93, v4.4.127.

v4.16.1: Build OK!
v4.15.16: Build OK!
v4.14.33: Build OK!
v4.9.93: Build OK!
v4.4.127: Failed to apply! Possible dependencies:

     62cccb8c8e7a ("mm: simplify lock_page_memcg()")

Please let us know if you'd like to have this patch included in a stable tree.

--
Thanks,
Sasha

Quoted 8 lines

Hi,

[This is an automated email]

This commit has been processed by the -stable helper bot and determined to be a high probability candidate for -stable trees. (score: 44.5575)

The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33,

Quoted 7 lines

v4.16.1: Build OK!
v4.15.16: Build OK!
v4.14.33: Build OK!
v4.9.93: Build OK!
v4.4.127: Failed to apply! Possible dependencies:

     62cccb8c8e7a ("mm: simplify lock_page_memcg()")

Hi Sasha,

I test the memory cgroup in lts v4.4, for this issue, 62cccb8c8e7a ("mm:
simplify lock_page_memcg()")
need to adjust and there are several other places that need to be fixed.

I will make the patch for lts v4.4 if no one did.

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

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.

[ This patch is only for 4.4 stable. Newer stable kernels should use be

cherry pick the upstream "writeback: safer lock nesting" patch. ]

Fixes 682aa8e1a6a1 ("writeback: implement unlocked_inode_to_wb

include/linux/backing-dev.h +2 -0

+static inline void unlocked_inode_to_wb_end(struct inode *inode,
+                                           struct wb_lock_cookie *cookie)
  {
  }
 

mm/page-writeback.c +7 -7

index 6d0dbde4503b..3309dbda7ffa 100644
@@ -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.

mm/page-writeback.c +1 -2

                         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.484.g0c8726318c-goog