mm, xfs: non-blocking inode reclaim

From: Dave Chinner <[email protected]>

Introduce a mechanism for ->count_objects() to indicate to the shrinker infrastructure that the reclaim context will not allow scanning work to be done and so the work it decides is necessary needs to be deferred.

This simplifies the code by separating out the accounting of deferred work from the actual doing of the work, and allows better decisions to be made by the shrinekr control logic on what action it can take.

Signed-off-by Dave Chinner <[email protected]>

---
 include/linux/shrinker.h | 7 +++++++
 mm/vmscan.c              | 8 ++++++++
 2 files changed, 15 insertions(+)

include/linux/shrinker.h +0 -0

index 9443cafd1969..af78c475fc32 100644

include/linux/shrinker.h +0 -0

 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index 44df66a98f2a..ae3035fe94bc 100644

mm/vmscan.c +1 -0

 	/*
 	 * Normally, we should not scan less than batch_size objects in one
 	 * pass to avoid too frequent shrinker calls, but if the slab has less

@@ -575,6 +582,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		cond_resched();
 	}
 
+done:
 	if (next_deferred >= scanned)
 		next_deferred -= scanned;
 	else

Signature · 1 line

2.22.0

Quoted 24 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Introduce a mechanism for ->count_objects() to indicate to the shrinker infrastructure that the reclaim context will not allow scanning work to be done and so the work it decides is necessary needs to be deferred.

This simplifies the code by separating out the accounting of deferred work from the actual doing of the work, and allows better decisions to be made by the shrinekr control logic on what action it can take.

Signed-off-by Dave Chinner <[email protected]>

---
 include/linux/shrinker.h | 7 +++++++
 mm/vmscan.c              | 8 ++++++++
 2 files changed, 15 insertions(+)

include/linux/shrinker.h +0 -0

index 9443cafd1969..af78c475fc32 100644
@@ -31,6 +31,13 @@ struct shrink_control {
 

include/linux/shrinker.h struct shrink_control { · line 40

Quoted 9 lines

include/linux/shrinker.h +7 -0

 	/* current memcg being shrunk (for memcg aware shrinkers) */
 	struct mem_cgroup *memcg;
+
+	/*
+	 * set by ->count_objects if reclaim context prevents reclaim from
+	 * occurring. This allows the shrinker to immediately defer all the
+	 * work and not even attempt to scan the cache.
+	 */
+	bool will_defer;

Functionality wise this seems fairly straightforward. FWIW, I find the 'will_defer' name a little confusing because it implies to me that the shrinker is telling the caller about something it would do if called as opposed to explicitly telling the caller to defer. I'd just call it 'defer' I guess, but that's just my .02. ;P

Quoted 11 lines

include/linux/shrinker.h +0 -0

 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index 44df66a98f2a..ae3035fe94bc 100644
@@ -541,6 +541,13 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
 				   freeable, delta, total_scan, priority);
 

mm/vmscan.c static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, · line 550

Quoted 7 lines

mm/vmscan.c +7 -0

+	/*
+	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then
+	 * defer the work to a context that can scan the cache.
+	 */
+	if (shrinkctl->will_defer)
+		goto done;
+

Who's responsible for clearing the flag? Perhaps we should do so here once it's acted upon since we don't call into the shrinker again?

Note that I see this structure is reinitialized on every iteration in the caller, but there already is the SHRINK_EMPTY case where we call back into do_shrink_slab().

Granted the deferred state likely hasn't
changed, but the fact that we'd call back into the count callback to set it again implies the logic could be a bit more explicit, particularly if this will eventually be used for more dynamic shrinker state that might change call to call (i.e., object dirty state, etc.).

BTW, do we need to care about the ->nr_cached_objects() call from the generic superblock shrinker (super_cache_scan())?

Quoted 25 lines · Brian Foster

Quoted 24 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Introduce a mechanism for ->count_objects() to indicate to the shrinker infrastructure that the reclaim context will not allow scanning work to be done and so the work it decides is necessary needs to be deferred.

This simplifies the code by separating out the accounting of deferred work from the actual doing of the work, and allows better decisions to be made by the shrinekr control logic on what action it can take.

Signed-off-by Dave Chinner <[email protected]>

---
 include/linux/shrinker.h | 7 +++++++
 mm/vmscan.c              | 8 ++++++++
 2 files changed, 15 insertions(+)

include/linux/shrinker.h +0 -0

index 9443cafd1969..af78c475fc32 100644
@@ -31,6 +31,13 @@ struct shrink_control {
 

Quoted 15 lines

include/linux/shrinker.h struct shrink_control { · line 40

Quoted 9 lines

include/linux/shrinker.h +7 -0

 	/* current memcg being shrunk (for memcg aware shrinkers) */
 	struct mem_cgroup *memcg;
+
+	/*
+	 * set by ->count_objects if reclaim context prevents reclaim from
+	 * occurring. This allows the shrinker to immediately defer all the
+	 * work and not even attempt to scan the cache.
+	 */
+	bool will_defer;

Functionality wise this seems fairly straightforward. FWIW, I find the 'will_defer' name a little confusing because it implies to me that the shrinker is telling the caller about something it would do if called as opposed to explicitly telling the caller to defer. I'd just call it 'defer' I guess, but that's just my .02. ;P

Ok, I'll change it to something like "defer_work" or "defer_scan" here.

Quoted 11 lines

Quoted 11 lines

include/linux/shrinker.h +0 -0

 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index 44df66a98f2a..ae3035fe94bc 100644
@@ -541,6 +541,13 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
 				   freeable, delta, total_scan, priority);
 

Quoted 10 lines

mm/vmscan.c static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, · line 550

Quoted 7 lines

mm/vmscan.c +7 -0

+	/*
+	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then
+	 * defer the work to a context that can scan the cache.
+	 */
+	if (shrinkctl->will_defer)
+		goto done;
+

Who's responsible for clearing the flag? Perhaps we should do so here once it's acted upon since we don't call into the shrinker again?

Each shrinker invocation has it's own shrink_control context - they are not shared between shrinkers - the higher level is responsible for setting up the control state of each individual shrinker invocation...

Quoted 3 lines

Note that I see this structure is reinitialized on every iteration in the caller, but there already is the SHRINK_EMPTY case where we call back into do_shrink_slab().

.... because there is external state tracking in memcgs that determine what shrinkers get run. See shrink_slab_memcg().

i.e. The SHRINK_EMPTY return value is a special hack for memcg shrinkers so it can track whether there are freeable objects in the cache externally to try to avoid calling into shrinkers where no work can be done. Think about having hundreds of shrinkers and hundreds of memcgs...

Anyway, the tracking of the freeable bit is racy, so the SHRINK_EMPTY hack where it clears the bit and calls back into the shrinker is handling the case where objects were freed between the shrinker running and shrink_slab_memcg() clearing the freeable bit from the slab. Hence it has to call back into the shrinker again - if it gets anything other than SHRINK_EMPTY returned, then it will set the bit again.

In reality, SHRINK_EMPTY and deferring work are mutually exclusive. Work only gets deferred when there's work that can be done and in that case SHRINK_EMPTY will not be returned - a value of "0 freed objects" will be returned when we defer work. So if the first call returns SHRINK_EMPTY, the "defer" state has not been touched and so doesn't require resetting to zero here.

Quoted 6 lines

Granted the deferred state likely hasn't
changed, but the fact that we'd call back into the count callback to set it again implies the logic could be a bit more explicit, particularly if this will eventually be used for more dynamic shrinker state that might change call to call (i.e., object dirty state, etc.).

Quoted 2 lines

BTW, do we need to care about the ->nr_cached_objects() call from the generic superblock shrinker (super_cache_scan())?

No, and we never had to because it is inside the superblock shrinker and the superblock shrinker does the GFP_NOFS context checks.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 24 lines · Dave Chinner

Quoted 12 lines · Brian Foster

Quoted 11 lines · Dave Chinner

};

#define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index 44df66a98f2a..ae3035fe94bc 100644
@@ -541,6 +541,13 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
 				   freeable, delta, total_scan, priority);
 

Quoted 10 lines

mm/vmscan.c static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, · line 550

Quoted 7 lines

mm/vmscan.c +7 -0

+	/*
+	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then
+	 * defer the work to a context that can scan the cache.
+	 */
+	if (shrinkctl->will_defer)
+		goto done;
+

Who's responsible for clearing the flag? Perhaps we should do so here once it's acted upon since we don't call into the shrinker again?

Quoted 5 lines

Each shrinker invocation has it's own shrink_control context - they are not shared between shrinkers - the higher level is responsible for setting up the control state of each individual shrinker invocation...

