xfs: reclaim bug fixes

From: Dave Chinner <[email protected]>

https://bugzilla.kernel.org/show_bug.cgi?id=16348

When the filesystem grows to a large number of allocation groups, the summing of recalimable inodes gets expensive. In many cases, most AGs won't have any reclaimable inodes and so we are wasting CPU time aggregating over these AGs. This is particularly important for the inode shrinker that gets called frequently under memory pressure.

To avoid the overhead, track AGs with reclaimable inodes in the per-ag radix tree so that we can find all the AGs with reclaimable inodes via a simple gang tag lookup. This involves setting the tag when the first reclaimable inode is tracked in the AG, and removing the tag when the last reclaimable inode is removed from the tree. Then the summation process becomes a loop walking the radix tree summing AGs with the reclaim tag set.

This significantly reduces the overhead of scanning - a 6400 AG filesystea now only uses about 25% of a cpu in kswapd while slab reclaim progresses instead of being permanently stuck at 100% CPU and making little progress. Clean filesystems filesystems will see no overhead and the overhead only increases linearly with the number of dirty AGs.

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

---
 fs/xfs/linux-2.6/xfs_sync.c  |   71 +++++++++++++++++++++++++++++++++++++----
 fs/xfs/linux-2.6/xfs_trace.h |    3 ++
 2 files changed, 67 insertions(+), 7 deletions(-)

fs/xfs/linux-2.6/xfs_sync.c +14 -0

index 56fed91..51da595 100644
@@ -133,6 +133,41 @@ restart:
 	return last_error;
 }
 
