scope GFP_NOFS api

From: Michal Hocko <[email protected]>

The current implementation of the reclaim lockup detection can lead to false positives and those even happen and usually lead to tweak the code to silence the lockdep by using GFP_NOFS even though the context can use __GFP_FS just fine. See
http://lkml.kernel.org/r/20160512080321.GA18496@dastard as an example.

=================================
[ INFO: inconsistent lock state ]
4.5.0-rc2+ #4 Tainted: G O
---------------------------------
inconsistent {RECLAIM_FS-ON-R} -> {IN-RECLAIM_FS-W} usage. kswapd0/543 [HC0[0]:SC0[0]:HE1:SE1] takes:

(&xfs_nondir_ilock_class){++++-+}, at: [<ffffffffa00781f7>] xfs_ilock+0x177/0x200 [xfs]

{RECLAIM_FS-ON-R} state was registered at:

  [<ffffffff8110f369>] mark_held_locks+0x79/0xa0
  [<ffffffff81113a43>] lockdep_trace_alloc+0xb3/0x100
  [<ffffffff81224623>] kmem_cache_alloc+0x33/0x230
  [<ffffffffa008acc1>] kmem_zone_alloc+0x81/0x120 [xfs]
  [<ffffffffa005456e>] xfs_refcountbt_init_cursor+0x3e/0xa0 [xfs]
  [<ffffffffa0053455>] __xfs_refcount_find_shared+0x75/0x580 [xfs]
  [<ffffffffa00539e4>] xfs_refcount_find_shared+0x84/0xb0 [xfs]
  [<ffffffffa005dcb8>] xfs_getbmap+0x608/0x8c0 [xfs]
  [<ffffffffa007634b>] xfs_vn_fiemap+0xab/0xc0 [xfs]
  [<ffffffff81244208>] do_vfs_ioctl+0x498/0x670
  [<ffffffff81244459>] SyS_ioctl+0x79/0x90
  [<ffffffff81847cd7>] entry_SYSCALL_64_fastpath+0x12/0x6f

       CPU0
       ----
  lock(&xfs_nondir_ilock_class);
  <Interrupt>
    lock(&xfs_nondir_ilock_class);

*** DEADLOCK ***

3 locks held by kswapd0/543:

stack backtrace:
CPU: 0 PID: 543 Comm: kswapd0 Tainted: G           O    4.5.0-rc2+ #4

Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006

ffffffff82a34f10 ffff88003aa078d0 ffffffff813a14f9 ffff88003d8551c0 ffff88003aa07920 ffffffff8110ec65 0000000000000000 0000000000000001 ffff880000000001 000000000000000b 0000000000000008 ffff88003d855aa0

Call Trace:
 [<ffffffff813a14f9>] dump_stack+0x4b/0x72
 [<ffffffff8110ec65>] print_usage_bug+0x215/0x240
 [<ffffffff8110ee85>] mark_lock+0x1f5/0x660
 [<ffffffff8110e100>] ? print_shortest_lock_dependencies+0x1a0/0x1a0
 [<ffffffff811102e0>] __lock_acquire+0xa80/0x1e50
 [<ffffffff8122474e>] ? kmem_cache_alloc+0x15e/0x230
 [<ffffffffa008acc1>] ? kmem_zone_alloc+0x81/0x120 [xfs]
 [<ffffffff811122e8>] lock_acquire+0xd8/0x1e0
 [<ffffffffa00781f7>] ? xfs_ilock+0x177/0x200 [xfs]
 [<ffffffffa0083a70>] ? xfs_reflink_cancel_cow_range+0x150/0x300 [xfs]
 [<ffffffff8110aace>] down_write_nested+0x5e/0xc0
 [<ffffffffa00781f7>] ? xfs_ilock+0x177/0x200 [xfs]
 [<ffffffffa00781f7>] xfs_ilock+0x177/0x200 [xfs]
 [<ffffffffa0083a70>] xfs_reflink_cancel_cow_range+0x150/0x300 [xfs]
 [<ffffffffa0085bdc>] xfs_fs_evict_inode+0xdc/0x1e0 [xfs]
 [<ffffffff8124d7d5>] evict+0xc5/0x190
 [<ffffffff8124d8d9>] dispose_list+0x39/0x60
 [<ffffffff8124eb2b>] prune_icache_sb+0x4b/0x60
 [<ffffffff8123317f>] super_cache_scan+0x14f/0x1a0
 [<ffffffff811e0d19>] shrink_slab.part.63.constprop.79+0x1e9/0x4e0
 [<ffffffff811e50ee>] shrink_zone+0x15e/0x170
 [<ffffffff811e5ef1>] kswapd+0x4f1/0xa80
 [<ffffffff811e5a00>] ? zone_reclaim+0x230/0x230
 [<ffffffff810e6882>] kthread+0xf2/0x110
 [<ffffffff810e6790>] ? kthread_create_on_node+0x220/0x220
 [<ffffffff8184803f>] ret_from_fork+0x3f/0x70
 [<ffffffff810e6790>] ? kthread_create_on_node+0x220/0x220

To quote Dave:
"
Ignoring whether reflink should be doing anything or not, that's a "xfs_refcountbt_init_cursor() gets called both outside and inside transactions" lockdep false positive case. The problem here is lockdep has seen this allocation from within a transaction, hence a GFP_NOFS allocation, and now it's seeing it in a GFP_KERNEL context. Also note that we have an active reference to this inode.

So, because the reclaim annotations overload the interrupt level detections and it's seen the inode ilock been taken in reclaim ("interrupt") context, this triggers a reclaim context warning where it thinks it is unsafe to do this allocation in GFP_KERNEL context holding the inode ilock...
"

This sounds like a fundamental problem of the reclaim lock detection. It is really impossible to annotate such a special usecase IMHO unless the reclaim lockup detection is reworked completely. Until then it is much better to provide a way to add "I know what I am doing flag" and mark problematic places. This would prevent from abusing GFP_NOFS flag which has a runtime effect even on configurations which have lockdep disabled.

Introduce __GFP_NOLOCKDEP flag which tells the lockdep gfp tracking to skip the current allocation request.

While we are at it also make sure that the radix tree doesn't accidentaly override tags stored in the upper part of the gfp_mask.

Suggested-by Peter Zijlstra <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

xfs has defined PF_FSTRANS to declare a scope GFP_NOFS semantic quite some time ago. We would like to make this concept more generic and use it for other filesystems as well. Let's start by giving the flag a more generic name PF_MEMALLOC_NOFS which is in line with an exiting PF_MEMALLOC_NOIO already used for the same purpose for GFP_NOIO contexts. Replace all PF_FSTRANS usage from the xfs code in the first step before we introduce a full API for it as xfs uses the flag directly anyway.

This patch doesn't introduce any functional change.

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Quoted 11 lines · Michal Hocko

From: Michal Hocko <[email protected]>

