mm, xfs: non-blocking inode reclaim

From: Dave Chinner <[email protected]>

In certain situations the background CIL push can be indefinitely delayed. While we have workarounds from the obvious cases now, it doesn't solve the underlying issue. This issue is that there is no upper limit on the CIL where we will either force or wait for a background push to start, hence allowing the CIL to grow without bound until it consumes all log space.

To fix this, add a new wait queue to the CIL which allows background pushes to wait for the CIL context to be switched out. This happens when the push starts, so it will allow us to block incoming transaction commit completion until the push has started. This will only affect processes that are running modifications, and only when the CIL threshold has been significantly overrun.

This has no apparent impact on performance, and doesn't even trigger until over 45 million inodes had been created in a 16-way fsmark test on a 2GB log. That was limiting at 64MB of log space used, so the active CIL size is only about 3% of the total log in that case. The concurrent removal of those files did not trigger the background sleep at all.

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

Reviewed-by Brian Foster <[email protected]>

---

 fs/xfs/xfs_log_cil.c  | 37 +++++++++++++++++++++++++++++++++----
 fs/xfs/xfs_log_priv.h | 24 ++++++++++++++++++++++++
 fs/xfs/xfs_trace.h    |  1 +
 3 files changed, 58 insertions(+), 4 deletions(-)

fs/xfs/xfs_log_cil.c +33 -4

index a1204424a938..fc3f8e849dec 100644
@@ -670,6 +670,11 @@ xlog_cil_push(
 	push_seq = cil->xc_push_seq;
 	ASSERT(push_seq <= ctx->sequence);
 
+	/*
+	 * Wake up any background push waiters now this context is being pushed.
+	 */
+	wake_up_all(&ctx->push_wait);
+
 	/*
 	 * Check if we've anything to push. If there is nothing, then we don't
 	 * move on to a new sequence number and so we have to be able to push

@@ -746,6 +751,7 @@ xlog_cil_push(
 	 */
 	INIT_LIST_HEAD(&new_ctx->committing);
 	INIT_LIST_HEAD(&new_ctx->busy_extents);
+	init_waitqueue_head(&new_ctx->push_wait);
 	new_ctx->sequence = ctx->sequence + 1;
 	new_ctx->cil = cil;
 	cil->xc_ctx = new_ctx;

@@ -900,7 +906,7 @@ xlog_cil_push_work(
  */
 static void
 xlog_cil_push_background(
-	struct xlog	*log)
+	struct xlog	*log) __releases(cil->xc_ctx_lock)
 {
 	struct xfs_cil	*cil = log->l_cilp;
 

@@ -914,14 +920,36 @@ xlog_cil_push_background(
 	 * don't do a background push if we haven't used up all the
 	 * space available yet.
 	 */
-	if (cil->xc_ctx->space_used < XLOG_CIL_SPACE_LIMIT(log))
+	if (cil->xc_ctx->space_used < XLOG_CIL_SPACE_LIMIT(log)) {
+		up_read(&cil->xc_ctx_lock);
 		return;
+	}
 
 	spin_lock(&cil->xc_push_lock);
 	if (cil->xc_push_seq < cil->xc_current_sequence) {
 		cil->xc_push_seq = cil->xc_current_sequence;
 		queue_work(log->l_mp->m_cil_workqueue, &cil->xc_push_work);
 	}
+
+	/*
+	 * Drop the context lock now, we can't hold that if we need to sleep
+	 * because we are over the blocking threshold. The push_lock is still
+	 * held, so blocking threshold sleep/wakeup is still correctly
+	 * serialised here.
+	 */
+	up_read(&cil->xc_ctx_lock);
+
+	/*
+	 * If we are well over the space limit, throttle the work that is being
+	 * done until the push work on this context has begun.
+	 */
+	if (cil->xc_ctx->space_used >= XLOG_CIL_BLOCKING_SPACE_LIMIT(log)) {
+		trace_xfs_log_cil_wait(log, cil->xc_ctx->ticket);
+		ASSERT(cil->xc_ctx->space_used < log->l_logsize);
+		xlog_wait(&cil->xc_ctx->push_wait, &cil->xc_push_lock);
+		return;
+	}
+
 	spin_unlock(&cil->xc_push_lock);
 
 }

@@ -1038,9 +1066,9 @@ xfs_log_commit_cil(
 		if (lip->li_ops->iop_committing)
 			lip->li_ops->iop_committing(lip, xc_commit_lsn);
 	}
-	xlog_cil_push_background(log);
 
-	up_read(&cil->xc_ctx_lock);
+	/* xlog_cil_push_background() releases cil->xc_ctx_lock */
+	xlog_cil_push_background(log);
 }
 
 /*

@@ -1199,6 +1227,7 @@ xlog_cil_init(
 
 	INIT_LIST_HEAD(&ctx->committing);
 	INIT_LIST_HEAD(&ctx->busy_extents);
+	init_waitqueue_head(&ctx->push_wait);
 	ctx->sequence = 1;
 	ctx->cil = cil;
 	cil->xc_ctx = ctx;

fs/xfs/xfs_log_priv.h +24 -0

index abd382cfffe3..1cc5333a3f6a 100644
@@ -242,6 +242,7 @@ struct xfs_cil_ctx {
 	struct xfs_log_vec	*lv_chain;	/* logvecs being pushed */
 	struct list_head	iclog_entry;
 	struct list_head	committing;	/* ctx committing list */
+	wait_queue_head_t	push_wait;	/* background push throttle */
 	struct work_struct	discard_endio_work;
 };
 