Yes, but more specifically, it appears to me that each level is responsible for setting up control state managed by that level. E.g., shrink_slab_memcg() initializes the unchanging state per iteration and do_shrink_slab() (re)sets the scan state prior to ->scan_objects().

Quoted 9 lines

Quoted 8 lines

Granted the deferred state likely hasn't
changed, but the fact that we'd call back into the count callback to set it again implies the logic could be a bit more explicit, particularly if this will eventually be used for more dynamic shrinker state that might change call to call (i.e., object dirty state, etc.).

BTW, do we need to care about the ->nr_cached_objects() call from the generic superblock shrinker (super_cache_scan())?

Quoted 3 lines

No, and we never had to because it is inside the superblock shrinker and the superblock shrinker does the GFP_NOFS context checks.

Ok. Though tbh this topic has me wondering whether a shrink_control boolean is the right approach here. Do you envision ->will_defer being used for anything other than allocation context restrictions? If not,

perhaps we should do something like optionally set alloc flags required for direct scanning in the struct shrinker itself and let the core shrinker code decide when to defer to kswapd based on the shrink_control flags and the current shrinker. That way an arbitrary shrinker can't muck around with core behavior in unintended ways. Hm?

Quoted 31 lines · Brian Foster

Quoted 24 lines · Dave Chinner

Quoted 12 lines · Brian Foster

Quoted 11 lines · Dave Chinner

};

#define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index 44df66a98f2a..ae3035fe94bc 100644
@@ -541,6 +541,13 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
 				   freeable, delta, total_scan, priority);
 

Quoted 10 lines

mm/vmscan.c static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, · line 550

Quoted 7 lines

mm/vmscan.c +7 -0

+	/*
+	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then
+	 * defer the work to a context that can scan the cache.
+	 */
+	if (shrinkctl->will_defer)
+		goto done;
+

Who's responsible for clearing the flag? Perhaps we should do so here once it's acted upon since we don't call into the shrinker again?

Quoted 5 lines

Each shrinker invocation has it's own shrink_control context - they are not shared between shrinkers - the higher level is responsible for setting up the control state of each individual shrinker invocation...

Quoted 4 lines

Yes, but more specifically, it appears to me that each level is responsible for setting up control state managed by that level. E.g., shrink_slab_memcg() initializes the unchanging state per iteration and do_shrink_slab() (re)sets the scan state prior to ->scan_objects().

do_shrink_slab() is responsible for iterating the scan in shrinker->batch sizes, that's all it's doing there. We have to do some accounting work from scan to scan. However, if ->will_defer is set, we skip that entire loop, so it's largely irrelevant IMO.

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

Granted the deferred state likely hasn't
changed, but the fact that we'd call back into the count callback to set it again implies the logic could be a bit more explicit, particularly if this will eventually be used for more dynamic shrinker state that might change call to call (i.e., object dirty state, etc.).

BTW, do we need to care about the ->nr_cached_objects() call from the generic superblock shrinker (super_cache_scan())?

No, and we never had to because it is inside the superblock shrinker and the superblock shrinker does the GFP_NOFS context checks.

Quoted 3 lines

Ok. Though tbh this topic has me wondering whether a shrink_control boolean is the right approach here. Do you envision ->will_defer being used for anything other than allocation context restrictions? If not,

Not at this point. If there are other control flags needed, we can ad them in future - I don't like the idea of having a single control flag mean different things in different contexts.

Quoted 5 lines

perhaps we should do something like optionally set alloc flags required for direct scanning in the struct shrinker itself and let the core shrinker code decide when to defer to kswapd based on the shrink_control flags and the current shrinker. That way an arbitrary shrinker can't muck around with core behavior in unintended ways. Hm?

Arbitrary shrinkers can't "muck about" with the core behaviour any more than they already could with this code. If you want to screw up the core reclaim by always returning SHRINK_STOP to ->scan_objects instead of doing work, then there is nothing stopping you from doing that right now. Formalising there work deferral into a flag in the shrink_control doesn't really change that at all, adn as such I don't see any need for over-complicating the mechanism here....

If you add a generic "defer work" knob to the shrinker mechanism, but only process it as an "allocation context" check, I expect it could be easily misused. For example, some shrinkers may decide to set the the flag dynamically based on in-core state.

This will work when called from
some contexts but not from others (unrelated to allocation context), which is confusing. Therefore, what I'm saying is that if the only current use case is to defer work from shrinkers that currently skip work due to allocation context restraints, this might be better codified with something like the appended (untested) example patch. This may or may not be a preferable interface to the flag, but it's certainly not an overcomplication...

Quoted 4 lines · Brian Foster

If you add a generic "defer work" knob to the shrinker mechanism, but only process it as an "allocation context" check, I expect it could be easily misused. For example, some shrinkers may decide to set the the flag dynamically based on in-core state.

Which is already the case. e.g. There are shrinkers that don't do anything because a try-lock fails. I haven't attempted to change them, but they are a clear example of how even ->scan_object to ->scan_object the shrinker context can change.

Quoted 8 lines

This will work when called from
some contexts but not from others (unrelated to allocation context), which is confusing. Therefore, what I'm saying is that if the only current use case is to defer work from shrinkers that currently skip work due to allocation context restraints, this might be better codified with something like the appended (untested) example patch. This may or may not be a preferable interface to the flag, but it's certainly not an overcomplication...

I don't think this is the right way to go.

I want the filesystem shrinkers to become entirely non-blocking so that we can dynamically decide on an object-by-object basis whether we can reclaim the object in GFP_NOFS context.

That is, a clean XFS inode that requires no special cleanup can be reclaimed even in GFP_NOFS context. The problem we have is that dentry reclaim can drop the last reference to an inode, causing inactivation and hence modification. However, if it's only going to move to the inode LRU and not evict the inode, we can reclaim that dentry. Similarly for inodes - if evicting the inode is not going to block or modify the inode, we can reclaim the inode even under GFP_NOFS constraints. And the same for XFS indoes - it if's clean we can reclaim it, GFP_NOFS context or not.

IMO, that's the direction we need to be heading in, and in those cases the "deferred work" tends towards a count of objects we could not reclaim during the scan because they require blocking work to be done. i.e. deferred work is a boolean now because the GFP_NOFS decision is boolean, but it's lays the ground work for deferred work to be integrated at a much finer-grained level in the shrinker scanning routines in future...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Dave Chinner <[email protected]>

For shrinkers that currently avoid scanning when called under GFP_NOFS contexts, conver them to use the new ->will_defer flag rather than checking and returning errors during scans.

This makes it very clear that these shrinkers are not doing any work because of the context limitations, not because there is no work that can be done.

Signed-off-by Dave Chinner <[email protected]>

---
 drivers/staging/android/ashmem.c |  8 ++++----
 fs/gfs2/glock.c                  |  5 +++--
 fs/gfs2/quota.c                  |  6 +++---
 fs/nfs/dir.c                     |  6 +++---
 fs/super.c                       |  6 +++---
 fs/xfs/xfs_buf.c                 |  4 ++++
 fs/xfs/xfs_qm.c                  | 11 ++++++++---
 net/sunrpc/auth.c                |  5 ++---
 8 files changed, 30 insertions(+), 21 deletions(-)

fs/xfs/xfs_buf.c +0 -0

index ca0849043f54..6e0f76532535 100644

fs/xfs/xfs_qm.c +8 -3

index 5e7a37f0cf84..13c842e8f13b 100644
@@ -502,9 +502,6 @@ xfs_qm_shrink_scan(
 	unsigned long		freed;
 	int			error;
 
-	if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) != (__GFP_FS|__GFP_DIRECT_RECLAIM))
-		return 0;
-
 	INIT_LIST_HEAD(&isol.buffers);
 	INIT_LIST_HEAD(&isol.dispose);
 

@@ -534,6 +531,14 @@ xfs_qm_shrink_count(
 	struct xfs_quotainfo	*qi = container_of(shrink,
 					struct xfs_quotainfo, qi_shrinker);
 
+	/*
+	 * __GFP_DIRECT_RECLAIM is used here to avoid blocking kswapd
+	 */
+	if ((sc->gfp_mask & (__GFP_FS|__GFP_DIRECT_RECLAIM)) !=
+					(__GFP_FS|__GFP_DIRECT_RECLAIM)) {
+		sc->will_defer = true;
+	}
+
 	return list_lru_shrink_count(&qi->qi_lru, sc);
 }
 

net/sunrpc/auth.c +2 -3