xfs has defined PF_FSTRANS to declare a scope GFP_NOFS semantic quite some time ago. We would like to make this concept more generic and use it for other filesystems as well. Let's start by giving the flag a more generic name PF_MEMALLOC_NOFS which is in line with an exiting PF_MEMALLOC_NOIO already used for the same purpose for GFP_NOIO contexts. Replace all PF_FSTRANS usage from the xfs code in the first step before we introduce a full API for it as xfs uses the flag directly anyway.

Quoted 4 lines

This patch doesn't introduce any functional change.

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Brian Foster <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

A nit:

Quoted 6 lines

include/linux/sched.h +0 -0

@@ -2320,6 +2320,8 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
 #define PF_SUSPEND_TASK 0x80000000      /* this thread called freeze_processes and should not be frozen */
 

include/linux/sched.h extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, · line 2323

Quoted 1 line

include/linux/sched.h +1 -0

+#define PF_MEMALLOC_NOFS PF_FSTRANS	/* Transition to a more generic GFP_NOFS scope semantic */

I don't see why this transition is needed, as there are already no users of PF_FSTRANS after this patch. The next patch doesn't remove any more, so this is just extra churn IMHO. But not a strong objection.

From: Michal Hocko <[email protected]>

xfs has defined PF_FSTRANS to declare a scope GFP_NOFS semantic quite some time ago. We would like to make this concept more generic and use it for other filesystems as well. Let's start by giving the flag a more generic name PF_MEMALLOC_NOFS which is in line with an exiting PF_MEMALLOC_NOIO already used for the same purpose for GFP_NOIO contexts. Replace all PF_FSTRANS usage from the xfs code in the first step before we introduce a full API for it as xfs uses the flag directly anyway.

This patch doesn't introduce any functional change.

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 fs/xfs/kmem.c             |  4 ++--
 fs/xfs/kmem.h             |  2 +-
 fs/xfs/libxfs/xfs_btree.c |  2 +-
 fs/xfs/xfs_aops.c         |  6 +++---
 fs/xfs/xfs_trans.c        | 12 ++++++------
 include/linux/sched.h     |  2 ++
 6 files changed, 15 insertions(+), 13 deletions(-)

fs/xfs/kmem.c +2 -2

index 339c696bbc01..a76a05dae96b 100644
@@ -80,13 +80,13 @@ kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 	 * context via PF_MEMALLOC_NOIO to prevent memory reclaim re-entering
 	 * the filesystem here and potentially deadlocking.
 	 */
-	if ((current->flags & PF_FSTRANS) || (flags & KM_NOFS))
+	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
 		noio_flag = memalloc_noio_save();
 
 	lflags = kmem_flags_convert(flags);
 	ptr = __vmalloc(size, lflags | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
 
-	if ((current->flags & PF_FSTRANS) || (flags & KM_NOFS))
+	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
 		memalloc_noio_restore(noio_flag);
 
 	return ptr;

fs/xfs/kmem.h +1 -1

index 689f746224e7..d973dbfc2bfa 100644
@@ -50,7 +50,7 @@ kmem_flags_convert(xfs_km_flags_t flags)
 		lflags = GFP_ATOMIC | __GFP_NOWARN;
 	} else {
 		lflags = GFP_KERNEL | __GFP_NOWARN;
-		if ((current->flags & PF_FSTRANS) || (flags & KM_NOFS))
+		if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
 			lflags &= ~__GFP_FS;
 	}
 

fs/xfs/libxfs/xfs_btree.c +1 -1

index 21e6a6ab6b9a..a2672ba4dc33 100644
@@ -2866,7 +2866,7 @@ xfs_btree_split_worker(
 	struct xfs_btree_split_args	*args = container_of(work,
 						struct xfs_btree_split_args, work);
 	unsigned long		pflags;
-	unsigned long		new_pflags = PF_FSTRANS;
+	unsigned long		new_pflags = PF_MEMALLOC_NOFS;
 
 	/*
 	 * we are in a transaction context here, but may also be doing work

fs/xfs/xfs_aops.c +3 -3

index ef382bfb402b..d4094bb55033 100644
@@ -189,7 +189,7 @@ xfs_setfilesize_trans_alloc(
 	 * We hand off the transaction to the completion thread now, so
 	 * clear the flag here.
 	 */
-	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 	return 0;
 }
 

@@ -252,7 +252,7 @@ xfs_setfilesize_ioend(
 	 * thus we need to mark ourselves as being in a transaction manually.
 	 * Similarly for freeze protection.
 	 */
-	current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 	__sb_writers_acquired(VFS_I(ip)->i_sb, SB_FREEZE_FS);
 
 	/* we abort the update if there was an IO error */

@@ -1015,7 +1015,7 @@ xfs_do_writepage(
 	 * Given that we do not allow direct reclaim to call us, we should
 	 * never be called while in a filesystem transaction.
 	 */
-	if (WARN_ON_ONCE(current->flags & PF_FSTRANS))
+	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS))
 		goto redirty;
 
 	/*

fs/xfs/xfs_trans.c +6 -6

index 70f42ea86dfb..f5969c8274fc 100644
@@ -134,7 +134,7 @@ xfs_trans_reserve(
 	bool		rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
 
 	/* Mark this thread as being in a transaction */
-	current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_set_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 
 	/*
 	 * Attempt to reserve the needed disk blocks by decrementing

@@ -144,7 +144,7 @@ xfs_trans_reserve(
 	if (blocks > 0) {
 		error = xfs_mod_fdblocks(tp->t_mountp, -((int64_t)blocks), rsvd);
 		if (error != 0) {
-			current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+			current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 			return -ENOSPC;
 		}
 		tp->t_blk_res += blocks;

@@ -221,7 +221,7 @@ xfs_trans_reserve(
 		tp->t_blk_res = 0;
 	}
 
-	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 
 	return error;
 }

@@ -914,7 +914,7 @@ __xfs_trans_commit(
 
 	xfs_log_commit_cil(mp, tp, &commit_lsn, regrant);
 
-	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 	xfs_trans_free(tp);
 
 	/*

@@ -944,7 +944,7 @@ __xfs_trans_commit(
 		if (commit_lsn == -1 && !error)
 			error = -EIO;
 	}
-	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 	xfs_trans_free_items(tp, NULLCOMMITLSN, !!error);
 	xfs_trans_free(tp);
 

@@ -998,7 +998,7 @@ xfs_trans_cancel(
 		xfs_log_done(mp, tp->t_ticket, NULL, false);
 
 	/* mark this thread as no longer being in a transaction */
-	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	current_restore_flags_nested(&tp->t_pflags, PF_MEMALLOC_NOFS);
 
 	xfs_trans_free_items(tp, NULLCOMMITLSN, dirty);
 	xfs_trans_free(tp);

include/linux/sched.h +2 -0

index 1531c48f56e2..abeb84604d32 100644
@@ -2320,6 +2320,8 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
 #define PF_SUSPEND_TASK 0x80000000      /* this thread called freeze_processes and should not be frozen */
 
+#define PF_MEMALLOC_NOFS PF_FSTRANS	/* Transition to a more generic GFP_NOFS scope semantic */
+
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
  * tasks can access tsk->flags in readonly mode for example

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