+/*
+ * Select the next per-ag structure to iterate during the walk. The reclaim
+ * walk is optimised only to walk AGs with reclaimable inodes in them.
+ */
+static struct xfs_perag *
+xfs_inode_ag_iter_next_pag(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		*first,
+	int			tag)
+{
+	struct xfs_perag	*pag = NULL;
+
+	if (tag == XFS_ICI_RECLAIM_TAG) {
+		int found;

Using the same tag represent a perag with reclaimable inodes as the tag representing an inode is reclaimable is nicely consistent...

Quoted 3 lines · Alex Elder

Using the same tag represent a perag with reclaimable inodes as the tag representing an inode is reclaimable is nicely consistent...

Oh, didn't notice that when reviewing it. Not sure I like it too much. If we ever grow more tags on the perag tree this might get rather confusing.

Quoted 19 lines · Dave Chinner

From: Dave Chinner <[email protected]>

https://bugzilla.kernel.org/show_bug.cgi?id=16348

When the filesystem grows to a large number of allocation groups, the summing of recalimable inodes gets expensive. In many cases, most AGs won't have any reclaimable inodes and so we are wasting CPU time aggregating over these AGs. This is particularly important for the inode shrinker that gets called frequently under memory pressure.

To avoid the overhead, track AGs with reclaimable inodes in the per-ag radix tree so that we can find all the AGs with reclaimable inodes via a simple gang tag lookup. This involves setting the tag when the first reclaimable inode is tracked in the AG, and removing the tag when the last reclaimable inode is removed from the tree. Then the summation process becomes a loop walking the radix tree summing AGs with the reclaim tag set.

Quoted 5 lines

This significantly reduces the overhead of scanning - a 6400 AG filesystea now only uses about 25% of a cpu in kswapd while slab reclaim progresses instead of being permanently stuck at 100% CPU and making little progress. Clean filesystems filesystems will see no overhead and the overhead only increases linearly with the number of dirty AGs.

Using the same tag represent a perag with reclaimable inodes as the tag representing an inode is reclaimable is nicely consistent...

I have a few comments below for your consideration but they are minor (and don't even require a response).

Overall this looks good.

Reviewed-by Alex Elder <[email protected]>

Quoted 28 lines

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

---
 fs/xfs/linux-2.6/xfs_sync.c  |   71 +++++++++++++++++++++++++++++++++++++----
 fs/xfs/linux-2.6/xfs_trace.h |    3 ++
 2 files changed, 67 insertions(+), 7 deletions(-)

fs/xfs/linux-2.6/xfs_sync.c +14 -0

index 56fed91..51da595 100644
@@ -133,6 +133,41 @@ restart:
 	return last_error;
 }
 
+/*
+ * Select the next per-ag structure to iterate during the walk. The reclaim
+ * walk is optimised only to walk AGs with reclaimable inodes in them.
+ */
+static struct xfs_perag *
+xfs_inode_ag_iter_next_pag(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		*first,
+	int			tag)
+{
+	struct xfs_perag	*pag = NULL;
+
+	if (tag == XFS_ICI_RECLAIM_TAG) {
+		int found;

fs/xfs/linux-2.6/xfs_sync.c restart: · line 159

Quoted 10 lines

fs/xfs/linux-2.6/xfs_sync.c +10 -0

+		int ref;
+
+		spin_lock(&mp->m_perag_lock);
+		found = radix_tree_gang_lookup_tag(&mp->m_perag_tree,
+				(void **)&pag, *first, 1, tag);
+		if (found <= 0) {
+			spin_unlock(&mp->m_perag_lock);
+			return NULL;
+		}
+		*first = pag->pag_agno + 1;

Maybe move this below, just before the return. I.e.:

	if (pag)
		*first = pag->pag_agno + 1;

To me it's slightly clearer that we're just setting up to search next time with the perag following the one returned.

fs/xfs/linux-2.6/xfs_sync.c restart: · line 160

Quoted 2 lines

fs/xfs/linux-2.6/xfs_sync.c +1 -0

+		/* open coded pag reference increment */

By open-coding here you miss the assertions in xfs_perag_get(). (Though a common inline encapsulating them would have to be in a header because the two functions reside in different files.)

From: Dave Chinner <[email protected]>

xfs_ireclaim has to get and put te pag structure because it is only called with the inode to reclaim. The one caller of this function already has a reference on the pag and a pointer to is, so move the radix tree delete to the caller and remove xfs_ireclaim completely. This avoids a xfs_perag_get/put on every inode being reclaimed.

The overhead was noticed in a bug report at:

https://bugzilla.kernel.org/show_bug.cgi?id=16348

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

---
 fs/xfs/linux-2.6/xfs_sync.c |   31 ++++++++++++++++++++++++-
 fs/xfs/xfs_iget.c           |   53 +------------------------------------------
 fs/xfs/xfs_inode.h          |    2 +-
 3 files changed, 32 insertions(+), 54 deletions(-)

From: Dave Chinner <[email protected]>

xfs_ireclaim has to get and put te pag structure because it is only called with the inode to reclaim. The one caller of this function already has a reference on the pag and a pointer to is, so move the radix tree delete to the caller and remove xfs_ireclaim completely. This avoids a xfs_perag_get/put on every inode being reclaimed.

The overhead was noticed in a bug report at:

https://bugzilla.kernel.org/show_bug.cgi?id=16348

From: Dave Chinner <[email protected]>

xfs_trans_add_item() is called with ip->i_ilock held, which means it is unsafe for memory reclaim to recurse back into the filesystem (ilock is required in writeback). Hence the allocation needs to be KM_NOFS to avoid recursion.

Lockdep report indicating memory allocation being called with the ip->i_ilock held is as follows:

fs/xfs/xfs_trans.c xfs_trans_add_item( · line 1137

Quoted 2 lines

fs/xfs/xfs_trans.c +1 -1

-	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP);
+	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP|KM_NOFS);

KM_NOFS by itself actually is sufficiant. If you want to keep the KM_SLEEP for documentation purposes at least add some spaces to make it more redable.

	lidp = kmem_zone_zalloc(xfs_log_item_desc_zone, KM_SLEEP | KM_NOFS);

From: Dave Chinner <[email protected]>

Avoid a lockdep warning by preventing page cache allocation from recursing back into the filesystem during memory reclaim.

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

---
 fs/xfs/linux-2.6/xfs_aops.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

fs/xfs/linux-2.6/xfs_aops.c +0 -0

index ed9c3db..1075791 100644

From: Dave Chinner <[email protected]>

Avoid a lockdep warning by preventing page cache allocation from recursing back into the filesystem during memory reclaim.

From: Dave Chinner <[email protected]>

Calling into memory reclaim with a locked inode buffer can deadlock if memory reclaim tries to lock the inode buffer during inode teardown. Convert the relevant memory allocations to use KM_NOFS to avoid this deadlock condition.

Reported-by Peter Watkins <[email protected]>

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

---
 fs/xfs/xfs_inode.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

fs/xfs/xfs_inode.c +8 -8

index 5715a9d..3c206b3 100644
@@ -422,7 +422,7 @@ xfs_iformat(
 	if (!XFS_DFORK_Q(dip))
 		return 0;
 	ASSERT(ip->i_afp == NULL);
-	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
+	ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP|KM_NOFS);
 	ip->i_afp->if_ext_max =
 		XFS_IFORK_ASIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
 	switch (dip->di_aformat) {

@@ -505,7 +505,7 @@ xfs_iformat_local(
 		ifp->if_u1.if_data = ifp->if_u2.if_inline_data;
 	else {
 		real_size = roundup(size, 4);
-		ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP);
+		ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP|KM_NOFS);
 	}
 	ifp->if_bytes = size;
 	ifp->if_real_bytes = real_size;

@@ -632,7 +632,7 @@ xfs_iformat_btree(
 	}
 
 	ifp->if_broot_bytes = size;
-	ifp->if_broot = kmem_alloc(size, KM_SLEEP);
+	ifp->if_broot = kmem_alloc(size, KM_SLEEP|KM_NOFS);
 	ASSERT(ifp->if_broot != NULL);
 	/*
 	 * Copy and convert from the on-disk structure

@@ -2191,7 +2191,7 @@ xfs_iroot_realloc(
 		 */
 		if (ifp->if_broot_bytes == 0) {
 			new_size = (size_t)XFS_BMAP_BROOT_SPACE_CALC(rec_diff);
-			ifp->if_broot = kmem_alloc(new_size, KM_SLEEP);
+			ifp->if_broot = kmem_alloc(new_size, KM_SLEEP|KM_NOFS);
 			ifp->if_broot_bytes = (int)new_size;
 			return;
 		}

@@ -2207,7 +2207,7 @@ xfs_iroot_realloc(
 		new_size = (size_t)XFS_BMAP_BROOT_SPACE_CALC(new_max);
 		ifp->if_broot = kmem_realloc(ifp->if_broot, new_size,
 				(size_t)XFS_BMAP_BROOT_SPACE_CALC(cur_max), /* old size */
-				KM_SLEEP);
+				KM_SLEEP|KM_NOFS);
 		op = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, ifp->if_broot, 1,
 						     ifp->if_broot_bytes);
 		np = (char *)XFS_BMAP_BROOT_PTR_ADDR(mp, ifp->if_broot, 1,

@@ -2233,7 +2233,7 @@ xfs_iroot_realloc(
 	else
 		new_size = 0;
 	if (new_size > 0) {
-		new_broot = kmem_alloc(new_size, KM_SLEEP);
+		new_broot = kmem_alloc(new_size, KM_SLEEP|KM_NOFS);
 		/*
 		 * First copy over the btree block header.
 		 */

@@ -2337,7 +2337,8 @@ xfs_idata_realloc(
 		real_size = roundup(new_size, 4);
 		if (ifp->if_u1.if_data == NULL) {
 			ASSERT(ifp->if_real_bytes == 0);
-			ifp->if_u1.if_data = kmem_alloc(real_size, KM_SLEEP);
+			ifp->if_u1.if_data = kmem_alloc(real_size,
+							KM_SLEEP|KM_NOFS);
 		} else if (ifp->if_u1.if_data != ifp->if_u2.if_inline_data) {
 			/*
 			 * Only do the realloc if the underlying size

@@ -2348,11 +2349,12 @@ xfs_idata_realloc(
 					kmem_realloc(ifp->if_u1.if_data,
 							real_size,
 							ifp->if_real_bytes,
-							KM_SLEEP);

From: Dave Chinner <[email protected]>

Calling into memory reclaim with a locked inode buffer can deadlock if memory reclaim tries to lock the inode buffer during inode teardown. Convert the relevant memory allocations to use KM_NOFS to avoid this deadlock condition.

The following patches fix excessive CPU consumption during inode cache shrinking when filesystems have lots of allocation groups as well as prevent a couple of lockdep reports that were found during testing. Also included is a fix for a reclaim recursion deadlock when allocating memory during inode initialisation.

Quoted 5 lines · Dave Chinner

The following patches fix excessive CPU consumption during inode cache shrinking when filesystems have lots of allocation groups as well as prevent a couple of lockdep reports that were found during testing. Also included is a fix for a reclaim recursion deadlock when allocating memory during inode initialisation.

Wa the overlap of patch 1 with the for-2.6.35 shrinker series intentional? In addition to patch 1 patches 3 to 5 are also for-2.6.35 material in my opinion.