index cdb05b48de44..6babcbac4a00 100644
@@ -527,9 +527,6 @@ static unsigned long
 rpcauth_cache_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
 
 {
-	if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
-		return SHRINK_STOP;
-
 	/* nothing left, don't come back */
 	if (list_empty(&cred_unused))
 		return SHRINK_STOP;

@@ -541,6 +538,8 @@ static unsigned long
 rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
 
 {
+	if ((sc->gfp_mask & GFP_KERNEL) != GFP_KERNEL)
+		sc->will_defer = true;
 	return number_cred_unused * sysctl_vfs_cache_pressure / 100;
 }
 

Signature · 1 line

2.22.0

Quoted 22 lines · Dave Chinner

From: Dave Chinner <[email protected]>

For shrinkers that currently avoid scanning when called under GFP_NOFS contexts, conver them to use the new ->will_defer flag rather than checking and returning errors during scans.

This makes it very clear that these shrinkers are not doing any work because of the context limitations, not because there is no work that can be done.

Signed-off-by Dave Chinner <[email protected]>

---
 drivers/staging/android/ashmem.c |  8 ++++----
 fs/gfs2/glock.c                  |  5 +++--
 fs/gfs2/quota.c                  |  6 +++---
 fs/nfs/dir.c                     |  6 +++---
 fs/super.c                       |  6 +++---
 fs/xfs/xfs_buf.c                 |  4 ++++
 fs/xfs/xfs_qm.c                  | 11 ++++++++---
 net/sunrpc/auth.c                |  5 ++---
 8 files changed, 30 insertions(+), 21 deletions(-)

...

Quoted 5 lines

fs/xfs/xfs_buf.c +0 -0

index ca0849043f54..6e0f76532535 100644

fs/xfs/xfs_buf.c xfs_buftarg_shrink_count( · line 1688

Quoted 9 lines

fs/xfs/xfs_buf.c +4 -0

@@ -1680,6 +1680,10 @@ xfs_buftarg_shrink_count(
 {
 	struct xfs_buftarg	*btp = container_of(shrink,
 					struct xfs_buftarg, bt_shrinker);
+
+	if (!(sc->gfp_mask & __GFP_FS))
+		sc->will_defer = true;
+
 	return list_lru_shrink_count(&btp->bt_lru, sc);
 }

This hunk looks like a behavior change / bug fix..? The rest of the

From: Dave Chinner <[email protected]>

Start to clean up the shrinker code by factoring out the calculation that determines how much work to do. This separates the calculation from clamping and other adjustments that are done before the shrinker work is run.

Also convert the calculation for the amount of work to be done to use 64 bit logic so we don't have to keep jumping through hoops to keep calculations within 32 bits on 32 bit systems.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 27 deletions(-)

mm/vmscan.c +0 -0

index ae3035fe94bc..b7472953b0e6 100644
@@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
 

mm/vmscan.c +12 -0

+	} else {
+		/*
+		 * These objects don't require any IO to create. Trim
+		 * them aggressively under memory pressure to keep
+		 * them from causing refetches in the IO caches.
+		 */
+		delta = freeable / 2;
+	}
+
+	*freeable_objects = freeable;
+	return delta > 0 ? delta : 0;
+}

mm/vmscan.c +13 -25

 	long nr;
 	long new_nr;
 	int nid = shrinkctl->nid;

@@ -481,9 +513,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
 		nid = 0;
 
-	freeable = shrinker->count_objects(shrinker, shrinkctl);
-	if (freeable == 0 || freeable == SHRINK_EMPTY)
-		return freeable;
+	scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
+					&freeable_objects);
+	if (scan_count == 0 || scan_count == SHRINK_EMPTY)
+		return scan_count;
 
 	/*
 	 * copy the current shrinker scan count into a local variable

@@ -492,25 +525,11 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 */
 	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
 
-	total_scan = nr;
-	if (shrinker->seeks) {
-		delta = freeable >> priority;
-		delta *= 4;
-		do_div(delta, shrinker->seeks);
-	} else {
-		/*
-		 * These objects don't require any IO to create. Trim
-		 * them aggressively under memory pressure to keep
-		 * them from causing refetches in the IO caches.
-		 */
-		delta = freeable / 2;
-	}
-
-	total_scan += delta;
+	total_scan = nr + scan_count;
 	if (total_scan < 0) {
 		pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
 		       shrinker->scan_objects, total_scan);
-		total_scan = freeable;
+		total_scan = scan_count;
 		next_deferred = nr;
 	} else
 		next_deferred = total_scan;

@@ -527,19 +546,20 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * Hence only allow the shrinker to scan the entire cache when
 	 * a large delta change is calculated directly.
 	 */
-	if (delta < freeable / 4)
-		total_scan = min(total_scan, freeable / 2);
+	if (scan_count < freeable_objects / 4)
+		total_scan = min_t(long, total_scan, freeable_objects / 2);
 
 	/*
 	 * Avoid risking looping forever due to too large nr value:
 	 * never try to free more than twice the estimate number of
 	 * freeable entries.
 	 */
-	if (total_scan > freeable * 2)
-		total_scan = freeable * 2;
+	if (total_scan > freeable_objects * 2)
+		total_scan = freeable_objects * 2;
 
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
-				   freeable, delta, total_scan, priority);
+				   freeable_objects, scan_count,
+				   total_scan, priority);
 
 	/*
 	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then

@@ -564,7 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * possible.
 	 */
 	while (total_scan >= batch_size ||
-	       total_scan >= freeable) {
+	       total_scan >= freeable_objects) {
 		unsigned long ret;
 		unsigned long nr_to_scan = min(batch_size, total_scan);
 

Quoted 24 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Start to clean up the shrinker code by factoring out the calculation that determines how much work to do. This separates the calculation from clamping and other adjustments that are done before the shrinker work is run.

Also convert the calculation for the amount of work to be done to use 64 bit logic so we don't have to keep jumping through hoops to keep calculations within 32 bits on 32 bit systems.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 27 deletions(-)

mm/vmscan.c +0 -0

index ae3035fe94bc..b7472953b0e6 100644
@@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
 
 #define SHRINK_BATCH 128
 

mm/vmscan.c EXPORT_SYMBOL(unregister_shrinker); · line 474

Quoted 8 lines

mm/vmscan.c +8 -0

+/*
+ * Calculate the number of new objects to scan this time around. Return
+ * the work to be done. If there are freeable objects, return that number in
+ * @freeable_objects.
+ */
+static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
+			    struct shrinker *shrinker, int priority,
+			    int64_t *freeable_objects)

nit: make the return parm definition also uin64_t, also we have u64 types.

Quoted 5 lines

mm/vmscan.c +5 -0