GFP_NOFS context is used for the following 5 reasons currently
- to prevent from deadlocks when the lock held by the allocation context would be needed during the memory reclaim
- to prevent from stack overflows during the reclaim because the allocation is performed from a deep context already
- to prevent lockups when the allocation context depends on other reclaimers to make a forward progress indirectly
- just in case because this would be safe from the fs POV
- silence lockdep false positives

Unfortunately overuse of this allocation context brings some problems to the MM. Memory reclaim is much weaker (especially during heavy FS metadata workloads), OOM killer cannot be invoked because the MM layer doesn't have enough information about how much memory is freeable by the FS layer.

In many cases it is far from clear why the weaker context is even used and so it might be used unnecessarily. We would like to get rid of those as much as possible. One way to do that is to use the flag in scopes rather than isolated cases. Such a scope is declared when really necessary, tracked per task and all the allocation requests from within the context will simply inherit the GFP_NOFS semantic.

Not only this is easier to understand and maintain because there are much less problematic contexts than specific allocation requests, this also helps code paths where FS layer interacts with other layers (e.g. crypto, security modules, MM etc...) and there is no easy way to convey the allocation context between the layers.

Introduce memalloc_nofs_{save,restore} API to control the scope of GFP_NOFS allocation context. This is basically copying memalloc_noio_{save,restore} API we have for other restricted allocation context GFP_NOIO. The PF_MEMALLOC_NOFS flag already exists and it is just an alias for PF_FSTRANS which has been xfs specific until recently. There are no more PF_FSTRANS users anymore so let's just drop it.

PF_MEMALLOC_NOFS is now checked in the MM layer and drops __GFP_FS implicitly same as PF_MEMALLOC_NOIO drops __GFP_IO. memalloc_noio_flags is renamed to current_gfp_context because it now cares about both PF_MEMALLOC_NOFS and PF_MEMALLOC_NOIO contexts. Xfs code paths preserve their semantic. kmem_flags_convert() doesn't need to evaluate the flag anymore.

This patch shouldn't introduce any functional changes.

Let's hope that filesystems will drop direct GFP_NOFS (resp. ~__GFP_FS) usage as much as possible and only use a properly documented memalloc_nofs_{save,restore} checkpoints where they are appropriate.

Signed-off-by Michal Hocko <[email protected]>

include/linux/sched.h +8 -0

+	return flags;
+}
+
+static inline void memalloc_nofs_restore(unsigned int flags)
+{
+	current->flags = (current->flags & ~PF_MEMALLOC_NOFS) | flags;
+}
+
 /* Per-process atomic flags. */
 #define PFA_NO_NEW_PRIVS 0	/* May not gain new privileges. */
 #define PFA_SPREAD_PAGE  1      /* Spread page cache over cpuset */

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline unsigned int memalloc_nofs_save(void)
+{
+	unsigned int flags = current->flags & PF_MEMALLOC_NOFS;
+	current->flags |= PF_MEMALLOC_NOFS;

So this is not new, as same goes for memalloc_noio_save, but I've noticed that e.g. exit_signal() does tsk->flags |= PF_EXITING; So is it possible that there's a r-m-w hazard here?

Review unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, · line 3032

Quoted 6 lines

(quoted diff) +1 -1

@@ -3029,7 +3029,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 	int nid;
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask & GFP_RECLAIM_MASK) |
+		.gfp_mask = (current_gfp_context(gfp_mask) & GFP_RECLAIM_MASK) |

So this function didn't do memalloc_noio_flags() before? Is it a bug that should be fixed separately or at least mentioned? Because that looks like a functional change...

On Mon 09-01-17 14:04:21, Vlastimil Babka wrote:

Now that you have opened this I have noticed that the code is wrong here because GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK would overwrite the removed GFP_FS.

I guess it would be better and less error prone
to move the current_gfp_context part into the direct reclaim entry - do_try_to_free_pages - and put the comment like this

---

mm/vmscan.c +10 -4

index 4ea6b610f20e..df7975185f11 100644
@@ -2756,6 +2756,13 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 	int initial_priority = sc->priority;
 	unsigned long total_scanned = 0;
 	unsigned long writeback_threshold;
+
+	/*
+	 * Make sure that the gfp context properly handles scope gfp mask.
+	 * This might weaken the reclaim context (e.g. make it GFP_NOFS or
+	 * GFP_NOIO).
+	 */
+	sc->gfp_mask = current_gfp_context(sc->gfp_mask);
 retry:
 	delayacct_freepages_start();
 

@@ -2949,7 +2956,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	unsigned long nr_reclaimed;
 	struct scan_control sc = {
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = gfp_mask,
 		.reclaim_idx = gfp_zone(gfp_mask),
 		.order = order,
 		.nodemask = nodemask,

@@ -3029,8 +3036,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 	int nid;
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (current_gfp_context(gfp_mask) & GFP_RECLAIM_MASK) |
-				(GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK),
+		.gfp_mask = GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK,
 		.reclaim_idx = MAX_NR_ZONES - 1,
 		.target_mem_cgroup = memcg,
 		.priority = DEF_PRIORITY,

@@ -3723,7 +3729,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	int classzone_idx = gfp_zone(gfp_mask);
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = gfp_mask,
 		.order = order,
 		.priority = NODE_RECLAIM_PRIORITY,
 		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),

Signature · 2 lines

Michal Hocko
SUSE Labs

On Mon 09-01-17 14:04:21, Vlastimil Babka wrote:
[...]

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline unsigned int memalloc_nofs_save(void)
+{
+	unsigned int flags = current->flags & PF_MEMALLOC_NOFS;
+	current->flags |= PF_MEMALLOC_NOFS;

So this is not new, as same goes for memalloc_noio_save, but I've noticed that e.g. exit_signal() does tsk->flags |= PF_EXITING; So is it possible that there's a r-m-w hazard here?

exit_signals operates on current and all task_struct::flags should be used only on the current.
[...]

Quoted 11 lines

mm/vmscan.c unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg, · line 3032

Quoted 7 lines

mm/vmscan.c +1 -1

@@ -3029,7 +3029,7 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
 	int nid;
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask & GFP_RECLAIM_MASK) |
+		.gfp_mask = (current_gfp_context(gfp_mask) & GFP_RECLAIM_MASK) |

So this function didn't do memalloc_noio_flags() before? Is it a bug that should be fixed separately or at least mentioned? Because that looks like a functional change...

We didn't need it. Kmem charges are opt-in and current all of them support GFP_IO. The LRU pages are not charged in NOIO context either. We need it now because there will be callers to charge GFP_KERNEL while being inside the NOFS scope.

Now that you have opened this I have noticed that the code is wrong here because GFP_HIGHUSER_MOVABLE & ~GFP_RECLAIM_MASK would overwrite the removed GFP_FS. I guess it would be better and less error prone to move the current_gfp_context part into the direct reclaim entry - do_try_to_free_pages - and put the comment like this

From: Michal Hocko <[email protected]>