@@ -339,10 +340,33 @@ struct xfs_cil {
  *   buffer window (32MB) as measurements have shown this to be roughly the
  *   point of diminishing performance increases under highly concurrent
  *   modification workloads.
+ *
+ * To prevent the CIL from overflowing upper commit size bounds, we introduce a
+ * new threshold at which we block committing transactions until the background
+ * CIL commit commences and switches to a new context. While this is not a hard
+ * limit, it forces the process committing a transaction to the CIL to block and
+ * yeild the CPU, giving the CIL push work a chance to be scheduled and start
+ * work. This prevents a process running lots of transactions from overfilling
+ * the CIL because it is not yielding the CPU. We set the blocking limit at
+ * twice the background push space threshold so we keep in line with the AIL
+ * push thresholds.
+ *
+ * Note: this is not a -hard- limit as blocking is applied after the transaction
+ * is inserted into the CIL and the push has been triggered. It is largely a
+ * throttling mechanism that allows the CIL push to be scheduled and run. A hard
+ * limit will be difficult to implement without introducing global serialisation
+ * in the CIL commit fast path, and it's not at all clear that we actually need
+ * such hard limits given the ~7 years we've run without a hard limit before
+ * finding the first situation where a checkpoint size overflow actually
+ * occurred. Hence the simple throttle, and an ASSERT check to tell us that
+ * we've overrun the max size.
  */
 #define XLOG_CIL_SPACE_LIMIT(log)	\
 	min_t(int, (log)->l_logsize >> 3, BBTOB(XLOG_TOTAL_REC_SHIFT(log)) << 4)
 
+#define XLOG_CIL_BLOCKING_SPACE_LIMIT(log)	\
+	(XLOG_CIL_SPACE_LIMIT(log) * 2)
+
 /*
  * ticket grant locks, queues and accounting have their own cachlines
  * as these are quite hot and can be operated on concurrently.

fs/xfs/xfs_trace.h +1 -0

index c13bb3655e48..d3635d1e3de6 100644
@@ -1011,6 +1011,7 @@ DEFINE_LOGGRANT_EVENT(xfs_log_regrant_reserve_sub);
 DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_enter);
 DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_exit);
 DEFINE_LOGGRANT_EVENT(xfs_log_ungrant_sub);
+DEFINE_LOGGRANT_EVENT(xfs_log_cil_wait);
 
 DECLARE_EVENT_CLASS(xfs_log_item_class,
 	TP_PROTO(struct xfs_log_item *lip),

Signature · 1 line

2.24.0.rc0

Quoted 23 lines · Dave Chinner

From: Dave Chinner <[email protected]>

In certain situations the background CIL push can be indefinitely delayed. While we have workarounds from the obvious cases now, it doesn't solve the underlying issue. This issue is that there is no upper limit on the CIL where we will either force or wait for a background push to start, hence allowing the CIL to grow without bound until it consumes all log space.

To fix this, add a new wait queue to the CIL which allows background pushes to wait for the CIL context to be switched out. This happens when the push starts, so it will allow us to block incoming transaction commit completion until the push has started. This will only affect processes that are running modifications, and only when the CIL threshold has been significantly overrun.

This has no apparent impact on performance, and doesn't even trigger until over 45 million inodes had been created in a 16-way fsmark test on a 2GB log. That was limiting at 64MB of log space used, so the active CIL size is only about 3% of the total log in that case. The concurrent removal of those files did not trigger the background sleep at all.

Quoted 3 lines

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

Reviewed-by Brian Foster <[email protected]>

---

I don't recall posting an R-b tag for this one...

That said, I think my only outstanding feedback (side discussion aside) was the code factoring in xlog_cil_push_background().

Quoted 24 lines · Brian Foster

Quoted 23 lines · Dave Chinner

From: Dave Chinner <[email protected]>

In certain situations the background CIL push can be indefinitely delayed. While we have workarounds from the obvious cases now, it doesn't solve the underlying issue. This issue is that there is no upper limit on the CIL where we will either force or wait for a background push to start, hence allowing the CIL to grow without bound until it consumes all log space.

To fix this, add a new wait queue to the CIL which allows background pushes to wait for the CIL context to be switched out. This happens when the push starts, so it will allow us to block incoming transaction commit completion until the push has started. This will only affect processes that are running modifications, and only when the CIL threshold has been significantly overrun.

This has no apparent impact on performance, and doesn't even trigger until over 45 million inodes had been created in a 16-way fsmark test on a 2GB log. That was limiting at 64MB of log space used, so the active CIL size is only about 3% of the total log in that case. The concurrent removal of those files did not trigger the background sleep at all.

Quoted 5 lines

Quoted 3 lines

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

Reviewed-by Brian Foster <[email protected]>

---

I don't recall posting an R-b tag for this one...

Argh, sorry. I must have screwed up transcribing them from the mailing list.

Quoted 2 lines

That said, I think my only outstanding feedback (side discussion aside) was the code factoring in xlog_cil_push_background().

I'll go back and look at that, 'cause clearly I was looking at the wrong patch when I screwed up the rvb tag...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Dave Chinner <[email protected]>

The buffer cache shrinker frees more than just the xfs_buf slab objects - it also frees the pages attached to the buffers. Make sure the memory reclaim code accounts for this memory being freed correctly, similar to how the inode shrinker accounts for pages freed from the page cache due to mapping invalidation.

We also need to make sure that the mm subsystem knows these are reclaimable objects. We provide the memory reclaim subsystem with a a shrinker to reclaim xfs_bufs, so we should really mark the slab that way.

We also have a lot of xfs_bufs in a busy system, spread them around like we do inodes.

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

---

 fs/xfs/xfs_buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_buf.c +5 -1

index 1e63dd3d1257..d34e5d2edacd 100644
@@ -324,6 +324,9 @@ xfs_buf_free(
 
 			__free_page(page);
 		}
+		if (current->reclaim_state)
+			current->reclaim_state->reclaimed_slab +=
+							bp->b_page_count;
 	} else if (bp->b_flags & _XBF_KMEM)
 		kmem_free(bp->b_addr);
 	_xfs_buf_free_pages(bp);

@@ -2061,7 +2064,8 @@ int __init
 xfs_buf_init(void)
 {
 	xfs_buf_zone = kmem_zone_init_flags(sizeof(xfs_buf_t), "xfs_buf",
-						KM_ZONE_HWALIGN, NULL);
+			KM_ZONE_HWALIGN | KM_ZONE_SPREAD | KM_ZONE_RECLAIM,
+			NULL);
 	if (!xfs_buf_zone)
 		goto out;
 

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

The buffer cache shrinker frees more than just the xfs_buf slab objects - it also frees the pages attached to the buffers. Make sure the memory reclaim code accounts for this memory being freed correctly, similar to how the inode shrinker accounts for pages freed from the page cache due to mapping invalidation.

We also need to make sure that the mm subsystem knows these are reclaimable objects. We provide the memory reclaim subsystem with a a shrinker to reclaim xfs_bufs, so we should really mark the slab that way.

We also have a lot of xfs_bufs in a busy system, spread them around like we do inodes.

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

---
 fs/xfs/xfs_buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_buf.c +3 -0

index 1e63dd3d1257..d34e5d2edacd 100644
@@ -324,6 +324,9 @@ xfs_buf_free(
 
 			__free_page(page);
 		}
+		if (current->reclaim_state)
+			current->reclaim_state->reclaimed_slab +=
+							bp->b_page_count;
 	} else if (bp->b_flags & _XBF_KMEM)
 		kmem_free(bp->b_addr);
 	_xfs_buf_free_pages(bp);

fs/xfs/xfs_buf.c +1 -0

+			NULL);
 	if (!xfs_buf_zone)
 		goto out;
 

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

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

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_inode_item.c | 12 +----------
 fs/xfs/xfs_trans_ail.c  | 48 ++++++++++++++++++++++-------------------
 fs/xfs/xfs_trans_priv.h |  4 +++-
 3 files changed, 30 insertions(+), 34 deletions(-)

fs/xfs/xfs_inode_item.c +1 -11

index bb8f076805b9..ab12e526540a 100644
@@ -743,17 +743,7 @@ xfs_iflush_done(
 				xfs_clear_li_failed(blip);
 			}
 		}
-
-		if (mlip_changed) {
-			if (!XFS_FORCED_SHUTDOWN(ailp->ail_mount))
-				xlog_assign_tail_lsn_locked(ailp->ail_mount);
-			if (list_empty(&ailp->ail_head))
-				wake_up_all(&ailp->ail_empty);
-		}
-		spin_unlock(&ailp->ail_lock);
-
-		if (mlip_changed)
-			xfs_log_space_wake(ailp->ail_mount);
+		xfs_ail_update_finish(ailp, mlip_changed);
 	}
 
 	/*

fs/xfs/xfs_trans_ail.c +21 -0

index 6ccfd75d3c24..656819523bbd 100644
@@ -678,6 +678,27 @@ xfs_ail_push_all_sync(
 	finish_wait(&ailp->ail_empty, &wait);
 }
 
+void
+xfs_ail_update_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;
+	}
+
+	if (!XFS_FORCED_SHUTDOWN(mp))
+		xlog_assign_tail_lsn_locked(mp);
+
+	if (list_empty(&ailp->ail_head))
+		wake_up_all(&ailp->ail_empty);
+	spin_unlock(&ailp->ail_lock);
+	xfs_log_space_wake(mp);
+}
+
 /*
  * xfs_trans_ail_update - bulk AIL insertion operation.
  *

fs/xfs/xfs_trans_ail.c +5 -16

-	} else {
-		spin_unlock(&ailp->ail_lock);
-	}
+	xfs_ail_update_finish(ailp, mlip_changed);
 }
 
 bool

@@ -789,10 +802,10 @@ 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;
+	bool			need_update;
 
 	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 		spin_unlock(&ailp->ail_lock);

@@ -805,17 +818,8 @@ xfs_trans_ail_delete(
 		return;
 	}
 
-	mlip_changed = xfs_ail_delete_one(ailp, lip);
-	if (mlip_changed) {
-		if (!XFS_FORCED_SHUTDOWN(mp))
-			xlog_assign_tail_lsn_locked(mp);
-		if (list_empty(&ailp->ail_head))
-			wake_up_all(&ailp->ail_empty);
-	}
-
-	spin_unlock(&ailp->ail_lock);
-	if (mlip_changed)
-		xfs_log_space_wake(ailp->ail_mount);
+	need_update = xfs_ail_delete_one(ailp, lip);
+	xfs_ail_update_finish(ailp, need_update);
 }
 
 int

fs/xfs/xfs_trans_priv.h +3 -1

index 2e073c1c4614..64ffa746730e 100644
@@ -92,8 +92,10 @@ xfs_trans_ail_update(
 }
 
 bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
+void xfs_ail_update_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(

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

We currently wake anything waiting on the log tail to move whenever the log item at the tail of the log is removed. Historically this was fine behaviour because there were very few items at any given LSN. But with delayed logging, there may be thousands of items at any given LSN, and we can't move the tail until they are all gone.

Hence if we are removing them in near tail-first order, we might be waking up processes waiting on the tail LSN to change (e.g. log space waiters) repeatedly without them being able to make progress. This also occurs with the new sync push waiters, and can result in thousands of spurious wakeups every second when under heavy direct reclaim pressure.

To fix this, check that the tail LSN has actually changed on the AIL before triggering wakeups. This will reduce the number of spurious wakeups when doing bulk AIL removal and make this code much more efficient.

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

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_inode_item.c | 18 ++++++++++----
 fs/xfs/xfs_trans_ail.c  | 52 ++++++++++++++++++++++++++++-------------
 fs/xfs/xfs_trans_priv.h |  4 ++--
 3 files changed, 51 insertions(+), 23 deletions(-)

fs/xfs/xfs_inode_item.c +13 -5

index ab12e526540a..79ffe6dff115 100644
@@ -731,19 +731,27 @@ xfs_iflush_done(
 	 * holding the lock before removing the inode from the AIL.
 	 */
 	if (need_ail) {
-		bool			mlip_changed = false;
+		xfs_lsn_t	tail_lsn = 0;
 
 		/* this is an opencoded batch version of xfs_trans_ail_delete */
 		spin_lock(&ailp->ail_lock);
 		list_for_each_entry(blip, &tmp, li_bio_list) {
 			if (INODE_ITEM(blip)->ili_logged &&
-			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
-				mlip_changed |= xfs_ail_delete_one(ailp, blip);
-			else {
+			    blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn) {
+				/*
+				 * xfs_ail_update_finish() only cares about the
+				 * lsn of the first tail item removed, any
+				 * others will be at the same or higher lsn so
+				 * we just ignore them.
+				 */
+				xfs_lsn_t lsn = xfs_ail_delete_one(ailp, blip);
+				if (!tail_lsn && lsn)
+					tail_lsn = lsn;
+			} else {
 				xfs_clear_li_failed(blip);
 			}
 		}
-		xfs_ail_update_finish(ailp, mlip_changed);
+		xfs_ail_update_finish(ailp, tail_lsn);
 	}
 
 	/*

fs/xfs/xfs_trans_ail.c +36 -16

index 656819523bbd..685a21cd24c0 100644
@@ -108,17 +108,25 @@ xfs_ail_next(
  * We need the AIL lock in order to get a coherent read of the lsn of the last
  * item in the AIL.
  */
+static xfs_lsn_t
+__xfs_ail_min_lsn(
+	struct xfs_ail		*ailp)
+{
+	struct xfs_log_item	*lip = xfs_ail_min(ailp);
+
+	if (lip)
+		return lip->li_lsn;
+	return 0;
+}
+
 xfs_lsn_t
 xfs_ail_min_lsn(
 	struct xfs_ail		*ailp)
 {
-	xfs_lsn_t		lsn = 0;
-	struct xfs_log_item	*lip;
+	xfs_lsn_t		lsn;
 
 	spin_lock(&ailp->ail_lock);
-	lip = xfs_ail_min(ailp);
-	if (lip)
-		lsn = lip->li_lsn;
+	lsn = __xfs_ail_min_lsn(ailp);
 	spin_unlock(&ailp->ail_lock);
 
 	return lsn;

@@ -681,11 +689,12 @@ xfs_ail_push_all_sync(
 void
 xfs_ail_update_finish(
 	struct xfs_ail		*ailp,
-	bool			do_tail_update) __releases(ailp->ail_lock)
+	xfs_lsn_t		old_lsn) __releases(ailp->ail_lock)
 {
 	struct xfs_mount	*mp = ailp->ail_mount;
 
-	if (!do_tail_update) {
+	/* if the tail lsn hasn't changed, don't do updates or wakeups. */
+	if (!old_lsn || old_lsn == __xfs_ail_min_lsn(ailp)) {
 		spin_unlock(&ailp->ail_lock);
 		return;
 	}

@@ -730,7 +739,7 @@ xfs_trans_ail_update_bulk(
 	xfs_lsn_t		lsn) __releases(ailp->ail_lock)
 {
 	struct xfs_log_item	*mlip;
-	int			mlip_changed = 0;
+	xfs_lsn_t		tail_lsn = 0;
 	int			i;
 	LIST_HEAD(tmp);
 

@@ -745,9 +754,10 @@ xfs_trans_ail_update_bulk(
 				continue;
 
 			trace_xfs_ail_move(lip, lip->li_lsn, lsn);
+			if (mlip == lip && !tail_lsn)
+				tail_lsn = lip->li_lsn;
+
 			xfs_ail_delete(ailp, lip);
-			if (mlip == lip)
-				mlip_changed = 1;
 		} else {
 			trace_xfs_ail_insert(lip, 0, lsn);
 		}

@@ -758,15 +768,23 @@ xfs_trans_ail_update_bulk(
 	if (!list_empty(&tmp))
 		xfs_ail_splice(ailp, cur, &tmp, lsn);
 
-	xfs_ail_update_finish(ailp, mlip_changed);
+	xfs_ail_update_finish(ailp, tail_lsn);
 }
 
-bool
+/*
+ * Delete one log item from the AIL.
+ *
+ * If this item was at the tail of the AIL, return the LSN of the log item so
+ * that we can use it to check if the LSN of the tail of the log has moved
+ * when finishing up the AIL delete process in xfs_ail_update_finish().
+ */
+xfs_lsn_t
 xfs_ail_delete_one(
 	struct xfs_ail		*ailp,
 	struct xfs_log_item	*lip)
 {
 	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
+	xfs_lsn_t		lsn = lip->li_lsn;
 
 	trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
 	xfs_ail_delete(ailp, lip);

@@ -774,7 +792,9 @@ xfs_ail_delete_one(
 	clear_bit(XFS_LI_IN_AIL, &lip->li_flags);
 	lip->li_lsn = 0;
 
-	return mlip == lip;
+	if (mlip == lip)
+		return lsn;
+	return 0;
 }
 
 /**

@@ -805,7 +825,7 @@ xfs_trans_ail_delete(
 	int			shutdown_type)
 {
 	struct xfs_mount	*mp = ailp->ail_mount;
-	bool			need_update;
+	xfs_lsn_t		tail_lsn;
 
 	if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) {
 		spin_unlock(&ailp->ail_lock);

@@ -818,8 +838,8 @@ xfs_trans_ail_delete(
 		return;
 	}
 
-	need_update = xfs_ail_delete_one(ailp, lip);
-	xfs_ail_update_finish(ailp, need_update);
+	tail_lsn = xfs_ail_delete_one(ailp, lip);
+	xfs_ail_update_finish(ailp, tail_lsn);
 }
 
 int

fs/xfs/xfs_trans_priv.h +2 -2

index 64ffa746730e..35655eac01a6 100644
@@ -91,8 +91,8 @@ xfs_trans_ail_update(
 	xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
 }
 
-bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
-void xfs_ail_update_finish(struct xfs_ail *ailp, bool do_tail_update)
+xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
+void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn)
 			__releases(ailp->ail_lock);
 void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
 		int shutdown_type);

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

There's lots of indent in this code which makes it a bit hard to follow. We are also going to completely rework the inode lookup code as part of the inode reclaim rework, so factor out the inode lookup code from the inode cluster freeing code.

Based on prototype code from Christoph Hellwig.

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

---

 fs/xfs/xfs_inode.c | 152 +++++++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 68 deletions(-)

fs/xfs/xfs_inode.c +84 -68

index e9e4f444f8ce..33edb18098ca 100644
@@ -2516,6 +2516,88 @@ xfs_iunlink_remove(
 	return error;
 }
 
+/*
+ * Look up the inode number specified and mark it stale if it is found. If it is
+ * dirty, return the inode so it can be attached to the cluster buffer so it can
+ * be processed appropriately when the cluster free transaction completes.
+ */
+static struct xfs_inode *
+xfs_ifree_get_one_inode(
+	struct xfs_perag	*pag,
+	struct xfs_inode	*free_ip,
+	int			inum)
+{
+	struct xfs_mount	*mp = pag->pag_mount;
+	struct xfs_inode	*ip;
+
+retry:
+	rcu_read_lock();
+	ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
+
+	/* Inode not in memory, nothing to do */
+	if (!ip)
+		goto out_rcu_unlock;
+
+	/*
+	 * because this is an RCU protected lookup, we could find a recently
+	 * freed or even reallocated inode during the lookup. We need to check
+	 * under the i_flags_lock for a valid inode here. Skip it if it is not
+	 * valid, the wrong inode or stale.
+	 */
+	spin_lock(&ip->i_flags_lock);
+	if (ip->i_ino != inum || __xfs_iflags_test(ip, XFS_ISTALE)) {
+		spin_unlock(&ip->i_flags_lock);
+		goto out_rcu_unlock;
+	}
+	spin_unlock(&ip->i_flags_lock);
+
+	/*
+	 * Don't try to lock/unlock the current inode, but we _cannot_ skip the
+	 * other inodes that we did not find in the list attached to the buffer
+	 * and are not already marked stale. If we can't lock it, back off and
+	 * retry.
+	 */
+	if (ip != free_ip) {
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
+			rcu_read_unlock();
+			delay(1);
+			goto retry;
+		}
+
+		/*
+		 * Check the inode number again in case we're racing with
+		 * freeing in xfs_reclaim_inode().  See the comments in that
+		 * function for more information as to why the initial check is
+		 * not sufficient.
+		 */
+		if (ip->i_ino != inum) {
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+			goto out_rcu_unlock;
+		}
+	}
+	rcu_read_unlock();
+
+	xfs_iflock(ip);
+	xfs_iflags_set(ip, XFS_ISTALE);
+
+	/*
+	 * We don't need to attach clean inodes or those only with unlogged
+	 * changes (which we throw away, anyway).
+	 */
+	if (!ip->i_itemp || xfs_inode_clean(ip)) {
+		ASSERT(ip != free_ip);
+		xfs_ifunlock(ip);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		goto out_no_inode;
+	}
+	return ip;
+
+out_rcu_unlock:
+	rcu_read_unlock();
+out_no_inode:
+	return NULL;
+}
+
 /*
  * A big issue when freeing the inode cluster is that we _cannot_ skip any
  * inodes that are in memory - they all must be marked stale and attached to

@@ -2616,77 +2698,11 @@ xfs_ifree_cluster(
 		 * even trying to lock them.
 		 */
 		for (i = 0; i < igeo->inodes_per_cluster; i++) {
-retry:
-			rcu_read_lock();
-			ip = radix_tree_lookup(&pag->pag_ici_root,
-					XFS_INO_TO_AGINO(mp, (inum + i)));
-
-			/* Inode not in memory, nothing to do */
-			if (!ip) {
-				rcu_read_unlock();
+			ip = xfs_ifree_get_one_inode(pag, free_ip, inum + i);
+			if (!ip)
 				continue;
-			}
-
-			/*
-			 * because this is an RCU protected lookup, we could
-			 * find a recently freed or even reallocated inode
-			 * during the lookup. We need to check under the
-			 * i_flags_lock for a valid inode here. Skip it if it
-			 * is not valid, the wrong inode or stale.
-			 */
-			spin_lock(&ip->i_flags_lock);
-			if (ip->i_ino != inum + i ||
-			    __xfs_iflags_test(ip, XFS_ISTALE)) {
-				spin_unlock(&ip->i_flags_lock);
-				rcu_read_unlock();
-				continue;
-			}
-			spin_unlock(&ip->i_flags_lock);
-
-			/*
-			 * Don't try to lock/unlock the current inode, but we
-			 * _cannot_ skip the other inodes that we did not find
-			 * in the list attached to the buffer and are not
-			 * already marked stale. If we can't lock it, back off
-			 * and retry.
-			 */
-			if (ip != free_ip) {
-				if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
-					rcu_read_unlock();
-					delay(1);
-					goto retry;
-				}
-
-				/*
-				 * Check the inode number again in case we're
-				 * racing with freeing in xfs_reclaim_inode().
-				 * See the comments in that function for more
-				 * information as to why the initial check is
-				 * not sufficient.
-				 */
-				if (ip->i_ino != inum + i) {
-					xfs_iunlock(ip, XFS_ILOCK_EXCL);
-					rcu_read_unlock();
-					continue;
-				}
-			}
-			rcu_read_unlock();
 
-			xfs_iflock(ip);
-			xfs_iflags_set(ip, XFS_ISTALE);
-
-			/*
-			 * we don't need to attach clean inodes or those only
-			 * with unlogged changes (which we throw away, anyway).
-			 */
 			iip = ip->i_itemp;
-			if (!iip || xfs_inode_clean(ip)) {
-				ASSERT(ip != free_ip);
-				xfs_ifunlock(ip);
-				xfs_iunlock(ip, XFS_ILOCK_EXCL);
-				continue;
-			}
-
 			iip->ili_last_fields = iip->ili_fields;
 			iip->ili_fields = 0;
 			iip->ili_fsync_fields = 0;

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

There's lots of indent in this code which makes it a bit hard to follow. We are also going to completely rework the inode lookup code as part of the inode reclaim rework, so factor out the inode lookup code from the inode cluster freeing code.

Based on prototype code from Christoph Hellwig.

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

---
 fs/xfs/xfs_inode.c | 152 +++++++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 68 deletions(-)

fs/xfs/xfs_inode.c +84 -68

index e9e4f444f8ce..33edb18098ca 100644
@@ -2516,6 +2516,88 @@ xfs_iunlink_remove(
 	return error;
 }
 
+/*
+ * Look up the inode number specified and mark it stale if it is found. If it is
+ * dirty, return the inode so it can be attached to the cluster buffer so it can
+ * be processed appropriately when the cluster free transaction completes.
+ */
+static struct xfs_inode *
+xfs_ifree_get_one_inode(
+	struct xfs_perag	*pag,
+	struct xfs_inode	*free_ip,
+	int			inum)
+{
+	struct xfs_mount	*mp = pag->pag_mount;
+	struct xfs_inode	*ip;
+
+retry:
+	rcu_read_lock();
+	ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
+
+	/* Inode not in memory, nothing to do */
+	if (!ip)
+		goto out_rcu_unlock;
+
+	/*
+	 * because this is an RCU protected lookup, we could find a recently
+	 * freed or even reallocated inode during the lookup. We need to check
+	 * under the i_flags_lock for a valid inode here. Skip it if it is not
+	 * valid, the wrong inode or stale.
+	 */
+	spin_lock(&ip->i_flags_lock);
+	if (ip->i_ino != inum || __xfs_iflags_test(ip, XFS_ISTALE)) {
+		spin_unlock(&ip->i_flags_lock);
+		goto out_rcu_unlock;
+	}
+	spin_unlock(&ip->i_flags_lock);
+
+	/*
+	 * Don't try to lock/unlock the current inode, but we _cannot_ skip the
+	 * other inodes that we did not find in the list attached to the buffer
+	 * and are not already marked stale. If we can't lock it, back off and
+	 * retry.
+	 */
+	if (ip != free_ip) {
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
+			rcu_read_unlock();
+			delay(1);
+			goto retry;
+		}
+
+		/*
+		 * Check the inode number again in case we're racing with
+		 * freeing in xfs_reclaim_inode().  See the comments in that
+		 * function for more information as to why the initial check is
+		 * not sufficient.
+		 */
+		if (ip->i_ino != inum) {
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+			goto out_rcu_unlock;
+		}
+	}
+	rcu_read_unlock();
+
+	xfs_iflock(ip);
+	xfs_iflags_set(ip, XFS_ISTALE);
+
+	/*
+	 * We don't need to attach clean inodes or those only with unlogged
+	 * changes (which we throw away, anyway).
+	 */
+	if (!ip->i_itemp || xfs_inode_clean(ip)) {
+		ASSERT(ip != free_ip);
+		xfs_ifunlock(ip);
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		goto out_no_inode;
+	}
+	return ip;
+
+out_rcu_unlock:
+	rcu_read_unlock();
+out_no_inode:
+	return NULL;
+}
+
 /*
  * A big issue when freeing the inode cluster is that we _cannot_ skip any
  * inodes that are in memory - they all must be marked stale and attached to

@@ -2616,77 +2698,11 @@ xfs_ifree_cluster(
 		 * even trying to lock them.
 		 */
 		for (i = 0; i < igeo->inodes_per_cluster; i++) {
-retry:
-			rcu_read_lock();
-			ip = radix_tree_lookup(&pag->pag_ici_root,
-					XFS_INO_TO_AGINO(mp, (inum + i)));
-
-			/* Inode not in memory, nothing to do */
-			if (!ip) {
-				rcu_read_unlock();
+			ip = xfs_ifree_get_one_inode(pag, free_ip, inum + i);
+			if (!ip)
 				continue;
-			}
-
-			/*
-			 * because this is an RCU protected lookup, we could
-			 * find a recently freed or even reallocated inode
-			 * during the lookup. We need to check under the
-			 * i_flags_lock for a valid inode here. Skip it if it
-			 * is not valid, the wrong inode or stale.
-			 */
-			spin_lock(&ip->i_flags_lock);
-			if (ip->i_ino != inum + i ||
-			    __xfs_iflags_test(ip, XFS_ISTALE)) {
-				spin_unlock(&ip->i_flags_lock);
-				rcu_read_unlock();
-				continue;
-			}
-			spin_unlock(&ip->i_flags_lock);
-
-			/*
-			 * Don't try to lock/unlock the current inode, but we
-			 * _cannot_ skip the other inodes that we did not find
-			 * in the list attached to the buffer and are not
-			 * already marked stale. If we can't lock it, back off
-			 * and retry.
-			 */
-			if (ip != free_ip) {
-				if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
-					rcu_read_unlock();
-					delay(1);
-					goto retry;
-				}
-
-				/*
-				 * Check the inode number again in case we're
-				 * racing with freeing in xfs_reclaim_inode().
-				 * See the comments in that function for more
-				 * information as to why the initial check is
-				 * not sufficient.
-				 */
-				if (ip->i_ino != inum + i) {
-					xfs_iunlock(ip, XFS_ILOCK_EXCL);
-					rcu_read_unlock();
-					continue;
-				}
-			}
-			rcu_read_unlock();
 
-			xfs_iflock(ip);
-			xfs_iflags_set(ip, XFS_ISTALE);
-
-			/*
-			 * we don't need to attach clean inodes or those only
-			 * with unlogged changes (which we throw away, anyway).
-			 */
 			iip = ip->i_itemp;
-			if (!iip || xfs_inode_clean(ip)) {
-				ASSERT(ip != free_ip);
-				xfs_ifunlock(ip);
-				xfs_iunlock(ip, XFS_ILOCK_EXCL);
-				continue;
-			}
-
 			iip->ili_last_fields = iip->ili_fields;
 			iip->ili_fields = 0;
 			iip->ili_fsync_fields = 0;

Signature · 1 line

2.24.0.rc0

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 +7 -0

index 0f80123650e2..3405c39ab92c 100644
@@ -31,6 +31,13 @@ struct shrink_control {
 
 	/* 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 defer_work;
 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index ee4eecc7e1c2..a215d71d9d4b 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

@@ -570,6 +577,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.24.0.rc0

Quoted 12 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.

Quoted 2 lines

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

---

My understanding from the previous discussion(s) is that this is not tied directly to the gfp mask because that is not the only intended use. While it is currently a boolean tied to the the entire shrinker call, the longer term objective is per-object granularity.

I find the argument reasonable enough, but if the above is true, why do we move these checks from ->scan_objects() to ->count_objects() (in the next patch) when per-object decisions will ultimately need to be made by the former?

That seems like unnecessary churn and inconsistent with the argument against just temporarily doing something like what Christoph suggested in the previous version, particularly since IIRC the only use in this series was for gfp mask purposes.

Quoted 30 lines

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

include/linux/shrinker.h +7 -0

index 0f80123650e2..3405c39ab92c 100644
@@ -31,6 +31,13 @@ struct shrink_control {
 
 	/* 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 defer_work;
 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index ee4eecc7e1c2..a215d71d9d4b 100644
@@ -536,6 +536,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 545

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->defer_work)
+		goto done;
+

I still find the fact that this per-shrinker invocation field is never reset unnecessarily fragile, and I don't see any good reason not to reset it prior to the shrinker callback that potentially sets it.

Quoted 13 lines · Brian Foster

Quoted 12 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.

Quoted 7 lines

Quoted 2 lines

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

---

My understanding from the previous discussion(s) is that this is not tied directly to the gfp mask because that is not the only intended use. While it is currently a boolean tied to the the entire shrinker call, the longer term objective is per-object granularity.

Longer term, yes, but right now such things are not possible as the shrinker needs more context to be able to make sane per-object decisions. shrinker policy decisions that affect the entire run scope should be handled by the ->count operation - it's the one that says whether the scan loop should run or not, and right now GFP_NOFS for all filesystem shrinkers is a pure boolean policy implementation.

The next future step is to provide a superblock context with GFP_NOFS to indicate which filesystem we cannot recurse into. That is also a shrinker instance wide check, so again it's something that ->count should be deciding.

i.e. ->count determines what is to be done, ->scan iterates the work that has to be done until we are done.

Quoted 4 lines

I find the argument reasonable enough, but if the above is true, why do we move these checks from ->scan_objects() to ->count_objects() (in the next patch) when per-object decisions will ultimately need to be made by the former?

Because run/no-run policy belongs in one place, and things like GFP_NOFS do no change across calls to the ->scan loop. i.e. after the first ->scan call in a loop that calls it hundreds to thousands of times, the GFP_NOFS run/no-run check is completely redundant.

Once we introduce a new policy that allows the fs shrinker to do careful reclaim in GFP_NOFS conditions, we need to do substantial rework the shrinker scan loop and how it accounts the work that is done - we now have at least 3 or 4 different return counters (skipped because locked, skipped because referenced, reclaimed, deferred reclaim because couldn't lock/recursion) and the accounting and decisions to be made are a lot more complex.

In that case, the ->count function will drop the GFP_NOFS check, but still do all the other things is needs to do. The GFP_NOFS check will go deep in the guts of the shrinker scan implementation where the per-object recursion problem exists. But for most shrinkers, it's still going to be a global boolean check...

Quoted 4 lines

That seems like unnecessary churn and inconsistent with the argument against just temporarily doing something like what Christoph suggested in the previous version, particularly since IIRC the only use in this series was for gfp mask purposes.

If people want to call avoiding repeated, unnecessary evaluation of the same condition hundreds of times instead of once "unnecessary churn", then I'll drop it.

Quoted 30 lines

Quoted 30 lines

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

include/linux/shrinker.h +7 -0

index 0f80123650e2..3405c39ab92c 100644
@@ -31,6 +31,13 @@ struct shrink_control {
 
 	/* 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 defer_work;
 };
 
 #define SHRINK_STOP (~0UL)

mm/vmscan.c +0 -0

index ee4eecc7e1c2..a215d71d9d4b 100644
@@ -536,6 +536,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 11 lines

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

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->defer_work)
+		goto done;
+

I still find the fact that this per-shrinker invocation field is never reset unnecessarily fragile, and I don't see any good reason not to reset it prior to the shrinker callback that potentially sets it.

I missed that when updating. I'll reset it in the next version.

-Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 30 lines · Dave Chinner

Quoted 13 lines · Brian Foster

Quoted 12 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.

Quoted 7 lines

Quoted 2 lines

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

---

My understanding from the previous discussion(s) is that this is not tied directly to the gfp mask because that is not the only intended use. While it is currently a boolean tied to the the entire shrinker call, the longer term objective is per-object granularity.

Longer term, yes, but right now such things are not possible as the shrinker needs more context to be able to make sane per-object decisions. shrinker policy decisions that affect the entire run scope should be handled by the ->count operation - it's the one that says whether the scan loop should run or not, and right now GFP_NOFS for all filesystem shrinkers is a pure boolean policy implementation.

Quoted 8 lines

The next future step is to provide a superblock context with GFP_NOFS to indicate which filesystem we cannot recurse into. That is also a shrinker instance wide check, so again it's something that ->count should be deciding.

i.e. ->count determines what is to be done, ->scan iterates the work that has to be done until we are done.

Sure, makes sense in general.

Quoted 10 lines

Quoted 4 lines

I find the argument reasonable enough, but if the above is true, why do we move these checks from ->scan_objects() to ->count_objects() (in the next patch) when per-object decisions will ultimately need to be made by the former?

Because run/no-run policy belongs in one place, and things like GFP_NOFS do no change across calls to the ->scan loop. i.e. after the first ->scan call in a loop that calls it hundreds to thousands of times, the GFP_NOFS run/no-run check is completely redundant.

What loop is currently called hundreds to thousands of times that this change prevents? AFAICT the current nofs checks in the ->scan calls explicitly terminate the scan loop.

So we're effectively saving a
function call by doing this earlier in the count ->call. (Nothing wrong with that, I'm just not following the numbers used in this reasoning..).

Quoted 8 lines

Once we introduce a new policy that allows the fs shrinker to do careful reclaim in GFP_NOFS conditions, we need to do substantial rework the shrinker scan loop and how it accounts the work that is done - we now have at least 3 or 4 different return counters (skipped because locked, skipped because referenced, reclaimed, deferred reclaim because couldn't lock/recursion) and the accounting and decisions to be made are a lot more complex.

Yeah, that's generally what I expected from your previous description.

Quoted 6 lines

In that case, the ->count function will drop the GFP_NOFS check, but still do all the other things is needs to do. The GFP_NOFS check will go deep in the guts of the shrinker scan implementation where the per-object recursion problem exists. But for most shrinkers, it's still going to be a global boolean check...

So once the nofs checks are lifted out of the ->count callback and into the core shrinker, is there still a use case to defer an entire ->count instance from the callback?

Quoted 4 lines

If people want to call avoiding repeated, unnecessary evaluation of the same condition hundreds of times instead of once "unnecessary churn", then I'll drop it.

I'm not referring to the functional change as churn. What I was referring to is that we're shuffling around the boilerplate gfp checking code between the different shrinker callbacks, knowing that it's eventually going to be lifted out, when we could potentially just lift that code up a level now.

Quoted 53 lines · Brian Foster

Quoted 30 lines · Dave Chinner

Quoted 13 lines · Brian Foster

Quoted 12 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.

Quoted 7 lines

Quoted 2 lines

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

---

My understanding from the previous discussion(s) is that this is not tied directly to the gfp mask because that is not the only intended use. While it is currently a boolean tied to the the entire shrinker call, the longer term objective is per-object granularity.

Longer term, yes, but right now such things are not possible as the shrinker needs more context to be able to make sane per-object decisions. shrinker policy decisions that affect the entire run scope should be handled by the ->count operation - it's the one that says whether the scan loop should run or not, and right now GFP_NOFS for all filesystem shrinkers is a pure boolean policy implementation.

Quoted 8 lines

The next future step is to provide a superblock context with GFP_NOFS to indicate which filesystem we cannot recurse into. That is also a shrinker instance wide check, so again it's something that ->count should be deciding.

i.e. ->count determines what is to be done, ->scan iterates the work that has to be done until we are done.

Sure, makes sense in general.

Quoted 10 lines

Quoted 4 lines

I find the argument reasonable enough, but if the above is true, why do we move these checks from ->scan_objects() to ->count_objects() (in the next patch) when per-object decisions will ultimately need to be made by the former?

Because run/no-run policy belongs in one place, and things like GFP_NOFS do no change across calls to the ->scan loop. i.e. after the first ->scan call in a loop that calls it hundreds to thousands of times, the GFP_NOFS run/no-run check is completely redundant.

Quoted 3 lines

What loop is currently called hundreds to thousands of times that this change prevents? AFAICT the current nofs checks in the ->scan calls explicitly terminate the scan loop.

Right, but when we are in GFP_KERNEL context, every call to ->scan() checks it and says "ok". If we are scanning tens of thousands of objects in a scan, and we are using a befault batch size of 128 objects per scan, then we have hundreds of calls in a single scan loop that check the GFP context and say "ok"....

Quoted 3 lines

So we're effectively saving a
function call by doing this earlier in the count ->call. (Nothing wrong with that, I'm just not following the numbers used in this reasoning..).

It's the don't terminate case. :)

Quoted 18 lines

Quoted 8 lines

Once we introduce a new policy that allows the fs shrinker to do careful reclaim in GFP_NOFS conditions, we need to do substantial rework the shrinker scan loop and how it accounts the work that is done - we now have at least 3 or 4 different return counters (skipped because locked, skipped because referenced, reclaimed, deferred reclaim because couldn't lock/recursion) and the accounting and decisions to be made are a lot more complex.

Yeah, that's generally what I expected from your previous description.

Quoted 6 lines

In that case, the ->count function will drop the GFP_NOFS check, but still do all the other things is needs to do. The GFP_NOFS check will go deep in the guts of the shrinker scan implementation where the per-object recursion problem exists. But for most shrinkers, it's still going to be a global boolean check...

Quoted 3 lines

So once the nofs checks are lifted out of the ->count callback and into the core shrinker, is there still a use case to defer an entire ->count instance from the callback?

Not right now. There may be in future, but I don't want to make things more complex than they need to be by trying to support functionality that isn't used.

Quoted 10 lines

Quoted 4 lines

If people want to call avoiding repeated, unnecessary evaluation of the same condition hundreds of times instead of once "unnecessary churn", then I'll drop it.

I'm not referring to the functional change as churn. What I was referring to is that we're shuffling around the boilerplate gfp checking code between the different shrinker callbacks, knowing that it's eventually going to be lifted out, when we could potentially just lift that code up a level now.

I don't think that lifting it up will save much code at all, once we add all the gfp mask intialisation to all the shrinkers, etc. It's just means we can't look at the shrinker implementation and know that it can't run in GFP_NOFS context - we have to go look up where it is instantiated instead to see if there are gfp context constraints.

I think it's better where it is, documenting the constraints the shrinker implementation runs under in the implementation itself...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

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. Document the scan batch size calculation better while we are there.

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 | 97 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 27 deletions(-)

mm/vmscan.c +31 -0

index a215d71d9d4b..2d39ec37c04d 100644
@@ -459,13 +459,68 @@ 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)
+{
+	int64_t delta;
+	int64_t freeable;
+
+	freeable = shrinker->count_objects(shrinker, shrinkctl);
+	if (freeable == 0 || freeable == SHRINK_EMPTY)
+		return freeable;
+
+	if (shrinker->seeks) {
+		/*
+		 * shrinker->seeks is a measure of how much IO is required to
+		 * reinstantiate the object in memory. The default value is 2
+		 * which is typical for a cold inode requiring a directory read
+		 * and an inode read to re-instantiate.
+		 *
+		 * The scan batch size is defined by the shrinker priority, but
+		 * to be able to bias the reclaim we increase the default batch
+		 * size by 4. Hence we end up with a scan batch multipler that
+		 * scales like so:
+		 *
+		 * ->seeks	scan batch multiplier
+		 *    1		      4.00x
+		 *    2               2.00x

mm/vmscan.c +16 -2

+		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;
+	}
+
+	*freeable_objects = freeable;
+	return delta > 0 ? delta : 0;
+}
+
 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;

mm/vmscan.c +0 -11

@@ -487,25 +543,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;

@@ -522,19 +564,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

@@ -559,7 +602,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.24.0.rc0

Quoted 8 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. Document the scan batch size calculation better while we are there.

Quoted 6 lines

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

---

I assume the kbuild warning thing will be fixed up...

Quoted 46 lines

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

mm/vmscan.c +35 -0

index a215d71d9d4b..2d39ec37c04d 100644
@@ -459,13 +459,68 @@ 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)
+{
+	int64_t delta;
+	int64_t freeable;
+
+	freeable = shrinker->count_objects(shrinker, shrinkctl);
+	if (freeable == 0 || freeable == SHRINK_EMPTY)
+		return freeable;
+
+	if (shrinker->seeks) {
+		/*
+		 * shrinker->seeks is a measure of how much IO is required to
+		 * reinstantiate the object in memory. The default value is 2
+		 * which is typical for a cold inode requiring a directory read
+		 * and an inode read to re-instantiate.
+		 *
+		 * The scan batch size is defined by the shrinker priority, but
+		 * to be able to bias the reclaim we increase the default batch
+		 * size by 4. Hence we end up with a scan batch multipler that
+		 * scales like so:
+		 *
+		 * ->seeks	scan batch multiplier
+		 *    1		      4.00x
+		 *    2               2.00x
+		 *    3               1.33x
+		 *    4               1.00x
+		 *    8               0.50x
+		 *

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

Quoted 6 lines

mm/vmscan.c +6 -0

+		 * IOWs, the more seeks it takes to pull the item into cache,
+		 * the smaller the reclaim scan batch. Hence we put more reclaim
+		 * pressure on caches that are fast to repopulate and to keep a
+		 * rough balance between caches that have different costs.
+		 */
+		delta = freeable >> (priority - 2);

Does anything prevent priority < 2 here?

Quoted 15 lines

mm/vmscan.c +1 -13

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

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

Quoted 5 lines

mm/vmscan.c +1 -1

 	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;

Same question as before: why the change in assignment? freeable was the ->count_objects() return value, which is now stored in freeable_objects.

FWIW, the change seems to make sense in that it just factors out the deferred count, but it's not clear if it's intentional...

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.

Note that we use signed integers for everything except the freed count as the returns from the shrinker callouts cannot be guaranteed untainted. Indeed, the shrinkers can return scan counts larger that were fed in, so we need scan counts to underflow in a detectable manner to terminate loops. This is necessary to avoid a misbehaving shrinker from triggering endless scanning loops.

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

---
 include/linux/shrinker.h |   2 +-
 mm/vmscan.c              | 100 ++++++++++++++++++++-------------------
 2 files changed, 53 insertions(+), 49 deletions(-)

include/linux/shrinker.h +1 -1

index 3405c39ab92c..30c10f42109f 100644
@@ -81,7 +81,7 @@ struct shrinker {
 	int id;
 #endif
 	/* objs pending delete, per node */
-	atomic_long_t *nr_deferred;
+	atomic64_t *nr_deferred;
 };
 #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
 

mm/vmscan.c +10 -28

index 2d39ec37c04d..c0e2bf656e3f 100644
@@ -517,16 +517,16 @@ static int64_t shrink_scan_count(struct shrink_control *shrinkctl,
 static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 				    struct shrinker *shrinker, int priority)
 {
-	unsigned long freed = 0;
-	long total_scan;
+	uint64_t freed = 0;
 	int64_t freeable_objects = 0;
 	int64_t scan_count;
-	long nr;
-	long new_nr;
+	int64_t scanned_objects = 0;
+	int64_t next_deferred = 0;
+	int64_t deferred_count = 0;
+	int64_t 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;

@@ -537,47 +537,51 @@ 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.
-	 */
-	nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
-
-	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;
-
-	/*
-	 * 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.
+	 * 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

mm/vmscan.c +9 -2

 	 */
-	if (scan_count < freeable_objects / 4)
-		total_scan = min_t(long, total_scan, freeable_objects / 2);
+	if (current_is_kswapd()) {
+		int64_t	deferred_scan;
+
+		deferred_count = atomic64_xchg(&shrinker->nr_deferred[nid], 0);
+
+		/* 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 +8 -9

-	if (total_scan > freeable_objects * 2)
-		total_scan = freeable_objects * 2;
+	scan_count = min(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

@@ -601,10 +605,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;

@@ -614,29 +618,29 @@ 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

-	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.24.0.rc0

Quoted 33 lines · Dave Chinner

mm/vmscan.c +7 -9

@@ -601,10 +605,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;

@@ -614,29 +618,29 @@ 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;
+	if (deferred_count)
+		next_deferred = deferred_count - scanned_objects;
 	else
-		next_deferred = 0;

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

Quoted 1 line

mm/vmscan.c +1 -0

+		next_deferred = scan_count;

Hmm.. so if there was no deferred count on this cycle, we set next_deferred to whatever is left from scan_count and add that back into the shrinker struct below. If there was a pending deferred count on this cycle, we subtract what we scanned from that and add that value back. But what happens to the remaining scan_count in the latter case? Is it lost, or am I missing something?

For example, suppose we start this cycle with a large scan_count and ->scan_objects() returned SHRINK_STOP before doing much work. In that scenario, it looks like whether ->nr_deferred is 0 or not is the only thing that determines whether we defer the entire remaining scan_count or just what is left from the previous ->nr_deferred. The existing code appears to consistently factor in what is left from the current scan with the previous deferred count. Hm?

Quoted 34 lines · Brian Foster

Quoted 33 lines · Dave Chinner

mm/vmscan.c +7 -9

@@ -601,10 +605,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;

@@ -614,29 +618,29 @@ 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;
+	if (deferred_count)
+		next_deferred = deferred_count - scanned_objects;
 	else
-		next_deferred = 0;

Quoted 8 lines

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

Quoted 1 line

mm/vmscan.c +1 -0

+		next_deferred = scan_count;

Hmm.. so if there was no deferred count on this cycle, we set next_deferred to whatever is left from scan_count and add that back into the shrinker struct below. If there was a pending deferred count on this cycle, we subtract what we scanned from that and add that value back. But what happens to the remaining scan_count in the latter case? Is it lost, or am I missing something?

if deferred_count is not zero, then it is kswapd that is running. It does the deferred work, and if it doesn't make progress then adding it's scan count to the deferred work doesn't matter. That's because it will come back with an increased priority in a short while and try to scan more of the deferred count plus it's larger scan count.

IOWs, if we defer kswapd unused scan count, we effectively increase the pressure as the priority goes up, potentially making the deferred count increase out of control. i.e. kswapd can make progress and free items, but the result is that it increased the deferred scan count rather than reducing it. This leads to excessive reclaim of the slab caches and kswapd can trash the caches long after the memory pressure has gone away...

Quoted 7 lines

For example, suppose we start this cycle with a large scan_count and ->scan_objects() returned SHRINK_STOP before doing much work. In that scenario, it looks like whether ->nr_deferred is 0 or not is the only thing that determines whether we defer the entire remaining scan_count or just what is left from the previous ->nr_deferred. The existing code appears to consistently factor in what is left from the current scan with the previous deferred count. Hm?

If kswapd doesn't have any deferred work, then it's largely no different in behaviour to direct reclaim. If it has no deferred work, then the shrinker is not getting stopped early in direct reclaim, so it's unlikely that kswapd is going to get stopped early, either....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Dave Chinner <[email protected]>

The tracepoint information in the shrinker code don't make a lot of

sense anymore and contain redundant information as a result of the changes in the patchset. Refine the information passed to the tracepoints so they expose the operation of the shrinkers more precisely and clean up the remaining code and varibles in the

shrinker code so it all makes sense.

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

---
 include/trace/events/vmscan.h | 69 ++++++++++++++++-------------------
 mm/vmscan.c                   | 24 +++++-------
 2 files changed, 41 insertions(+), 52 deletions(-)

mm/vmscan.c +0 -0

index c0e2bf656e3f..7a8256322150 100644

mm/vmscan.c +0 -0

@@ -624,23 +622,21 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
 		cond_resched();
 	}
 done:

return freed;
}

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

Add a wrapper to account for page freeing in shrinker reclaim so that the high level scanning accounts for all the memory freed during a shrinker scan.

No logic changes, just replacing open coded checks with a simple wrapper.

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

---

 fs/inode.c           |  3 +--
 fs/xfs/xfs_buf.c     |  4 +---
 include/linux/swap.h | 20 ++++++++++++++++++--
 mm/slab.c            |  3 +--
 mm/slob.c            |  4 +---
 mm/slub.c            |  3 +--
 mm/vmscan.c          |  4 ++--
 7 files changed, 25 insertions(+), 16 deletions(-)

fs/inode.c +1 -2

index fef457a42882..a77caf216659 100644
@@ -764,8 +764,7 @@ static enum lru_status inode_lru_isolate(struct list_head *item,
 				__count_vm_events(KSWAPD_INODESTEAL, reap);
 			else
 				__count_vm_events(PGINODESTEAL, reap);
-			if (current->reclaim_state)
-				current->reclaim_state->reclaimed_slab += reap;
+			current_reclaim_account_pages(reap);
 		}
 		iput(inode);
 		spin_lock(lru_lock);

fs/xfs/xfs_buf.c +1 -3

index d34e5d2edacd..55b082bc53b3 100644
@@ -324,9 +324,7 @@ xfs_buf_free(
 
 			__free_page(page);
 		}
-		if (current->reclaim_state)
-			current->reclaim_state->reclaimed_slab +=
-							bp->b_page_count;
+		current_reclaim_account_pages(bp->b_page_count);
 	} else if (bp->b_flags & _XBF_KMEM)
 		kmem_free(bp->b_addr);
 	_xfs_buf_free_pages(bp);

include/linux/swap.h +18 -2

index 063c0c1e112b..72b855fe20b0 100644
@@ -126,12 +126,28 @@ union swap_header {
 
 /*
  * current->reclaim_state points to one of these when a task is running
- * memory reclaim
+ * memory reclaim. It is typically used by shrinkers to return reclaim
+ * information back to the main vmscan loop.
  */
 struct reclaim_state {
-	unsigned long reclaimed_slab;
+	unsigned long	reclaimed_pages;	/* pages freed by shrinkers */
 };
 
+/*
+ * When code frees a page that may be run from a memory reclaim context, it
+ * needs to account for the pages it frees so memory reclaim can track them.
+ * Slab memory that is freed is accounted via this mechanism, so this is not
+ * necessary for slab or heap memory being freed. However, if the object being
+ * freed frees pages directly, then those pages should be accounted as well when
+ * in memory reclaim. This helper function takes care accounting for the pages
+ * being reclaimed when it is required.
+ */
+static inline void current_reclaim_account_pages(int nr_pages)
+{
+	if (current->reclaim_state)
+		current->reclaim_state->reclaimed_pages += nr_pages;
+}
+
 #ifdef __KERNEL__
 
 struct address_space;

mm/slab.c +1 -2

index 66e5d8032bae..419be005f41a 100644
@@ -1395,8 +1395,7 @@ static void kmem_freepages(struct kmem_cache *cachep, struct page *page)
 	page_mapcount_reset(page);
 	page->mapping = NULL;
 
-	if (current->reclaim_state)
-		current->reclaim_state->reclaimed_slab += 1 << order;
+	current_reclaim_account_pages(1 << order);
 	uncharge_slab_page(page, order, cachep);
 	__free_pages(page, order);
 }

mm/slob.c +1 -3

index fa53e9f73893..c54a7eeee86d 100644
@@ -211,9 +211,7 @@ static void slob_free_pages(void *b, int order)
 {
 	struct page *sp = virt_to_page(b);
 
-	if (current->reclaim_state)
-		current->reclaim_state->reclaimed_slab += 1 << order;
-
+	current_reclaim_account_pages(1 << order);
 	mod_node_page_state(page_pgdat(sp), NR_SLAB_UNRECLAIMABLE,
 			    -(1 << order));
 	__free_pages(sp, order);

mm/slub.c +1 -2

index b25c807a111f..478554082079 100644
@@ -1746,8 +1746,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
 	__ClearPageSlab(page);
 
 	page->mapping = NULL;
-	if (current->reclaim_state)
-		current->reclaim_state->reclaimed_slab += pages;
+	current_reclaim_account_pages(pages);
 	uncharge_slab_page(page, order, s);
 	__free_pages(page, order);
 }

mm/vmscan.c +2 -2

index 7a8256322150..967e3d3c7748 100644
@@ -2870,8 +2870,8 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 		} while ((memcg = mem_cgroup_iter(root, memcg, NULL)));
 
 		if (reclaim_state) {
-			sc->nr_reclaimed += reclaim_state->reclaimed_slab;
-			reclaim_state->reclaimed_slab = 0;
+			sc->nr_reclaimed += reclaim_state->reclaimed_pages;
+			reclaim_state->reclaimed_pages = 0;
 		}
 
 		/* Record the subtree's reclaim efficiency */

Signature · 1 line

2.24.0.rc0

From: Dave Chinner <[email protected]>

When the majority of possible shrinker reclaim work is deferred by the shrinkers (e.g. due to GFP_NOFS context), and there is more work defered than LRU pages were scanned, back off reclaim if there are

large amounts of IO in progress.

This tends to occur when there are inode cache heavy workloads that have little page cache or application memory pressure on filesytems like XFS. Inode cache heavy workloads involve lots of IO, so if we are getting device congestion it is indicative of memory reclaim running up against an IO throughput limitation. in this situation we need to throttle direct reclaim as we nee dto wait for kswapd to

get some of the deferred work done.

However, if there is no device congestion, then the system is keeping up with both the workload and memory reclaim and so there's no need to throttle.

Hence we should only back off scanning for a bit if we see this condition and there is block device congestion present.

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

---
 include/linux/swap.h |  2 ++
 mm/vmscan.c          | 30 +++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

include/linux/swap.h +0 -0

index 72b855fe20b0..da0913e14bb9 100644

include/linux/swap.h +1 -0

+	unsigned long	deferred_objects;	/* work that wasn't done */
 };
 
 /*

mm/vmscan.c +0 -0

index 967e3d3c7748..13c11e10c9c5 100644