+{
+	uint64_t delta;
+	uint64_t freeable;
+
+	freeable = shrinker->count_objects(shrinker, shrinkctl);

mm/vmscan.c EXPORT_SYMBOL(unregister_shrinker); · line 485

Quoted 6 lines

mm/vmscan.c +6 -0

+	if (freeable == 0 || freeable == SHRINK_EMPTY)
+		return freeable;
+
+	if (shrinker->seeks) {
+		delta = freeable >> (priority - 2);
+		do_div(delta, shrinker->seeks);

a comment about the reasoning behind this calculation would be nice.

mm/vmscan.c EXPORT_SYMBOL(unregister_shrinker); · line 505

Quoted 9 lines

mm/vmscan.c +2 -2

 static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
 	unsigned long freed = 0;
-	unsigned long long delta;
 	long total_scan;
-	long freeable;
+	int64_t freeable_objects = 0;
+	int64_t scan_count;

why int and not uint64 ? We can never have negative object count, right?

From: Dave Chinner <[email protected]>

Start to clean up the shrinker code by factoring out the calculation that determines how much work to do. This separates the calculation from clamping and other adjustments that are done before the shrinker work is run.

Also convert the calculation for the amount of work to be done to use 64 bit logic so we don't have to keep jumping through hoops to keep calculations within 32 bits on 32 bit systems.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 74 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 27 deletions(-)

mm/vmscan.c +20 -0

index ae3035fe94bc..b7472953b0e6 100644
@@ -464,13 +464,45 @@ EXPORT_SYMBOL(unregister_shrinker);
 
 #define SHRINK_BATCH 128
 
+/*
+ * Calculate the number of new objects to scan this time around. Return
+ * the work to be done. If there are freeable objects, return that number in
+ * @freeable_objects.
+ */
+static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
+			    struct shrinker *shrinker, int priority,
+			    int64_t *freeable_objects)
+{
+	uint64_t delta;
+	uint64_t freeable;
+
+	freeable = shrinker->count_objects(shrinker, shrinkctl);
+	if (freeable == 0 || freeable == SHRINK_EMPTY)
+		return freeable;
+
+	if (shrinker->seeks) {
+		delta = freeable >> (priority - 2);
+		do_div(delta, shrinker->seeks);
+	} else {

mm/vmscan.c +8 -16

+}
+
 static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
 	unsigned long freed = 0;
-	unsigned long long delta;
 	long total_scan;
-	long freeable;
+	int64_t freeable_objects = 0;
+	int64_t scan_count;
 	long nr;
 	long new_nr;
 	int nid = shrinkctl->nid;

@@ -481,9 +513,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
 		nid = 0;
 
-	freeable = shrinker->count_objects(shrinker, shrinkctl);
-	if (freeable == 0 || freeable == SHRINK_EMPTY)
-		return freeable;
+	scan_count = shrink_scan_count(shrinkctl, shrinker, priority,
+					&freeable_objects);
+	if (scan_count == 0 || scan_count == SHRINK_EMPTY)
+		return scan_count;
 
 	/*
 	 * copy the current shrinker scan count into a local variable

@@ -492,25 +525,11 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 */
 	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
 
-	total_scan = nr;
-	if (shrinker->seeks) {
-		delta = freeable >> priority;
-		delta *= 4;
-		do_div(delta, shrinker->seeks);
-	} else {
-		/*
-		 * These objects don't require any IO to create. Trim
-		 * them aggressively under memory pressure to keep
-		 * them from causing refetches in the IO caches.
-		 */

mm/vmscan.c +7 -6

 		next_deferred = nr;
 	} else
 		next_deferred = total_scan;

@@ -527,19 +546,20 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * Hence only allow the shrinker to scan the entire cache when
 	 * a large delta change is calculated directly.
 	 */
-	if (delta < freeable / 4)
-		total_scan = min(total_scan, freeable / 2);
+	if (scan_count < freeable_objects / 4)
+		total_scan = min_t(long, total_scan, freeable_objects / 2);
 
 	/*
 	 * Avoid risking looping forever due to too large nr value:
 	 * never try to free more than twice the estimate number of
 	 * freeable entries.
 	 */
-	if (total_scan > freeable * 2)
-		total_scan = freeable * 2;
+	if (total_scan > freeable_objects * 2)
+		total_scan = freeable_objects * 2;
 
 	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
-				   freeable, delta, total_scan, priority);
+				   freeable_objects, scan_count,
+				   total_scan, priority);
 
 	/*
 	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then

@@ -564,7 +584,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * possible.
 	 */
 	while (total_scan >= batch_size ||
-	       total_scan >= freeable) {
+	       total_scan >= freeable_objects) {
 		unsigned long ret;
 		unsigned long nr_to_scan = min(batch_size, total_scan);
 

Signature · 1 line

2.22.0

From: Dave Chinner <[email protected]>

Right now deferred work is picked up by whatever GFP_KERNEL context reclaimer that wins the race to empty the node's deferred work counter. However, if there are lots of direct reclaimers, that work might be continually picked up by contexts taht can't do any work and so the opportunities to do the work are missed by contexts that could do them.

A further problem with the current code is that the deferred work can be picked up by a random direct reclaimer, resulting in that specific process having to do all the deferred reclaim work and hence can take extremely long latencies if the reclaim work blocks regularly. This is not good for direct reclaim fairness or for minimising long tail latency events.

To avoid these problems, simply limit deferred work to kswapd contexts. We know kswapd is a context that can always do reclaim work, and hence deferring work to kswapd allows the deferred work to be done in the background and not adversely affect any specific process context doing direct reclaim.

The advantage of this is that amount of work to be done in direct reclaim is now bound and predictable - it is entirely based on the cache's freeable objects and the reclaim priority. hence all direct reclaimers running at the same time should be doing relatively equal amounts of work, thereby reducing the incidence of long tail latencies due to uneven reclaim workloads.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

mm/vmscan.c +24 -29

index b7472953b0e6..c583b4efb9bf 100644
@@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
 	unsigned long freed = 0;
-	long total_scan;
 	int64_t freeable_objects = 0;
 	int64_t scan_count;
-	long nr;
+	int64_t scanned_objects = 0;
+	int64_t next_deferred = 0;
+	int64_t deferred_count = 0;
 	long new_nr;
 	int nid = shrinkctl->nid;
 	long batch_size = shrinker->batch ? shrinker->batch
 					  : SHRINK_BATCH;
-	long scanned = 0, next_deferred;
 
 	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
 		nid = 0;

@@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		return scan_count;
 
 	/*
-	 * copy the current shrinker scan count into a local variable
-	 * and zero it so that other concurrent shrinker invocations
-	 * don't also do this scanning work.
+	 * If kswapd, we take all the deferred work and do it here. We don't let
+	 * direct reclaim do this, because then it means some poor sod is going
+	 * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
+	 * amount of reclaim work from concurrent kswapd operations. Hence we do
+	 * the work in the wrong place, at the wrong time, and it's largely
+	 * unpredictable.
+	 *
+	 * By doing the deferred work only in kswapd, we can schedule the work
+	 * according the the reclaim priority - low priority reclaim will do
+	 * less deferred work, hence we'll do more of the deferred work the more
+	 * desperate we become for free memory. This avoids the need for needing
+	 * to specifically avoid deferred work windup as low amount os memory
+	 * pressure won't excessive trim caches anymore.
 	 */
-	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+	if (current_is_kswapd()) {
+		int64_t	deferred_scan;
 
-	total_scan = nr + scan_count;
-	if (total_scan < 0) {
-		pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
-		       shrinker->scan_objects, total_scan);
-		total_scan = scan_count;
-		next_deferred = nr;
-	} else
-		next_deferred = total_scan;
+		deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
 
-	/*
-	 * We need to avoid excessive windup on filesystem shrinkers
-	 * due to large numbers of GFP_NOFS allocations causing the
-	 * shrinkers to return -1 all the time. This results in a large
-	 * nr being built up so when a shrink that can do some work
-	 * comes along it empties the entire cache due to nr >>>
-	 * freeable. This is bad for sustaining a working set in
-	 * memory.
-	 *
-	 * Hence only allow the shrinker to scan the entire cache when
-	 * a large delta change is calculated directly.
-	 */
-	if (scan_count < freeable_objects / 4)
-		total_scan = min_t(long, total_scan, freeable_objects / 2);
+		/* we want to scan 5-10% of the deferred work here at minimum */
+		deferred_scan = deferred_count;
+		if (priority)
+			do_div(deferred_scan, priority);
+		scan_count += deferred_scan;

mm/vmscan.c +10 -9

+	}
 
 	/*
 	 * Avoid risking looping forever due to too large nr value:
 	 * never try to free more than twice the estimate number of
 	 * freeable entries.
 	 */
-	if (total_scan > freeable_objects * 2)
-		total_scan = freeable_objects * 2;
+	if (scan_count > freeable_objects * 2)
+		scan_count = freeable_objects * 2;
 
-	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
+	trace_mm_shrink_slab_start(shrinker, shrinkctl, deferred_count,
 				   freeable_objects, scan_count,
-				   total_scan, priority);
+				   scan_count, priority);
 
 	/*
 	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then

@@ -583,10 +589,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * scanning at high prio and therefore should try to reclaim as much as
 	 * possible.
 	 */