kmem_zalloc_large and _xfs_buf_map_pages use memalloc_noio_{save,restore} API to prevent from reclaim recursion into the fs because vmalloc can invoke unconditional GFP_KERNEL allocations and these functions might be called from the NOFS contexts. The memalloc_noio_save will enforce GFP_NOIO context which is even weaker than GFP_NOFS and that seems to be unnecessary. Let's use memalloc_nofs_{save,restore} instead as it should provide exactly what we need here - implicit GFP_NOFS context.

Changes since v1
- s@memalloc_noio_restore@memalloc_nofs_restore@ in _xfs_buf_map_pages as per Brian Foster

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c    | 10 +++++-----
 fs/xfs/xfs_buf.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

fs/xfs/kmem.c +1 -1

index a76a05dae96b..d69ed5e76621 100644
@@ -65,7 +65,7 @@ kmem_alloc(size_t size, xfs_km_flags_t flags)
 void *
 kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 {
-	unsigned noio_flag = 0;
+	unsigned nofs_flag = 0;
 	void	*ptr;
 	gfp_t	lflags;
 

fs/xfs/kmem.c +4 -4

 	 */
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		noio_flag = memalloc_noio_save();
+	if (flags & KM_NOFS)
+		nofs_flag = memalloc_nofs_save();
 
 	lflags = kmem_flags_convert(flags);
 	ptr = __vmalloc(size, lflags | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
 
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		memalloc_noio_restore(noio_flag);
+	if (flags & KM_NOFS)
+		memalloc_nofs_restore(nofs_flag);
 
 	return ptr;
 }

fs/xfs/xfs_buf.c +3 -3

index 7f0a01f7b592..8cb8dd4cdfd8 100644
@@ -441,17 +441,17 @@ _xfs_buf_map_pages(
 		bp->b_addr = NULL;
 	} else {
 		int retried = 0;
-		unsigned noio_flag;
+		unsigned nofs_flag;
 
 		/*
 		 * vm_map_ram() will allocate auxillary structures (e.g.
 		 * pagetables) with GFP_KERNEL, yet we are likely to be under
 		 * GFP_NOFS context here. Hence we need to tell memory reclaim
-		 * that we are in such a context via PF_MEMALLOC_NOIO to prevent
+		 * that we are in such a context via PF_MEMALLOC_NOFS to prevent
 		 * memory reclaim re-entering the filesystem here and
 		 * potentially deadlocking.
 		 */
-		noio_flag = memalloc_noio_save();
+		nofs_flag = memalloc_nofs_save();
 		do {
 			bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count,
 						-1, PAGE_KERNEL);

Quoted 14 lines · Michal Hocko

From: Michal Hocko <[email protected]>

kmem_zalloc_large and _xfs_buf_map_pages use memalloc_noio_{save,restore} API to prevent from reclaim recursion into the fs because vmalloc can invoke unconditional GFP_KERNEL allocations and these functions might be called from the NOFS contexts. The memalloc_noio_save will enforce GFP_NOIO context which is even weaker than GFP_NOFS and that seems to be unnecessary. Let's use memalloc_nofs_{save,restore} instead as it should provide exactly what we need here - implicit GFP_NOFS context.

Changes since v1
- s@memalloc_noio_restore@memalloc_nofs_restore@ in _xfs_buf_map_pages as per Brian Foster

Quoted 1 line

Signed-off-by Michal Hocko <[email protected]>

Not a xfs expert, but seems correct.

Acked-by Vlastimil Babka <[email protected]>

Nit below:

Quoted 18 lines

---
 fs/xfs/kmem.c    | 10 +++++-----
 fs/xfs/xfs_buf.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

fs/xfs/kmem.c +1 -1

index a76a05dae96b..d69ed5e76621 100644
@@ -65,7 +65,7 @@ kmem_alloc(size_t size, xfs_km_flags_t flags)
 void *
 kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 {
-	unsigned noio_flag = 0;
+	unsigned nofs_flag = 0;
 	void	*ptr;
 	gfp_t	lflags;
 

fs/xfs/kmem.c kmem_zalloc_large(size_t size, xfs_km_flags_t flags) · line 81

Quoted 3 lines

fs/xfs/kmem.c +0 -0

@@ -80,14 +80,14 @@ kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 	 * context via PF_MEMALLOC_NOIO to prevent memory reclaim re-entering
 	 * the filesystem here and potentially deadlocking.

The comment above is now largely obsolete, or minimally should be changed to PF_MEMALLOC_NOFS?

From: Michal Hocko <[email protected]>

kmem_zalloc_large and _xfs_buf_map_pages use memalloc_noio_{save,restore} API to prevent from reclaim recursion into the fs because vmalloc can invoke unconditional GFP_KERNEL allocations and these functions might be called from the NOFS contexts. The memalloc_noio_save will enforce GFP_NOIO context which is even weaker than GFP_NOFS and that seems to be unnecessary. Let's use memalloc_nofs_{save,restore} instead as it should provide exactly what we need here - implicit GFP_NOFS context.

Changes since v1
- s@memalloc_noio_restore@memalloc_nofs_restore@ in _xfs_buf_map_pages as per Brian Foster

Signed-off-by Michal Hocko <[email protected]>

---

 fs/xfs/kmem.c    | 10 +++++-----
 fs/xfs/xfs_buf.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

fs/xfs/kmem.c +5 -5

index a76a05dae96b..d69ed5e76621 100644
@@ -65,7 +65,7 @@ kmem_alloc(size_t size, xfs_km_flags_t flags)
 void *
 kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 {
-	unsigned noio_flag = 0;
+	unsigned nofs_flag = 0;
 	void	*ptr;
 	gfp_t	lflags;
 

@@ -80,14 +80,14 @@ kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 	 * context via PF_MEMALLOC_NOIO to prevent memory reclaim re-entering
 	 * the filesystem here and potentially deadlocking.
 	 */
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		noio_flag = memalloc_noio_save();
+	if (flags & KM_NOFS)
+		nofs_flag = memalloc_nofs_save();
 
 	lflags = kmem_flags_convert(flags);
 	ptr = __vmalloc(size, lflags | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
 
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		memalloc_noio_restore(noio_flag);
+	if (flags & KM_NOFS)
+		memalloc_nofs_restore(nofs_flag);
 
 	return ptr;
 }

fs/xfs/xfs_buf.c +4 -4

index 7f0a01f7b592..8cb8dd4cdfd8 100644
@@ -441,17 +441,17 @@ _xfs_buf_map_pages(
 		bp->b_addr = NULL;
 	} else {
 		int retried = 0;
-		unsigned noio_flag;
+		unsigned nofs_flag;
 
 		/*
 		 * vm_map_ram() will allocate auxillary structures (e.g.
 		 * pagetables) with GFP_KERNEL, yet we are likely to be under
 		 * GFP_NOFS context here. Hence we need to tell memory reclaim
-		 * that we are in such a context via PF_MEMALLOC_NOIO to prevent
+		 * that we are in such a context via PF_MEMALLOC_NOFS to prevent
 		 * memory reclaim re-entering the filesystem here and
 		 * potentially deadlocking.
 		 */
-		noio_flag = memalloc_noio_save();
+		nofs_flag = memalloc_nofs_save();
 		do {
 			bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count,
 						-1, PAGE_KERNEL);

@@ -459,7 +459,7 @@ _xfs_buf_map_pages(
 				break;
 			vm_unmap_aliases();
 		} while (retried++ <= 1);
-		memalloc_noio_restore(noio_flag);
+		memalloc_nofs_restore(nofs_flag);
 
 		if (!bp->b_addr)
 			return -ENOMEM;

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

kmem_zalloc_large and _xfs_buf_map_pages use memalloc_noio_{save,restore} API to prevent from reclaim recursion into the fs because vmalloc can invoke unconditional GFP_KERNEL allocations and these functions might be called from the NOFS contexts. The memalloc_noio_save will enforce GFP_NOIO context which is even weaker than GFP_NOFS and that seems to be unnecessary. Let's use memalloc_nofs_{save,restore} instead as it should provide exactly what we need here - implicit GFP_NOFS context.

Changes since v1
- s@memalloc_noio_restore@memalloc_nofs_restore@ in _xfs_buf_map_pages as per Brian Foster

Signed-off-by Michal Hocko <[email protected]>

---
 fs/xfs/kmem.c    | 10 +++++-----
 fs/xfs/xfs_buf.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

fs/xfs/kmem.c +5 -5

index a76a05dae96b..d69ed5e76621 100644
@@ -65,7 +65,7 @@ kmem_alloc(size_t size, xfs_km_flags_t flags)
 void *
 kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 {
-	unsigned noio_flag = 0;
+	unsigned nofs_flag = 0;
 	void	*ptr;
 	gfp_t	lflags;
 

@@ -80,14 +80,14 @@ kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
 	 * context via PF_MEMALLOC_NOIO to prevent memory reclaim re-entering
 	 * the filesystem here and potentially deadlocking.
 	 */
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		noio_flag = memalloc_noio_save();
+	if (flags & KM_NOFS)
+		nofs_flag = memalloc_nofs_save();
 
 	lflags = kmem_flags_convert(flags);
 	ptr = __vmalloc(size, lflags | __GFP_HIGHMEM | __GFP_ZERO, PAGE_KERNEL);
 
-	if ((current->flags & PF_MEMALLOC_NOFS) || (flags & KM_NOFS))
-		memalloc_noio_restore(noio_flag);
+	if (flags & KM_NOFS)
+		memalloc_nofs_restore(nofs_flag);
 
 	return ptr;
 }

fs/xfs/xfs_buf.c +4 -4

index 7f0a01f7b592..8cb8dd4cdfd8 100644
@@ -441,17 +441,17 @@ _xfs_buf_map_pages(
 		bp->b_addr = NULL;
 	} else {
 		int retried = 0;
-		unsigned noio_flag;
+		unsigned nofs_flag;
 
 		/*
 		 * vm_map_ram() will allocate auxillary structures (e.g.
 		 * pagetables) with GFP_KERNEL, yet we are likely to be under
 		 * GFP_NOFS context here. Hence we need to tell memory reclaim
-		 * that we are in such a context via PF_MEMALLOC_NOIO to prevent
+		 * that we are in such a context via PF_MEMALLOC_NOFS to prevent
 		 * memory reclaim re-entering the filesystem here and
 		 * potentially deadlocking.
 		 */
-		noio_flag = memalloc_noio_save();
+		nofs_flag = memalloc_nofs_save();
 		do {
 			bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count,
 						-1, PAGE_KERNEL);

@@ -459,7 +459,7 @@ _xfs_buf_map_pages(
 				break;
 			vm_unmap_aliases();
 		} while (retried++ <= 1);
-		memalloc_noio_restore(noio_flag);
+		memalloc_nofs_restore(nofs_flag);
 
 		if (!bp->b_addr)
 			return -ENOMEM;

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Michal Hocko <[email protected]>

This reverts commit c45653c341f5c8a0ce19c8f0ad4678640849cb86 because sb_getblk_gfp is not really needed as
sb_getblk
__getblk_gfp

    __getblk_slow
      grow_buffers
        grow_dev_page
	  gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp

so __GFP_FS is cleared unconditionally and therefore the above commit didn't have any real effect in fact.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Quoted 18 lines · Michal Hocko

From: Michal Hocko <[email protected]>

This reverts commit c45653c341f5c8a0ce19c8f0ad4678640849cb86 because sb_getblk_gfp is not really needed as
sb_getblk
__getblk_gfp

    __getblk_slow
      grow_buffers
        grow_dev_page
	  gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp

so __GFP_FS is cleared unconditionally and therefore the above commit didn't have any real effect in fact.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Quoted 2 lines

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

If I'm not mistaken, this patch is not dependent on any of the other patches in this series (and the other patches are not dependent on this one). Hence, I could take this patch via the ext4 tree, correct?

Quoted 19 lines · Theodore Ts'o

Quoted 18 lines · Michal Hocko

From: Michal Hocko <[email protected]>

This reverts commit c45653c341f5c8a0ce19c8f0ad4678640849cb86 because sb_getblk_gfp is not really needed as
sb_getblk
__getblk_gfp

    __getblk_slow
      grow_buffers
        grow_dev_page
	  gfp_mask = mapping_gfp_constraint(inode->i_mapping, ~__GFP_FS) | gfp

so __GFP_FS is cleared unconditionally and therefore the above commit didn't have any real effect in fact.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

If I'm not mistaken, this patch is not dependent on any of the other patches in this series (and the other patches are not dependent on this one). Hence, I could take this patch via the ext4 tree, correct?

Yes, that is correct

From: Michal Hocko <[email protected]>

This reverts commit 216553c4b7f3e3e2beb4981cddca9b2027523928. Now that the transaction context uses memalloc_nofs_save and all allocations within the this context inherit GFP_NOFS automatically, there is no reason to mark specific allocations explicitly.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Quoted 11 lines · Michal Hocko

From: Michal Hocko <[email protected]>

This reverts commit 216553c4b7f3e3e2beb4981cddca9b2027523928. Now that the transaction context uses memalloc_nofs_save and all allocations within the this context inherit GFP_NOFS automatically, there is no reason to mark specific allocations explicitly.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Quoted 2 lines

Signed-off-by Michal Hocko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Changes in the jbd2 layer aren't going to guarantee that memalloc_nofs_save() will be executed if we are running ext4 without a journal (aka in no journal mode). And this is a *very* common configuration; it's how ext4 is used inside Google in our production servers.

So that means the earlier patches will probably need to be changed so the nOFS scope is done in the ext4_journal_{start,stop} functions in fs/ext4/ext4_jbd2.c.

Quoted 12 lines · Theodore Ts'o

Quoted 11 lines · Michal Hocko

From: Michal Hocko <mhocko-IBi9RG/[email protected]>

This reverts commit 216553c4b7f3e3e2beb4981cddca9b2027523928. Now that the transaction context uses memalloc_nofs_save and all allocations within the this context inherit GFP_NOFS automatically, there is no reason to mark specific allocations explicitly.

This patch should not introduce any functional change. The main point of this change is to reduce explicit GFP_NOFS usage inside ext4 code to make the review of the remaining usage easier.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Michal Hocko <mhocko-IBi9RG/[email protected]>

Reviewed-by Jan Kara <[email protected]>

Changes in the jbd2 layer aren't going to guarantee that memalloc_nofs_save() will be executed if we are running ext4 without a journal (aka in no journal mode). And this is a *very* common configuration; it's how ext4 is used inside Google in our production servers.

OK, I wasn't aware of that.

Quoted 3 lines

So that means the earlier patches will probably need to be changed so the nOFS scope is done in the ext4_journal_{start,stop} functions in fs/ext4/ext4_jbd2.c.

I could definitely appreciated some help here. The call paths are rather complex and I am not familiar with the code enough. On of the biggest problem I have currently is that there doesn't seem to be an easy place to store the old allocation context.

OK, so I've been staring into the code and AFAIU current->journal_info can contain my stored information. I could either hijack part of the word as the ref counting is only consuming low 12b. But that looks too ugly to live. Or I can allocate some placeholder.

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 5 lines · Michal Hocko

OK, so I've been staring into the code and AFAIU current->journal_info can contain my stored information. I could either hijack part of the word as the ref counting is only consuming low 12b. But that looks too ugly to live. Or I can allocate some placeholder.

Yeah, I was looking at something similar. Can you guarantee that the context will only take one or two bits? (Looks like it only needs one bit ATM, even though at the moment you're storing the whole GFP mask, correct?)

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

Certainly not for the same inode, and while we could make it work if the mm was writing back another inode, or another superblock, there are also stack depth considerations that would make this be a bad idea. So we do need to be able to assert GFP_NOFS even in no journal mode, and for any file system including ext2, for that matter.

Because of the fact that we're going to have to play games with current->journal_info, maybe this is something that I should take responsibility for, and to go through the the ext4 tree after the main patch series go through?

Maybe you could use xfs and ext2 as sample
(simple) implementations?

My only ask is that the memalloc nofs context be a well defined N bits, where N < 16, and I'll find some place to put them (probably journal_info).

Quoted 5 lines

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 3 lines

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

But we do not enter the writeback path from the direct reclaim. Or do you mean something other than pageout()'s mapping->a_ops->writepage? There is only try_to_release_page where we get back to the filesystems but I do not see any NOFS protection in ext4_releasepage.

Quoted 5 lines · Michal Hocko

Quoted 5 lines

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 8 lines

Quoted 3 lines

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

But we do not enter the writeback path from the direct reclaim. Or do you mean something other than pageout()'s mapping->a_ops->writepage? There is only try_to_release_page where we get back to the filesystems but I do not see any NOFS protection in ext4_releasepage.

Maybe to expand a bit: These days, direct reclaim can call ->releasepage() callback, ->evict_inode() callback (and only for inodes with i_nlink > 0), shrinkers. That's it. So the recursion possibilities are rather more limited than they used to be several years ago and we likely do not need as much GFP_NOFS protection as we used to.

Quoted 15 lines · Jan Kara

Quoted 5 lines · Michal Hocko

Quoted 5 lines

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 8 lines

Quoted 3 lines

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

But we do not enter the writeback path from the direct reclaim. Or do you mean something other than pageout()'s mapping->a_ops->writepage? There is only try_to_release_page where we get back to the filesystems but I do not see any NOFS protection in ext4_releasepage.

Quoted 5 lines

Maybe to expand a bit: These days, direct reclaim can call ->releasepage() callback, ->evict_inode() callback (and only for inodes with i_nlink > 0), shrinkers. That's it. So the recursion possibilities are rather more limited than they used to be several years ago and we likely do not need as much GFP_NOFS protection as we used to.

Thanks for making my remark more clear Jack! I would just want to add that I was playing with the patch below (it is basically GFP_NOFS->GFP_KERNEL for all allocations which trigger warning from the debugging patch which means they are called from within transaction) and it didn't hit the lockdep when running xfstests both with or without the enabled journal.

So am I still missing something or the nojournal mode is safe and the current series is OK wrt. ext*?

The following patch in its current form is WIP and needs a proper review before I post it.

---
 fs/ext4/inode.c       |  4 ++--
 fs/ext4/mballoc.c     | 14 +++++++-------
 fs/ext4/xattr.c       |  2 +-
 fs/jbd2/journal.c     |  4 ++--
 fs/jbd2/revoke.c      |  2 +-
 fs/jbd2/transaction.c |  2 +-
 6 files changed, 14 insertions(+), 14 deletions(-)

fs/ext4/inode.c +2 -2

index b7d141c3b810..841cb8c4cb5e 100644
@@ -2085,7 +2085,7 @@ static int ext4_writepage(struct page *page,
 		return __ext4_journalled_writepage(page, len);
 
 	ext4_io_submit_init(&io_submit, wbc);
-	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
+	io_submit.io_end = ext4_init_io_end(inode, GFP_KERNEL);
 	if (!io_submit.io_end) {
 		redirty_page_for_writepage(wbc, page);
 		unlock_page(page);

@@ -3794,7 +3794,7 @@ static int __ext4_block_zero_page_range(handle_t *handle,
 	int err = 0;
 
 	page = find_or_create_page(mapping, from >> PAGE_SHIFT,
-				   mapping_gfp_constraint(mapping, ~__GFP_FS));
+				   mapping_gfp_mask(mapping));
 	if (!page)
 		return -ENOMEM;
 

fs/ext4/mballoc.c +7 -7

index d9fd184b049e..67b97cd6e3d6 100644
@@ -1251,7 +1251,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 static int ext4_mb_load_buddy(struct super_block *sb, ext4_group_t group,
 			      struct ext4_buddy *e4b)
 {
-	return ext4_mb_load_buddy_gfp(sb, group, e4b, GFP_NOFS);
+	return ext4_mb_load_buddy_gfp(sb, group, e4b, GFP_KERNEL);
 }
 
 static void ext4_mb_unload_buddy(struct ext4_buddy *e4b)

@@ -2054,7 +2054,7 @@ static int ext4_mb_good_group(struct ext4_allocation_context *ac,
 
 	/* We only do this if the grp has never been initialized */
 	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
-		int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS);
+		int ret = ext4_mb_init_group(ac->ac_sb, group, GFP_KERNEL);
 		if (ret)
 			return ret;
 	}

@@ -3600,7 +3600,7 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac)
 	BUG_ON(ac->ac_status != AC_STATUS_FOUND);
 	BUG_ON(!S_ISREG(ac->ac_inode->i_mode));
 
-	pa = kmem_cache_alloc(ext4_pspace_cachep, GFP_NOFS);
+	pa = kmem_cache_alloc(ext4_pspace_cachep, GFP_KERNEL);
 	if (pa == NULL)
 		return -ENOMEM;
 

@@ -3694,7 +3694,7 @@ ext4_mb_new_group_pa(struct ext4_allocation_context *ac)
 	BUG_ON(!S_ISREG(ac->ac_inode->i_mode));
 
 	BUG_ON(ext4_pspace_cachep == NULL);
-	pa = kmem_cache_alloc(ext4_pspace_cachep, GFP_NOFS);
+	pa = kmem_cache_alloc(ext4_pspace_cachep, GFP_KERNEL);
 	if (pa == NULL)
 		return -ENOMEM;
 

@@ -4479,7 +4479,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
 		}
 	}
 
-	ac = kmem_cache_zalloc(ext4_ac_cachep, GFP_NOFS);
+	ac = kmem_cache_zalloc(ext4_ac_cachep, GFP_KERNEL);
 	if (!ac) {
 		ar->len = 0;
 		*errp = -ENOMEM;

@@ -4813,7 +4813,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 
 	/* __GFP_NOFAIL: retry infinitely, ignore TIF_MEMDIE and memcg limit. */
 	err = ext4_mb_load_buddy_gfp(sb, block_group, &e4b,
-				     GFP_NOFS|__GFP_NOFAIL);
+				     GFP_KERNEL|__GFP_NOFAIL);
 	if (err)
 		goto error_return;
 

@@ -4832,7 +4832,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 		 * to fail.
 		 */
 		new_entry = kmem_cache_alloc(ext4_free_data_cachep,
-				GFP_NOFS|__GFP_NOFAIL);
+				GFP_KERNEL|__GFP_NOFAIL);
 		new_entry->efd_start_cluster = bit;
 		new_entry->efd_group = block_group;
 		new_entry->efd_count = count_clusters;

fs/ext4/xattr.c +1 -1

index 172317462238..f68e8c87f9f2 100644
@@ -1650,7 +1650,7 @@ ext4_xattr_cache_insert(struct mb_cache *ext4_mb_cache, struct buffer_head *bh)
 		       EXT4_XATTR_REFCOUNT_MAX;
 	int error;
 
-	error = mb_cache_entry_create(ext4_mb_cache, GFP_NOFS, hash,
+	error = mb_cache_entry_create(ext4_mb_cache, GFP_KERNEL, hash,
 				      bh->b_blocknr, reusable);
 	if (error) {
 		if (error == -EBUSY)

fs/jbd2/journal.c +2 -2

index 3a449150f834..bd29daa975a5 100644
@@ -379,7 +379,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	 */
 	J_ASSERT_BH(bh_in, buffer_jbddirty(bh_in));
 
-	new_bh = alloc_buffer_head(GFP_NOFS|__GFP_NOFAIL);
+	new_bh = alloc_buffer_head(GFP_KERNEL|__GFP_NOFAIL);
 
 	/* keep subsequent assertions sane */
 	atomic_set(&new_bh->b_count, 1);

@@ -2375,7 +2375,7 @@ static struct journal_head *journal_alloc_journal_head(void)
 #ifdef CONFIG_JBD2_DEBUG
 	atomic_inc(&nr_journal_heads);
 #endif
-	ret = kmem_cache_zalloc(jbd2_journal_head_cache, GFP_NOFS);
+	ret = kmem_cache_zalloc(jbd2_journal_head_cache, GFP_KERNEL);
 	if (!ret) {
 		jbd_debug(1, "out of memory for journal_head\n");
 		pr_notice_ratelimited("ENOMEM in %s, retrying.\n", __func__);

fs/jbd2/revoke.c +1 -1

index cfc38b552118..c9c347468c5b 100644
@@ -141,7 +141,7 @@ static int insert_revoke_hash(journal_t *journal, unsigned long long blocknr,
 {
 	struct list_head *hash_list;
 	struct jbd2_revoke_record_s *record;
-	gfp_t gfp_mask = GFP_NOFS;
+	gfp_t gfp_mask = GFP_KERNEL;
 
 	if (journal_oom_retry)
 		gfp_mask |= __GFP_NOFAIL;

fs/jbd2/transaction.c +1 -1

index 35a5d3d76182..a7e50eb330a8 100644
@@ -974,7 +974,7 @@ do_get_write_access(handle_t *handle, struct journal_head *jh,
 			JBUFFER_TRACE(jh, "allocate memory for buffer");
 			jbd_unlock_bh_state(bh);
 			frozen_buffer = jbd2_alloc(jh2bh(jh)->b_size,
-						   GFP_NOFS | __GFP_NOFAIL);
+						   GFP_KERNEL | __GFP_NOFAIL);
 			goto repeat;
 		}
 		jh->b_frozen_data = frozen_buffer;

Signature · 5 lines

2.11.0

-- 
Michal Hocko
SUSE Labs

Quoted 29 lines · Michal Hocko

Quoted 15 lines · Jan Kara

Quoted 5 lines · Michal Hocko

Quoted 5 lines

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 8 lines

Quoted 3 lines

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

But we do not enter the writeback path from the direct reclaim. Or do you mean something other than pageout()'s mapping->a_ops->writepage? There is only try_to_release_page where we get back to the filesystems but I do not see any NOFS protection in ext4_releasepage.

Quoted 5 lines

Maybe to expand a bit: These days, direct reclaim can call ->releasepage() callback, ->evict_inode() callback (and only for inodes with i_nlink > 0), shrinkers. That's it. So the recursion possibilities are rather more limited than they used to be several years ago and we likely do not need as much GFP_NOFS protection as we used to.

Thanks for making my remark more clear Jack! I would just want to add that I was playing with the patch below (it is basically GFP_NOFS->GFP_KERNEL for all allocations which trigger warning from the debugging patch which means they are called from within transaction) and it didn't hit the lockdep when running xfstests both with or without the enabled journal.

Quoted 2 lines

So am I still missing something or the nojournal mode is safe and the current series is OK wrt. ext*?

I'm convinced the current series is OK, only real life will tell us whether we missed something or not ;)

Quoted 2 lines

The following patch in its current form is WIP and needs a proper review before I post it.

So jbd2 changes look confusing (although technically correct) to me - we *always* should run in NOFS context in those place so having GFP_KERNEL there looks like it is unnecessarily hiding what is going on. So in those places I'd prefer to keep GFP_NOFS or somehow else make it very clear these allocations are expected to be GFP_NOFS (and assert that). Otherwise the changes look good to me.

Quoted 30 lines · Jan Kara

Quoted 29 lines · Michal Hocko

Quoted 15 lines · Jan Kara

Quoted 5 lines · Michal Hocko

Quoted 5 lines

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

Quoted 8 lines

Quoted 3 lines

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path.

But we do not enter the writeback path from the direct reclaim. Or do you mean something other than pageout()'s mapping->a_ops->writepage? There is only try_to_release_page where we get back to the filesystems but I do not see any NOFS protection in ext4_releasepage.

Quoted 5 lines

Maybe to expand a bit: These days, direct reclaim can call ->releasepage() callback, ->evict_inode() callback (and only for inodes with i_nlink > 0), shrinkers. That's it. So the recursion possibilities are rather more limited than they used to be several years ago and we likely do not need as much GFP_NOFS protection as we used to.

Thanks for making my remark more clear Jack! I would just want to add that I was playing with the patch below (it is basically GFP_NOFS->GFP_KERNEL for all allocations which trigger warning from the debugging patch which means they are called from within transaction) and it didn't hit the lockdep when running xfstests both with or without the enabled journal.

Quoted 5 lines

Quoted 2 lines

So am I still missing something or the nojournal mode is safe and the current series is OK wrt. ext*?

I'm convinced the current series is OK, only real life will tell us whether we missed something or not ;)

I would like to extend the changelog of "jbd2: mark the transaction context with the scope GFP_NOFS context".

"
Please note that setups without journal do not suffer from potential recursion problems and so they do not need the scope protection because neither ->releasepage nor ->evict_inode (which are the only fs entry points from the direct reclaim) can reenter a locked context which is doing the allocation currently.
"

Quoted 6 lines

Quoted 2 lines

I'm convinced the current series is OK, only real life will tell us whether we missed something or not ;)

I would like to extend the changelog of "jbd2: mark the transaction context with the scope GFP_NOFS context".

Quoted 7 lines

"
Please note that setups without journal do not suffer from potential recursion problems and so they do not need the scope protection because neither ->releasepage nor ->evict_inode (which are the only fs entry points from the direct reclaim) can reenter a locked context which is doing the allocation currently.
"

Could you comment on this Ted, please?

Quoted 14 lines · Michal Hocko

Quoted 6 lines

Quoted 2 lines

I'm convinced the current series is OK, only real life will tell us whether we missed something or not ;)

I would like to extend the changelog of "jbd2: mark the transaction context with the scope GFP_NOFS context".

Quoted 7 lines

"
Please note that setups without journal do not suffer from potential recursion problems and so they do not need the scope protection because neither ->releasepage nor ->evict_inode (which are the only fs entry points from the direct reclaim) can reenter a locked context which is doing the allocation currently.
"

Quoted 1 line

Could you comment on this Ted, please?

I guess.... so there still is one way this could screw us, and it's this reason for GFP_NOFS:

- to prevent from stack overflows during the reclaim because the allocation is performed from a deep context already

The writepages call stack can be pretty deep. (Especially if we're using ext4 in no journal mode over, say, iSCSI.)

How much stack space can get consumed by a reclaim?

If this ever turn out to be a problem and with the vmapped stacks we have good chances to get a proper stack traces on a potential overflow we can add the scope API around the problematic code path with the explanation why it is needed.

The reason why I'm nervous is that nojournal mode is not a common configuration, and "wait until production systems start failing" is not a strategy that I or many SRE-types find.... comforting.

Quoted 3 lines · Theodore Ts'o

The reason why I'm nervous is that nojournal mode is not a common configuration, and "wait until production systems start failing" is not a strategy that I or many SRE-types find.... comforting.

What does SRE stand for?

Quoted 4 lines · Michal Hocko

If this ever turn out to be a problem and with the vmapped stacks we have good chances to get a proper stack traces on a potential overflow we can add the scope API around the problematic code path with the explanation why it is needed.

Yeah, or maybe we can automate it? Can the reclaim code check how much stack space is left and do the right thing automatically?

The reason why I'm nervous is that nojournal mode is not a common configuration, and "wait until production systems start failing" is not a strategy that I or many SRE-types find.... comforting.

Quoted 8 lines · Theodore Ts'o

Quoted 4 lines · Michal Hocko

If this ever turn out to be a problem and with the vmapped stacks we have good chances to get a proper stack traces on a potential overflow we can add the scope API around the problematic code path with the explanation why it is needed.

Yeah, or maybe we can automate it? Can the reclaim code check how much stack space is left and do the right thing automatically?

I am not sure how to do that. Checking for some magic value sounds quite fragile to me. It also sounds a bit strange to focus only on the reclaim while other code paths might suffer from the same problem.

What is actually the deepest possible call chain from the slab reclaim where I stopped? I have tried to follow that path but hit the callback wall quite early.

Quoted 3 lines

The reason why I'm nervous is that nojournal mode is not a common configuration, and "wait until production systems start failing" is not a strategy that I or many SRE-types find.... comforting.

I understand that but I would be much more happier if we did the decision based on the actual data rather than a fear something would break down.

Quoted 5 lines · Michal Hocko

OK, so I've been staring into the code and AFAIU current->journal_info can contain my stored information. I could either hijack part of the word as the ref counting is only consuming low 12b. But that looks too ugly to live. Or I can allocate some placeholder.

Yeah, I was looking at something similar. Can you guarantee that the context will only take one or two bits? (Looks like it only needs one bit ATM, even though at the moment you're storing the whole GFP mask, correct?)

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path. Certainly not for the same inode, and while we could make it work if the mm was writing back another inode, or another superblock, there are also stack depth considerations that would make this be a bad idea. So we do need to be able to assert GFP_NOFS even in no journal mode, and for any file system including ext2, for that matter.

Because of the fact that we're going to have to play games with current->journal_info, maybe this is something that I should take responsibility for, and to go through the the ext4 tree after the main patch series go through? Maybe you could use xfs and ext2 as sample (simple) implementations?

My only ask is that the memalloc nofs context be a well defined N bits, where N < 16, and I'll find some place to put them (probably journal_info).

Quoted 33 lines · Theodore Ts'o

Quoted 5 lines · Michal Hocko

OK, so I've been staring into the code and AFAIU current->journal_info can contain my stored information. I could either hijack part of the word as the ref counting is only consuming low 12b. But that looks too ugly to live. Or I can allocate some placeholder.

Yeah, I was looking at something similar. Can you guarantee that the context will only take one or two bits? (Looks like it only needs one bit ATM, even though at the moment you're storing the whole GFP mask, correct?)

Quoted 4 lines

But before going to play with that I am really wondering whether we need all this with no journal at all. AFAIU what Jack told me it is the journal lock(s) which is the biggest problem from the reclaim recursion point of view. What would cause a deadlock in no journal mode?

We still have the original problem for why we need GFP_NOFS even in ext2. If we are in a writeback path, and we need to allocate memory, we don't want to recurse back into the file system's writeback path. Certainly not for the same inode, and while we could make it work if the mm was writing back another inode, or another superblock, there are also stack depth considerations that would make this be a bad idea. So we do need to be able to assert GFP_NOFS even in no journal mode, and for any file system including ext2, for that matter.

Because of the fact that we're going to have to play games with current->journal_info, maybe this is something that I should take responsibility for, and to go through the the ext4 tree after the main patch series go through? Maybe you could use xfs and ext2 as sample (simple) implementations?

Quoted 3 lines

My only ask is that the memalloc nofs context be a well defined N bits, where N < 16, and I'll find some place to put them (probably journal_info).

I think Dave was suggesting that the NOFS context allow a pointer to an arbitrary struct, so that it is possible to dereference this in the filesystem itself to determine if the recursion is safe or not.