-	while (total_scan >= batch_size ||
-	       total_scan >= freeable_objects) {
+	while (scan_count >= batch_size ||
+	       scan_count >= freeable_objects) {
 		unsigned long ret;
-		unsigned long nr_to_scan = min(batch_size, total_scan);
+		unsigned long nr_to_scan = min_t(long, batch_size, scan_count);
 
 		shrinkctl->nr_to_scan = nr_to_scan;
 		shrinkctl->nr_scanned = nr_to_scan;

@@ -596,17 +602,17 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		freed += ret;
 
 		count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
-		total_scan -= shrinkctl->nr_scanned;
-		scanned += shrinkctl->nr_scanned;
+		scan_count -= shrinkctl->nr_scanned;
+		scanned_objects += shrinkctl->nr_scanned;
 
 		cond_resched();
 	}

mm/vmscan.c +2 -1

 	/*
 	 * move the unused scan count back into the shrinker in a
 	 * manner that handles concurrent updates. If we exhausted the

@@ -618,7 +624,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	else
 		new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
 
-	trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
+	trace_mm_shrink_slab_end(shrinker, nid, freed, deferred_count, new_nr,
+					scan_count);
 	return freed;
 }
 

Signature · 1 line

2.22.0

From: Dave Chinner <[email protected]>

Right now deferred work is picked up by whatever GFP_KERNEL context reclaimer that wins the race to empty the node's deferred work counter. However, if there are lots of direct reclaimers, that work might be continually picked up by contexts taht can't do any work and so the opportunities to do the work are missed by contexts that could do them.

A further problem with the current code is that the deferred work can be picked up by a random direct reclaimer, resulting in that specific process having to do all the deferred reclaim work and hence can take extremely long latencies if the reclaim work blocks regularly. This is not good for direct reclaim fairness or for minimising long tail latency events.

To avoid these problems, simply limit deferred work to kswapd contexts. We know kswapd is a context that can always do reclaim work, and hence deferring work to kswapd allows the deferred work to be done in the background and not adversely affect any specific process context doing direct reclaim.

The advantage of this is that amount of work to be done in direct reclaim is now bound and predictable - it is entirely based on the cache's freeable objects and the reclaim priority. hence all direct reclaimers running at the same time should be doing relatively equal amounts of work, thereby reducing the incidence of long tail latencies due to uneven reclaim workloads.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

mm/vmscan.c +23 -29

index b7472953b0e6..c583b4efb9bf 100644
@@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
 	unsigned long freed = 0;
-	long total_scan;
 	int64_t freeable_objects = 0;
 	int64_t scan_count;
-	long nr;
+	int64_t scanned_objects = 0;
+	int64_t next_deferred = 0;
+	int64_t deferred_count = 0;
 	long new_nr;
 	int nid = shrinkctl->nid;
 	long batch_size = shrinker->batch ? shrinker->batch
 					  : SHRINK_BATCH;
-	long scanned = 0, next_deferred;
 
 	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
 		nid = 0;

@@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		return scan_count;
 
 	/*
-	 * copy the current shrinker scan count into a local variable
-	 * and zero it so that other concurrent shrinker invocations
-	 * don't also do this scanning work.
+	 * If kswapd, we take all the deferred work and do it here. We don't let
+	 * direct reclaim do this, because then it means some poor sod is going
+	 * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
+	 * amount of reclaim work from concurrent kswapd operations. Hence we do
+	 * the work in the wrong place, at the wrong time, and it's largely
+	 * unpredictable.
+	 *
+	 * By doing the deferred work only in kswapd, we can schedule the work
+	 * according the the reclaim priority - low priority reclaim will do
+	 * less deferred work, hence we'll do more of the deferred work the more
+	 * desperate we become for free memory. This avoids the need for needing
+	 * to specifically avoid deferred work windup as low amount os memory
+	 * pressure won't excessive trim caches anymore.
 	 */
-	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+	if (current_is_kswapd()) {
+		int64_t	deferred_scan;
 
-	total_scan = nr + scan_count;
-	if (total_scan < 0) {
-		pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
-		       shrinker->scan_objects, total_scan);
-		total_scan = scan_count;
-		next_deferred = nr;
-	} else
-		next_deferred = total_scan;
+		deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
 
-	/*
-	 * We need to avoid excessive windup on filesystem shrinkers
-	 * due to large numbers of GFP_NOFS allocations causing the
-	 * shrinkers to return -1 all the time. This results in a large
-	 * nr being built up so when a shrink that can do some work
-	 * comes along it empties the entire cache due to nr >>>
-	 * freeable. This is bad for sustaining a working set in
-	 * memory.
-	 *
-	 * Hence only allow the shrinker to scan the entire cache when
-	 * a large delta change is calculated directly.
-	 */
-	if (scan_count < freeable_objects / 4)
-		total_scan = min_t(long, total_scan, freeable_objects / 2);
+		/* we want to scan 5-10% of the deferred work here at minimum */
+		deferred_scan = deferred_count;
+		if (priority)
+			do_div(deferred_scan, priority);

mm/vmscan.c +13 -12

-	trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
+	trace_mm_shrink_slab_start(shrinker, shrinkctl, deferred_count,
 				   freeable_objects, scan_count,
-				   total_scan, priority);
+				   scan_count, priority);
 
 	/*
 	 * If the shrinker can't run (e.g. due to gfp_mask constraints), then

@@ -583,10 +589,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	 * scanning at high prio and therefore should try to reclaim as much as
 	 * possible.
 	 */
-	while (total_scan >= batch_size ||
-	       total_scan >= freeable_objects) {
+	while (scan_count >= batch_size ||
+	       scan_count >= freeable_objects) {
 		unsigned long ret;
-		unsigned long nr_to_scan = min(batch_size, total_scan);
+		unsigned long nr_to_scan = min_t(long, batch_size, scan_count);
 
 		shrinkctl->nr_to_scan = nr_to_scan;
 		shrinkctl->nr_scanned = nr_to_scan;

@@ -596,17 +602,17 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		freed += ret;
 
 		count_vm_events(SLABS_SCANNED, shrinkctl->nr_scanned);
-		total_scan -= shrinkctl->nr_scanned;
-		scanned += shrinkctl->nr_scanned;
+		scan_count -= shrinkctl->nr_scanned;
+		scanned_objects += shrinkctl->nr_scanned;
 
 		cond_resched();
 	}
 
 done:
-	if (next_deferred >= scanned)
-		next_deferred -= scanned;
-	else
-		next_deferred = 0;
+	if (deferred_count)
+		next_deferred = deferred_count - scanned_objects;
+	else if (scan_count > 0)
+		next_deferred = scan_count;
 	/*
 	 * move the unused scan count back into the shrinker in a
 	 * manner that handles concurrent updates. If we exhausted the

@@ -618,7 +624,8 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 	else
 		new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
 
-	trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
+	trace_mm_shrink_slab_end(shrinker, nid, freed, deferred_count, new_nr,
+					scan_count);
 	return freed;
 }
 

Quoted 114 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Right now deferred work is picked up by whatever GFP_KERNEL context reclaimer that wins the race to empty the node's deferred work counter. However, if there are lots of direct reclaimers, that work might be continually picked up by contexts taht can't do any work and so the opportunities to do the work are missed by contexts that could do them.

A further problem with the current code is that the deferred work can be picked up by a random direct reclaimer, resulting in that specific process having to do all the deferred reclaim work and hence can take extremely long latencies if the reclaim work blocks regularly. This is not good for direct reclaim fairness or for minimising long tail latency events.

To avoid these problems, simply limit deferred work to kswapd contexts. We know kswapd is a context that can always do reclaim work, and hence deferring work to kswapd allows the deferred work to be done in the background and not adversely affect any specific process context doing direct reclaim.

The advantage of this is that amount of work to be done in direct reclaim is now bound and predictable - it is entirely based on the cache's freeable objects and the reclaim priority. hence all direct reclaimers running at the same time should be doing relatively equal amounts of work, thereby reducing the incidence of long tail latencies due to uneven reclaim workloads.

Signed-off-by Dave Chinner <[email protected]>

---
 mm/vmscan.c | 93 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

mm/vmscan.c +27 -29

index b7472953b0e6..c583b4efb9bf 100644
@@ -500,15 +500,15 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
 	unsigned long freed = 0;
-	long total_scan;
 	int64_t freeable_objects = 0;
 	int64_t scan_count;
-	long nr;
+	int64_t scanned_objects = 0;
+	int64_t next_deferred = 0;
+	int64_t deferred_count = 0;
 	long new_nr;
 	int nid = shrinkctl->nid;
 	long batch_size = shrinker->batch ? shrinker->batch
 					  : SHRINK_BATCH;
-	long scanned = 0, next_deferred;
 
 	if (!(shrinker->flags & SHRINKER_NUMA_AWARE))
 		nid = 0;

@@ -519,47 +519,53 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		return scan_count;
 
 	/*
-	 * copy the current shrinker scan count into a local variable
-	 * and zero it so that other concurrent shrinker invocations
-	 * don't also do this scanning work.
+	 * If kswapd, we take all the deferred work and do it here. We don't let
+	 * direct reclaim do this, because then it means some poor sod is going
+	 * to have to do somebody else's GFP_NOFS reclaim, and it hides the real
+	 * amount of reclaim work from concurrent kswapd operations. Hence we do
+	 * the work in the wrong place, at the wrong time, and it's largely
+	 * unpredictable.
+	 *
+	 * By doing the deferred work only in kswapd, we can schedule the work
+	 * according the the reclaim priority - low priority reclaim will do
+	 * less deferred work, hence we'll do more of the deferred work the more
+	 * desperate we become for free memory. This avoids the need for needing
+	 * to specifically avoid deferred work windup as low amount os memory
+	 * pressure won't excessive trim caches anymore.
 	 */
-	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+	if (current_is_kswapd()) {
+		int64_t	deferred_scan;
 
-	total_scan = nr + scan_count;
-	if (total_scan < 0) {
-		pr_err("shrink_slab: %pS negative objects to delete nr=%ld\n",
-		       shrinker->scan_objects, total_scan);
-		total_scan = scan_count;
-		next_deferred = nr;
-	} else
-		next_deferred = total_scan;
+		deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
 
-	/*
-	 * We need to avoid excessive windup on filesystem shrinkers
-	 * due to large numbers of GFP_NOFS allocations causing the
-	 * shrinkers to return -1 all the time. This results in a large
-	 * nr being built up so when a shrink that can do some work
-	 * comes along it empties the entire cache due to nr >>>
-	 * freeable. This is bad for sustaining a working set in
-	 * memory.
-	 *
-	 * Hence only allow the shrinker to scan the entire cache when
-	 * a large delta change is calculated directly.
-	 */
-	if (scan_count < freeable_objects / 4)
-		total_scan = min_t(long, total_scan, freeable_objects / 2);
+		/* we want to scan 5-10% of the deferred work here at minimum */
+		deferred_scan = deferred_count;
+		if (priority)
+			do_div(deferred_scan, priority);
+		scan_count += deferred_scan;
+
+		/*
+		 * If there is more deferred work than the number of freeable

mm/vmscan.c static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, · line 554

Quoted 6 lines

mm/vmscan.c +6 -0

+		 * items in the cache, limit the amount of work we will carry
+		 * over to the next kswapd run on this cache. This prevents
+		 * deferred work windup.
+		 */
+		if (deferred_count > freeable_objects * 2)
+			deferred_count = freeable_objects * 2;

nit : deferred_count = min(deferred_count, freeable_objects * 2).

How can we have more deferred objects than are currently on the LRU?

Aren't deferred objects always some part of freeable objects.

From: Dave Chinner <[email protected]>

Running metadata intensive workloads, I've been seeing the AIL pushing getting stuck on pinned buffers and triggering log forces. The log force is taking a long time to run because the log IO is getting throttled by wbt_wait() - the block layer writeback throttle. It's being throttled because there is a huge amount of metadata writeback going on which is filling the request queue.

IOWs, we have a priority inversion problem here.

Mark the log IO bios with REQ_IDLE so they don't get throttled by the block layer writeback throttle. When we are forcing the CIL, we are likely to need to to tens of log IOs, and they are issued as fast as they can be build and IO completed. Hence REQ_IDLE is appropriate - it's an indication that more IO will follow shortly.

And because we also set REQ_SYNC, the writeback throttle will no treat log IO the same way it treats direct IO writes - it will not throttle them at all. Hence we solve the priority inversion problem caused by the writeback throttle being unable to distinguish between high priority log IO and background metadata writeback.

Quoted 17 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Running metadata intensive workloads, I've been seeing the AIL pushing getting stuck on pinned buffers and triggering log forces. The log force is taking a long time to run because the log IO is getting throttled by wbt_wait() - the block layer writeback throttle. It's being throttled because there is a huge amount of metadata writeback going on which is filling the request queue.

IOWs, we have a priority inversion problem here.

Mark the log IO bios with REQ_IDLE so they don't get throttled by the block layer writeback throttle. When we are forcing the CIL, we are likely to need to to tens of log IOs, and they are issued as fast as they can be build and IO completed. Hence REQ_IDLE is appropriate - it's an indication that more IO will follow shortly.

Quoted 6 lines

And because we also set REQ_SYNC, the writeback throttle will no treat log IO the same way it treats direct IO writes - it will not throttle them at all. Hence we solve the priority inversion problem caused by the writeback throttle being unable to distinguish between high priority log IO and background metadata writeback.

[ cc Jens ]

We spent a lot of time getting rid of these inversions in io.latency (and the new io.cost), where REQ_META just blows through the throttling and goes into back charging instead.

It feels awkward to have one set of prio inversion workarounds for io.* and another for wbt. Jens, should we make an explicit one that doesn't rely on magic side effects, or just decide that metadata is meta enough to break all the rules?

Which simply reinforces the fact that that request type based throttling is a fundamentally broken architecture.

Quoted 4 lines

It feels awkward to have one set of prio inversion workarounds for io.* and another for wbt. Jens, should we make an explicit one that doesn't rely on magic side effects, or just decide that metadata is meta enough to break all the rules?

The problem isn't REQ_META blows throw the throttling, the problem is that different REQ_META IOs have different priority.

IOWs, the problem here is that we are trying to infer priority from the request type rather than an actual priority assigned by the submitter. There is no way direct IO has higher priority in a filesystem than log IO tagged with REQ_META as direct IO can require log IO to make progress. Priority is a policy determined by the submitter, not the mechanism doing the throttling.

Can we please move this all over to priorites based on bio->b_ioprio? And then document how the range of priorities are managed, such as:

Quoted 25 lines · Chris Mason

Quoted 17 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Running metadata intensive workloads, I've been seeing the AIL pushing getting stuck on pinned buffers and triggering log forces. The log force is taking a long time to run because the log IO is getting throttled by wbt_wait() - the block layer writeback throttle. It's being throttled because there is a huge amount of metadata writeback going on which is filling the request queue.

IOWs, we have a priority inversion problem here.

Mark the log IO bios with REQ_IDLE so they don't get throttled by the block layer writeback throttle. When we are forcing the CIL, we are likely to need to to tens of log IOs, and they are issued as fast as they can be build and IO completed. Hence REQ_IDLE is appropriate - it's an indication that more IO will follow shortly.

Quoted 6 lines

And because we also set REQ_SYNC, the writeback throttle will no treat log IO the same way it treats direct IO writes - it will not throttle them at all. Hence we solve the priority inversion problem caused by the writeback throttle being unable to distinguish between high priority log IO and background metadata writeback.

Quoted 6 lines

[ cc Jens ]

We spent a lot of time getting rid of these inversions in io.latency (and the new io.cost), where REQ_META just blows through the throttling
and goes into back charging instead.

Which simply reinforces the fact that that request type based throttling is a fundamentally broken architecture.

Quoted 7 lines

It feels awkward to have one set of prio inversion workarounds for io.*
and another for wbt. Jens, should we make an explicit one that doesn't
rely on magic side effects, or just decide that metadata is meta enough
to break all the rules?

The problem isn't REQ_META blows throw the throttling, the problem is that different REQ_META IOs have different priority.

IOWs, the problem here is that we are trying to infer priority from the request type rather than an actual priority assigned by the submitter. There is no way direct IO has higher priority in a filesystem than log IO tagged with REQ_META as direct IO can require log IO to make progress. Priority is a policy determined by the submitter, not the mechanism doing the throttling.

Can we please move this all over to priorites based on bio->b_ioprio? And then document how the range of priorities are managed, such as:

(99 = highest prio to 0 = lowest)

swap out
swap in >90
User hard RT max 89
User hard RT min 80
filesystem max 79
ionice max 60
background data writeback 40
ionice min 20
filesystem min 10
idle 0

So that we can appropriately prioritise different types of kernel internal IO w.r.t user controlled IO priorities? This way we can still tag the bios with the type of data they contain, but we no longer use that to determine whether to throttle that IO or not - throttling/scheduling should be done entirely on a priority basis.

Yes and no. At some point important FS threads have the potential to wait on every single REQ_META IO on the box, so every single REQ_META IO has the potential to create priority inversions.

Tejun reminded me that in a lot of ways, swap is user IO and it's actually fine to have it prioritized at the same level as user IO. We don't want to let a low prio app thrash the drive swapping things in and out all the time, and it's actually fine to make them wait as long as other higher priority processes aren't waiting for the memory. This depends on the cgroup config, so wrt your current patches it probably sounds crazy, but we have a lot of data around this from the fleet.

On 1 Aug 2019, at 19:58, Dave Chinner wrote:
I can't really see bio->b_ioprio working without the rest of the IO controller logic creating a sensible system,

framework to define weights etc. My question is if it's worth trying inside of the wbt code, or if we should just let the metadata go through.

Tejun reminded me that in a lot of ways, swap is user IO and it's actually fine to have it prioritized at the same level as user IO. We

don't want to let a low prio app thrash the drive swapping things in and out all the time,

other higher priority processes aren't waiting for the memory. This depends on the cgroup config, so wrt your current patches it probably sounds crazy, but we have a lot of data around this from the fleet.

Quoted 3 lines · Chris Mason

On 1 Aug 2019, at 19:58, Dave Chinner wrote:
I can't really see bio->b_ioprio working without the rest of the IO controller logic creating a sensible system,

That's exactly the problem we need to solve. The current situation is ... untenable. Regardless of whether the io.latency controller works well, the fact is that the wbt subsystem is active on -all- configurations and the way it "prioritises" is completely broken.

Quoted 4 lines

framework to define weights etc. My question is if it's worth trying inside of the wbt code, or if we should just let the metadata go through.

As I said, that doesn't solve the problem. We /want/ critical journal IO to have higher priority that background metadata writeback. Just ignoring REQ_META doesn't help us there - it just moves the priority inversion to blocking on request queue tags.

Quoted 4 lines

Tejun reminded me that in a lot of ways, swap is user IO and it's actually fine to have it prioritized at the same level as user IO. We

I think that's wrong. Swap *in* could have user priority but swap *out* is global as there is no guarantee that the page being swapped belongs to the user context that is reclaiming memory.

Lots of other user and kernel reclaim contexts may be waiting on that swap to complete, so it's important that swap out is not arbitrarily delayed or susceptible to priority inversions. i.e. swap out must take priority over swap-in and other user IO because that IO may require allocation to make progress via swapping to free "user" file data cached in memory....

Quoted 3 lines

don't want to let a low prio app thrash the drive swapping things in and
out all the time,

Low priority apps will be throttled on *swap in* IO - i.e. by their incoming memory demand. High priority apps should be swapping out low priority app memory if there are shortages - that's what priority defines....

Quoted 3 lines

other higher priority processes aren't waiting for the memory. This depends on the cgroup config, so wrt your current patches it probably sounds crazy, but we have a lot of data around this from the fleet.

I'm not using cgroups.

Core infrastructure needs to work without cgroups being configured to confine everything in userspace to "safe" bounds, and right now just running things in the root cgroup doesn't appear to work very well at all.

Quoted 9 lines · Dave Chinner

Quoted 3 lines · Chris Mason

On 1 Aug 2019, at 19:58, Dave Chinner wrote:
I can't really see bio->b_ioprio working without the rest of the IO controller logic creating a sensible system,

That's exactly the problem we need to solve. The current situation is ... untenable. Regardless of whether the io.latency controller works well, the fact is that the wbt subsystem is active on -all- configurations and the way it "prioritises" is completely broken.

Completely broken is probably a little strong. Before wbt, it was
impossible to do buffered IO without periodically saturating the drive in unexpected ways. We've got a lot of data showing it helping, and it's pretty easy to setup a new A/B experiment to demonstrate it's usefulness in current kernels. But that doesn't mean it's perfect.

Quoted 8 lines

Quoted 3 lines

framework to define weights etc. My question is if it's worth trying inside of the wbt code, or if we should just let the metadata go through.

As I said, that doesn't solve the problem. We /want/ critical journal IO to have higher priority that background metadata writeback. Just ignoring REQ_META doesn't help us there - it just moves the priority inversion to blocking on request queue tags.

Does XFS background metadata IO ever get waited on by critical journal threads?

My understanding is that all of the filesystems do this from time to time. Without a way to bump the priority of throttled background metadata IO, I can't see how to avoid prio inversions without running background metadata at the same prio as all of the critical journal IO.

Quoted 4 lines

Core infrastructure needs to work without cgroups being configured to confine everything in userspace to "safe" bounds, and right now just running things in the root cgroup doesn't appear to work very well at all.

I'm not disagreeing with this part, my real point is there isn't a single answer. It's possible for swap to be critical to the running of the box in some workloads, and totally unimportant in others.

fs/xfs/xfs_buf.c xfs_buftarg_shrink_scan( · line 1677

Quoted 8 lines

fs/xfs/xfs_buf.c +8 -0

+
+		/*
+		 * Account for the buffer memory freed here so memory reclaim
+		 * sees this and not just the xfs_buf slab entry being freed.
+		 */
+		if (current->reclaim_state)
+			current->reclaim_state->reclaimed_pages += bp->b_page_count;
+

I think this wants a mm-layer helper ala:

static inline void shrinker_mark_pages_reclaimed(unsigned long nr_pages)

{
	if (current->reclaim_state)
		current->reclaim_state->reclaimed_pages += nr_pages;
}

plus good documentation on when to use it.

Quoted 7 lines

static inline void shrinker_mark_pages_reclaimed(unsigned long nr_pages)

{
	if (current->reclaim_state)
		current->reclaim_state->reclaimed_pages += nr_pages;
}

plus good documentation on when to use it.

Sure, but that's something for patch 6, not this one :)

From: Dave Chinner <[email protected]>

The XFS inode item slab actually reclaimed by inode shrinker callbacks from the memory reclaim subsystem. These should be marked as reclaimable so the mm subsystem has the full picture of how much memory it can actually reclaim from the XFS slab caches.

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

The XFS inode item slab actually reclaimed by inode shrinker callbacks from the memory reclaim subsystem. These should be marked as reclaimable so the mm subsystem has the full picture of how much memory it can actually reclaim from the XFS slab caches.

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Btw, I wonder if we should just kill off our KM_ZONE_* defined. They just make it a little harder to figure out what is actually going on without a real benefit.

From: Dave Chinner <[email protected]>

Provide an interface to push the AIL to a target LSN and wait for the tail of the log to move past that LSN. This is used to wait for all items older than a specific LSN to either be cleaned (written back) or relogged to a higher LSN in the AIL. The primary use for this is to allow IO free inode reclaim throttling.

Factor the common AIL deletion code that does all the wakeups into a helper so we only have one copy of this somewhat tricky code to interface with all the wakeups necessary when the LSN of the log tail changes.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode_item.c | 12 +------
 fs/xfs/xfs_trans_ail.c  | 69 +++++++++++++++++++++++++++++++++--------
 fs/xfs/xfs_trans_priv.h |  6 +++-
 3 files changed, 62 insertions(+), 25 deletions(-)

fs/xfs/xfs_trans_ail.c +10 -0

index 6ccfd75d3c24..9e3102179221 100644
@@ -654,6 +654,37 @@ xfs_ail_push_all(
 		xfs_ail_push(ailp, threshold_lsn);
 }
 
+/*
+ * Push the AIL to a specific lsn and wait for it to complete.
+ */
+void
+xfs_ail_push_sync(
+	struct xfs_ail		*ailp,
+	xfs_lsn_t		threshold_lsn)
+{
+	struct xfs_log_item	*lip;
+	DEFINE_WAIT(wait);

fs/xfs/xfs_trans_ail.c +13 -0

+		wake_up_process(ailp->ail_task);
+		spin_unlock(&ailp->ail_lock);
+		schedule();
+		spin_lock(&ailp->ail_lock);
+	}
+	spin_unlock(&ailp->ail_lock);
+
+	finish_wait(&ailp->ail_push, &wait);
+}
+
+
 /*
  * Push out all items in the AIL immediately and wait until the AIL is empty.
  */

@@ -764,6 +795,28 @@ xfs_ail_delete_one(
 	return mlip == lip;
 }
 
+void
+xfs_ail_delete_finish(

fs/xfs/xfs_trans_ail.c +11 -4

+	if (!XFS_FORCED_SHUTDOWN(mp))
+		xlog_assign_tail_lsn_locked(mp);
+
+	wake_up_all(&ailp->ail_push);
+	if (list_empty(&ailp->ail_head))
+		wake_up_all(&ailp->ail_empty);
+	spin_unlock(&ailp->ail_lock);
+	xfs_log_space_wake(mp);
+}
+
 /**
  * Remove a log items from the AIL
  *

@@ -789,10 +842,9 @@ void
 xfs_trans_ail_delete(
 	struct xfs_ail		*ailp,
 	struct xfs_log_item	*lip,
-	int			shutdown_type) __releases(ailp->ail_lock)
+	int			shutdown_type)
 {
 	struct xfs_mount	*mp = ailp->ail_mount;
-	bool			mlip_changed;
 
 	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 		spin_unlock(&ailp->ail_lock);

@@ -805,17 +857,7 @@ xfs_trans_ail_delete(
 		return;
 	}
 
-	mlip_changed = xfs_ail_delete_one(ailp, lip);
-	if (mlip_changed) {

fs/xfs/xfs_trans_ail.c +1 -0

 }
 
 int

@@ -834,6 +876,7 @@ xfs_trans_ail_init(
 	spin_lock_init(&ailp->ail_lock);
 	INIT_LIST_HEAD(&ailp->ail_buf_list);
 	init_waitqueue_head(&ailp->ail_empty);
+	init_waitqueue_head(&ailp->ail_push);
 
 	ailp->ail_task = kthread_run(xfsaild, ailp, "xfsaild/%s",
 			ailp->ail_mount->m_fsname);

fs/xfs/xfs_trans_priv.h +5 -1

index 2e073c1c4614..5ab70b9b896f 100644
@@ -61,6 +61,7 @@ struct xfs_ail {
 	int			ail_log_flush;
 	struct list_head	ail_buf_list;
 	wait_queue_head_t	ail_empty;
+	wait_queue_head_t	ail_push;
 };
 
 /*

@@ -92,8 +93,10 @@ xfs_trans_ail_update(
 }
 
 bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
+void xfs_ail_delete_finish(struct xfs_ail *ailp, bool do_tail_update)
+			__releases(ailp->ail_lock);
 void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
-		int shutdown_type) __releases(ailp->ail_lock);
+		int shutdown_type);
 
 static inline void
 xfs_trans_ail_remove(

@@ -111,6 +114,7 @@ xfs_trans_ail_remove(
 }
 
 void			xfs_ail_push(struct xfs_ail *, xfs_lsn_t);
+void			xfs_ail_push_sync(struct xfs_ail *, xfs_lsn_t);
 void			xfs_ail_push_all(struct xfs_ail *);
 void			xfs_ail_push_all_sync(struct xfs_ail *);
 struct xfs_log_item	*xfs_ail_min(struct xfs_ail  *ailp);

Signature · 1 line

2.22.0

Quoted 20 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Provide an interface to push the AIL to a target LSN and wait for the tail of the log to move past that LSN. This is used to wait for all items older than a specific LSN to either be cleaned (written back) or relogged to a higher LSN in the AIL. The primary use for this is to allow IO free inode reclaim throttling.

Factor the common AIL deletion code that does all the wakeups into a helper so we only have one copy of this somewhat tricky code to interface with all the wakeups necessary when the LSN of the log tail changes.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode_item.c | 12 +------
 fs/xfs/xfs_trans_ail.c  | 69 +++++++++++++++++++++++++++++++++--------
 fs/xfs/xfs_trans_priv.h |  6 +++-
 3 files changed, 62 insertions(+), 25 deletions(-)

...

Quoted 22 lines

fs/xfs/xfs_trans_ail.c +14 -0

index 6ccfd75d3c24..9e3102179221 100644
@@ -654,6 +654,37 @@ xfs_ail_push_all(
 		xfs_ail_push(ailp, threshold_lsn);
 }
 
+/*
+ * Push the AIL to a specific lsn and wait for it to complete.
+ */
+void
+xfs_ail_push_sync(
+	struct xfs_ail		*ailp,
+	xfs_lsn_t		threshold_lsn)
+{
+	struct xfs_log_item	*lip;
+	DEFINE_WAIT(wait);
+
+	spin_lock(&ailp->ail_lock);
+	while ((lip = xfs_ail_min(ailp)) != NULL) {
+		prepare_to_wait(&ailp->ail_push, &wait, TASK_UNINTERRUPTIBLE);

fs/xfs/xfs_trans_ail.c xfs_ail_push_all( · line 676

Quoted 6 lines

fs/xfs/xfs_trans_ail.c +6 -0

+		if (XFS_FORCED_SHUTDOWN(ailp->ail_mount) ||
+		    XFS_LSN_CMP(threshold_lsn, lip->li_lsn) <= 0)
+			break;
+		/* XXX: cmpxchg? */
+		while (XFS_LSN_CMP(threshold_lsn, ailp->ail_target) > 0)
+			xfs_trans_ail_copy_lsn(ailp, &ailp->ail_target, &threshold_lsn);

Why the need to repeatedly copy the ail_target like this? If the push

target only ever moves forward, we should only need to do this once at the start of the function. In fact I'm kind of wondering why we can't just call xfs_ail_push(). If we check the tail item after grabbing the spin lock, we should be able to avoid any races with the waker, no?

Quoted 11 lines

fs/xfs/xfs_trans_ail.c +11 -0

+void
+xfs_ail_delete_finish(
+	struct xfs_ail		*ailp,
+	bool			do_tail_update) __releases(ailp->ail_lock)
+{
+	struct xfs_mount	*mp = ailp->ail_mount;
+
+	if (!do_tail_update) {
+		spin_unlock(&ailp->ail_lock);
+		return;
+	}

fs/xfs/xfs_trans_ail.c xfs_ail_push_all( · line 688

Quoted 1 line

fs/xfs/xfs_trans_ail.c +1 -0

+

Hmm.. so while what we really care about here are tail updates, this logic is currently driven by removing the min ail log item. That seems like a lot of potential churn if we're waking the pusher on every object written back covered by a single log record / checkpoint. Perhaps we should implement a bit more coarse wakeup logic such as only when the tail lsn actually changes, for example?

FWIW, it also doesn't look like you've handled the case of relogged items moving the tail forward anywhere that I can see, so we might be missing some wakeups here. See xfs_trans_ail_update_bulk() for additional AIL manipulation.

Quoted 44 lines · Brian Foster

Quoted 20 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Provide an interface to push the AIL to a target LSN and wait for the tail of the log to move past that LSN. This is used to wait for all items older than a specific LSN to either be cleaned (written back) or relogged to a higher LSN in the AIL. The primary use for this is to allow IO free inode reclaim throttling.

Factor the common AIL deletion code that does all the wakeups into a helper so we only have one copy of this somewhat tricky code to interface with all the wakeups necessary when the LSN of the log tail changes.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_inode_item.c | 12 +------
 fs/xfs/xfs_trans_ail.c  | 69 +++++++++++++++++++++++++++++++++--------
 fs/xfs/xfs_trans_priv.h |  6 +++-
 3 files changed, 62 insertions(+), 25 deletions(-)

...

Quoted 22 lines

fs/xfs/xfs_trans_ail.c +14 -0

index 6ccfd75d3c24..9e3102179221 100644
@@ -654,6 +654,37 @@ xfs_ail_push_all(
 		xfs_ail_push(ailp, threshold_lsn);
 }
 
+/*
+ * Push the AIL to a specific lsn and wait for it to complete.
+ */
+void
+xfs_ail_push_sync(
+	struct xfs_ail		*ailp,
+	xfs_lsn_t		threshold_lsn)
+{
+	struct xfs_log_item	*lip;
+	DEFINE_WAIT(wait);
+
+	spin_lock(&ailp->ail_lock);
+	while ((lip = xfs_ail_min(ailp)) != NULL) {
+		prepare_to_wait(&ailp->ail_push, &wait, TASK_UNINTERRUPTIBLE);

Quoted 8 lines

fs/xfs/xfs_trans_ail.c xfs_ail_push_all( · line 676

Quoted 6 lines

fs/xfs/xfs_trans_ail.c +6 -0

+		if (XFS_FORCED_SHUTDOWN(ailp->ail_mount) ||
+		    XFS_LSN_CMP(threshold_lsn, lip->li_lsn) <= 0)
+			break;
+		/* XXX: cmpxchg? */
+		while (XFS_LSN_CMP(threshold_lsn, ailp->ail_target) > 0)
+			xfs_trans_ail_copy_lsn(ailp, &ailp->ail_target, &threshold_lsn);

Why the need to repeatedly copy the ail_target like this? If the push

It's a hack because the other updates are done unlocked and this doesn't contain the memroy barriers needed to make it correct and race free.

Hence the comment "XXX: cmpxchg" to ensure that:

a) we only ever move the target forwards;
b) we resolve update races in an obvious, simple manner; and
c) we can get rid of the possibly incorrect memory barriers around this (unlocked) update.

RFC. WIP. :)

Quoted 4 lines

target only ever moves forward, we should only need to do this once at the start of the function. In fact I'm kind of wondering why we can't just call xfs_ail_push(). If we check the tail item after grabbing the spin lock, we should be able to avoid any races with the waker, no?

I didn't use xfs_ail_push() because of having to prepare to wait between determining if the AIL is empty and checking if we need to update the target.

I also didn't want to affect the existing xfs_ail_push() as I was modifying the xfs_ail_push_sync() code to do what was needed. Eventually they can probably come back together, but for now I'm not 100% sure that the code is correct and race free.

Quoted 11 lines

Quoted 11 lines

fs/xfs/xfs_trans_ail.c +11 -0

+void
+xfs_ail_delete_finish(
+	struct xfs_ail		*ailp,
+	bool			do_tail_update) __releases(ailp->ail_lock)
+{
+	struct xfs_mount	*mp = ailp->ail_mount;
+
+	if (!do_tail_update) {
+		spin_unlock(&ailp->ail_lock);
+		return;
+	}

Quoted 8 lines

fs/xfs/xfs_trans_ail.c xfs_ail_push_all( · line 688

Quoted 1 line

fs/xfs/xfs_trans_ail.c +1 -0

+

Hmm.. so while what we really care about here are tail updates, this logic is currently driven by removing the min ail log item. That seems like a lot of potential churn if we're waking the pusher on every object written back covered by a single log record / checkpoint. Perhaps we should implement a bit more coarse wakeup logic such as only when the tail lsn actually changes, for example?

You mean the next patch?

Quoted 4 lines

FWIW, it also doesn't look like you've handled the case of relogged items moving the tail forward anywhere that I can see, so we might be missing some wakeups here. See xfs_trans_ail_update_bulk() for additional AIL manipulation.

Good catch. That might be the race the next patch exposes :)

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]