[PATCH 0/2] Per superblock inode reclaim

[PATCH 0/2] Per superblock inode reclaim

[Bob Peterson]

In July 2011, near patch b0d40c92adafde7c2d81203ce7c1c69275f41140,
Dave Chinner introduced the concept of per-superblock shrinkers.
However, vfs still uses its own function, prune_icache_sb, which
gives almost no control to the underlying file system over the
inode eviction.

The trouble is, some file systems (GFS2 in particular) need more
control over the eviction of inodes. When you evict an inode in GFS2,
it may need to do inter-node locking and unlocking, for which it
calls the distributed lock manager, DLM. But DLM may not be able to
service the request immediately, due to unforeseen circumstances.

For example, if a cluster node has failed, DLM may need to wait for
that node to be fenced which, in turn, may block on a response from the
user space fence daemon. That, in turn, may block waiting on memory
allocation which caused the shrinker to be called in the first place.
Thus, GFS2 sits forever in an unrecoverable deadlock.

This is not unique to GFS2: OCFS2 and NFS probably have the same issue.

The first patch set extends Dave Chinner's idea further, adding a
filesystem-specific prune_icache_sb function and exporting the
existing one.

The second patch changes GFS2 to provide a prune_icache_sb function,
which really just tells its running daemon to shrink the inode slab
when it can, and by how many items. It does so by calling the vfs
prune_icache_sb to do the majority of the work. Deferring the
shrinker to the GFS2 daemon allows the vfs shrinker to proceed
without blocking on GFS2. The process that caused the shrink (in the
example above, the fence daemon) may then proceed without blocking.
The daemon can then safely evict the inodes, calling the DLM without
the risk of deadlock.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
Bob Peterson (2):
  vfs: Add hooks for filesystem-specific prune_icache_sb
  GFS2: Add a gfs2-specific prune_icache_sb

 Documentation/filesystems/vfs.txt | 15 +++++++++++++++
 fs/gfs2/incore.h                  |  2 ++
 fs/gfs2/ops_fstype.c              |  1 +
 fs/gfs2/quota.c                   | 25 +++++++++++++++++++++++++
 fs/gfs2/super.c                   | 13 +++++++++++++
 fs/inode.c                        |  1 +
 fs/super.c                        |  5 ++++-
 include/linux/fs.h                |  3 +++
 8 files changed, 64 insertions(+), 1 deletion(-)

-- 
2.5.5

[PATCH 1/2] vfs: Add hooks for filesystem-specific prune_icache_sb

[Bob Peterson]

This patch adds filesystem-specific callbacks for shrinking the
inode cache, prune_icache_sb. This is provided for filesystems in
which the default VFS prune_icache_sb needs to be delayed due to
filesystem locking requirements not met by vfs.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 Documentation/filesystems/vfs.txt | 15 +++++++++++++++
 fs/inode.c                        |  1 +
 fs/super.c                        |  5 ++++-
 include/linux/fs.h                |  3 +++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index c61a223..7cb4c5c 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -230,6 +230,7 @@ struct super_operations {
         ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
 	int (*nr_cached_objects)(struct super_block *);
 	void (*free_cached_objects)(struct super_block *, int);
+	long (*prune_icache_sb)(struct super_block *, struct shrink_control *);
 };
 
 All methods are called without any locks being held, unless otherwise
@@ -319,6 +320,20 @@ or bottom half).
 	implementations will cause holdoff problems due to large scan batch
 	sizes.
 
+  prune_icache_sb: called by the sb cache shrinking function for the file
+	filesystem to reduce the number of inodes from slab. This is done to
+	accomodate file systems that may not be able to immediately remove
+	inodes from cache, but must queue them to be removed ASAP.
+
+	This can happen in GFS2, for example, where evicting an inode
+	may require an inter-node lock (glock) which makes a call into DLM
+	(distributed lock manager), which may block for any number of reasons.
+	For example, it may block because a customer node needs to be fenced,
+	so the lock cannot be granted until the fencing is complete.
+	The fencing, in turn, may be blocked for other reasons, such as
+	memory allocations that caused the shrinker to be called in the first
+	place. Optional. If not set, the default vfs prune_icache_sb is called.
+
 Whoever sets up the inode is responsible for filling in the "i_op" field. This
 is a pointer to a "struct inode_operations" which describes the methods that
 can be performed on individual inodes.
diff --git a/fs/inode.c b/fs/inode.c
index 4ccbc21..82c10f3 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -771,6 +771,7 @@ long prune_icache_sb(struct super_block *sb, struct shrink_control *sc)
 	dispose_list(&freeable);
 	return freed;
 }
+EXPORT_SYMBOL(prune_icache_sb);
 
 static void __wait_on_freeing_inode(struct inode *inode);
 /*
diff --git a/fs/super.c b/fs/super.c
index d78b984..8087903 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -98,7 +98,10 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 	sc->nr_to_scan = dentries + 1;
 	freed = prune_dcache_sb(sb, sc);
 	sc->nr_to_scan = inodes + 1;
-	freed += prune_icache_sb(sb, sc);
+	if (sb->s_op->prune_icache_sb)
+		freed += sb->s_op->prune_icache_sb(sb, sc);
+	else
+		freed += prune_icache_sb(sb, sc);
 
 	if (fs_objects) {
 		sc->nr_to_scan = fs_objects + 1;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5f61431..96e6ae2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1797,6 +1797,8 @@ struct super_operations {
 				  struct shrink_control *);
 	long (*free_cached_objects)(struct super_block *,
 				    struct shrink_control *);
+	long (*prune_icache_sb)(struct super_block *sb,
+				struct shrink_control *sc);
 };
 
 /*
@@ -2714,6 +2716,7 @@ extern void lockdep_annotate_inode_mutex_key(struct inode *inode);
 static inline void lockdep_annotate_inode_mutex_key(struct inode *inode) { };
 #endif
 extern void unlock_new_inode(struct inode *);
+extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern unsigned int get_next_ino(void);
 
 extern void __iget(struct inode * inode);
-- 
2.5.5

[PATCH 2/2] GFS2: Add a gfs2-specific prune_icache_sb

[Bob Peterson]

This patch adds a new prune_icache_sb function for the VFS slab
shrinker to call. Trying to directly free the inodes from memory
might deadlock because it evicts inodes, which calls into DLM
to acquire the glock. The DLM, in turn, may block on a pending
fence operation, which may already be blocked on memory allocation
that caused the slab shrinker to be called in the first place.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/incore.h     |  2 ++
 fs/gfs2/ops_fstype.c |  1 +
 fs/gfs2/quota.c      | 25 +++++++++++++++++++++++++
 fs/gfs2/super.c      | 13 +++++++++++++
 4 files changed, 41 insertions(+)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index a6a3389..a367459 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -757,6 +757,8 @@ struct gfs2_sbd {
 
 	struct task_struct *sd_logd_process;
 	struct task_struct *sd_quotad_process;
+	int sd_iprune; /* inodes to prune */
+	spinlock_t sd_shrinkspin;
 
 	/* Quota stuff */
 
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 4546360..65a69be 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -95,6 +95,7 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
 	spin_lock_init(&sdp->sd_jindex_spin);
 	mutex_init(&sdp->sd_jindex_mutex);
 	init_completion(&sdp->sd_journal_ready);
+	spin_lock_init(&sdp->sd_shrinkspin);
 
 	INIT_LIST_HEAD(&sdp->sd_quota_list);
 	mutex_init(&sdp->sd_quota_mutex);
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index ce7d69a..5810a2c 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1528,14 +1528,39 @@ void gfs2_wake_up_statfs(struct gfs2_sbd *sdp) {
 int gfs2_quotad(void *data)
 {
 	struct gfs2_sbd *sdp = data;
+	struct super_block *sb = sdp->sd_vfs;
 	struct gfs2_tune *tune = &sdp->sd_tune;
 	unsigned long statfs_timeo = 0;
 	unsigned long quotad_timeo = 0;
 	unsigned long t = 0;
 	DEFINE_WAIT(wait);
 	int empty;
+	int rc;
+	struct shrink_control sc = {.gfp_mask = GFP_KERNEL, };
 
 	while (!kthread_should_stop()) {
+		/* TODO: Deal with shrinking of dcache */
+		/* Prune any inode cache intended by the shrinker. */
+		spin_lock(&sdp->sd_shrinkspin);
+		if (sdp->sd_iprune > 0) {
+			sc.nr_to_scan = sdp->sd_iprune;
+			if (sc.nr_to_scan > 1024)
+				sc.nr_to_scan = 1024;
+			sdp->sd_iprune -= sc.nr_to_scan;
+			spin_unlock(&sdp->sd_shrinkspin);
+			rc = prune_icache_sb(sb, &sc);
+			if (rc < 0) {
+				spin_lock(&sdp->sd_shrinkspin);
+				sdp->sd_iprune = 0;
+				spin_unlock(&sdp->sd_shrinkspin);
+			}
+			if (sdp->sd_iprune) {
+				cond_resched();
+				continue;
+			}
+		} else {
+			spin_unlock(&sdp->sd_shrinkspin);
+		}
 
 		/* Update the master statfs file */
 		if (sdp->sd_statfs_force_sync) {
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 9b2ff353..75e8a85 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1667,6 +1667,18 @@ static void gfs2_destroy_inode(struct inode *inode)
 	call_rcu(&inode->i_rcu, gfs2_i_callback);
 }
 
+static long gfs2_prune_icache_sb(struct super_block *sb,
+				 struct shrink_control *sc)
+{
+	struct gfs2_sbd *sdp;
+
+	sdp = sb->s_fs_info;
+	spin_lock(&sdp->sd_shrinkspin);
+	sdp->sd_iprune = sc->nr_to_scan + 1;
+	spin_unlock(&sdp->sd_shrinkspin);
+	return 0;
+}
+
 const struct super_operations gfs2_super_ops = {
 	.alloc_inode		= gfs2_alloc_inode,
 	.destroy_inode		= gfs2_destroy_inode,
@@ -1681,5 +1693,6 @@ const struct super_operations gfs2_super_ops = {
 	.remount_fs		= gfs2_remount_fs,
 	.drop_inode		= gfs2_drop_inode,
 	.show_options		= gfs2_show_options,
+	.prune_icache_sb	= gfs2_prune_icache_sb,
 };
 
-- 
2.5.5

Re: [Cluster-devel] [PATCH 2/2] GFS2: Add a gfs2-specific prune_icache_sb

[Steven Whitehouse]

Hi,

I think the idea looks good. A couple of comments below though...

On 24/06/16 20:50, Bob Peterson wrote:
> This patch adds a new prune_icache_sb function for the VFS slab
> shrinker to call. Trying to directly free the inodes from memory
> might deadlock because it evicts inodes, which calls into DLM
> to acquire the glock. The DLM, in turn, may block on a pending
> fence operation, which may already be blocked on memory allocation
> that caused the slab shrinker to be called in the first place.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/incore.h     |  2 ++
>   fs/gfs2/ops_fstype.c |  1 +
>   fs/gfs2/quota.c      | 25 +++++++++++++++++++++++++
>   fs/gfs2/super.c      | 13 +++++++++++++
>   4 files changed, 41 insertions(+)
>
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index a6a3389..a367459 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -757,6 +757,8 @@ struct gfs2_sbd {
>   
>   	struct task_struct *sd_logd_process;
>   	struct task_struct *sd_quotad_process;
> +	int sd_iprune; /* inodes to prune */
> +	spinlock_t sd_shrinkspin;
>   
>   	/* Quota stuff */
>   
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 4546360..65a69be 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -95,6 +95,7 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
>   	spin_lock_init(&sdp->sd_jindex_spin);
>   	mutex_init(&sdp->sd_jindex_mutex);
>   	init_completion(&sdp->sd_journal_ready);
> +	spin_lock_init(&sdp->sd_shrinkspin);
>   
>   	INIT_LIST_HEAD(&sdp->sd_quota_list);
>   	mutex_init(&sdp->sd_quota_mutex);
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index ce7d69a..5810a2c 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -1528,14 +1528,39 @@ void gfs2_wake_up_statfs(struct gfs2_sbd *sdp) {
>   int gfs2_quotad(void *data)
>   {
>   	struct gfs2_sbd *sdp = data;
> +	struct super_block *sb = sdp->sd_vfs;
>   	struct gfs2_tune *tune = &sdp->sd_tune;
>   	unsigned long statfs_timeo = 0;
>   	unsigned long quotad_timeo = 0;
>   	unsigned long t = 0;
>   	DEFINE_WAIT(wait);
>   	int empty;
> +	int rc;
> +	struct shrink_control sc = {.gfp_mask = GFP_KERNEL, };
>   
>   	while (!kthread_should_stop()) {
> +		/* TODO: Deal with shrinking of dcache */
> +		/* Prune any inode cache intended by the shrinker. */
> +		spin_lock(&sdp->sd_shrinkspin);
> +		if (sdp->sd_iprune > 0) {
> +			sc.nr_to_scan = sdp->sd_iprune;
> +			if (sc.nr_to_scan > 1024)
> +				sc.nr_to_scan = 1024;
> +			sdp->sd_iprune -= sc.nr_to_scan;
> +			spin_unlock(&sdp->sd_shrinkspin);
> +			rc = prune_icache_sb(sb, &sc);
> +			if (rc < 0) {
> +				spin_lock(&sdp->sd_shrinkspin);
> +				sdp->sd_iprune = 0;
> +				spin_unlock(&sdp->sd_shrinkspin);
> +			}
> +			if (sdp->sd_iprune) {
> +				cond_resched();
> +				continue;
> +			}
> +		} else {
> +			spin_unlock(&sdp->sd_shrinkspin);
> +		}
>   
>   		/* Update the master statfs file */
>   		if (sdp->sd_statfs_force_sync) {
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 9b2ff353..75e8a85 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1667,6 +1667,18 @@ static void gfs2_destroy_inode(struct inode *inode)
>   	call_rcu(&inode->i_rcu, gfs2_i_callback);
>   }
>   
> +static long gfs2_prune_icache_sb(struct super_block *sb,
> +				 struct shrink_control *sc)
> +{
> +	struct gfs2_sbd *sdp;
> +
> +	sdp = sb->s_fs_info;
> +	spin_lock(&sdp->sd_shrinkspin);
> +	sdp->sd_iprune = sc->nr_to_scan + 1;
> +	spin_unlock(&sdp->sd_shrinkspin);
> +	return 0;
> +}
This doesn't wake up the thread that will do the reclaim, so that there 
may be a significant delay between the request to shrink and the actual 
shrink. Also, using quotad is not a good plan, since it might itself 
block waiting for memory. This should be done by a thread on its own to 
avoid any deadlock possibility here.

There also appears to be a limit of 1024 to scan per run of quotad, 
which means it would take a very long time to push out any significant 
number, and it does seem a bit arbitrary - was there a reason for 
selecting that number? It would probably be better to simply yield every 
now and then if there are a lot of items to process,

Steve.

> +
>   const struct super_operations gfs2_super_ops = {
>   	.alloc_inode		= gfs2_alloc_inode,
>   	.destroy_inode		= gfs2_destroy_inode,
> @@ -1681,5 +1693,6 @@ const struct super_operations gfs2_super_ops = {
>   	.remount_fs		= gfs2_remount_fs,
>   	.drop_inode		= gfs2_drop_inode,
>   	.show_options		= gfs2_show_options,
> +	.prune_icache_sb	= gfs2_prune_icache_sb,
>   };
>   

Re: [PATCH 1/2] vfs: Add hooks for filesystem-specific prune_icache_sb

[Dave Chinner]

On Fri, Jun 24, 2016 at 02:50:10PM -0500, Bob Peterson wrote:
> This patch adds filesystem-specific callbacks for shrinking the
> inode cache, prune_icache_sb. This is provided for filesystems in
> which the default VFS prune_icache_sb needs to be delayed due to
> filesystem locking requirements not met by vfs.
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  Documentation/filesystems/vfs.txt | 15 +++++++++++++++
>  fs/inode.c                        |  1 +
>  fs/super.c                        |  5 ++++-
>  include/linux/fs.h                |  3 +++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
> index c61a223..7cb4c5c 100644
> --- a/Documentation/filesystems/vfs.txt
> +++ b/Documentation/filesystems/vfs.txt
> @@ -230,6 +230,7 @@ struct super_operations {
>          ssize_t (*quota_write)(struct super_block *, int, const char *, size_t, loff_t);
>  	int (*nr_cached_objects)(struct super_block *);
>  	void (*free_cached_objects)(struct super_block *, int);
> +	long (*prune_icache_sb)(struct super_block *, struct shrink_control *);
>  };

IIRC, I originally proposed this before adding the
nr_cached_objects/free_cached_objects interface because it allowed
filesystems direct control over inode reclaim. I don't recall the
reasons for it being rejected now - I think the main one was that
the inode life cycle was owned by the VFS, not the filesystem, and
that it woul dbe too easy for people to implement their own
shrinkers and get it all wrong and hence screw over the entire
system....

>  All methods are called without any locks being held, unless otherwise
> @@ -319,6 +320,20 @@ or bottom half).
>  	implementations will cause holdoff problems due to large scan batch
>  	sizes.
>  
> +  prune_icache_sb: called by the sb cache shrinking function for the file
> +	filesystem to reduce the number of inodes from slab. This is done to
> +	accomodate file systems that may not be able to immediately remove
> +	inodes from cache, but must queue them to be removed ASAP.
> +
> +	This can happen in GFS2, for example, where evicting an inode
> +	may require an inter-node lock (glock) which makes a call into DLM
> +	(distributed lock manager), which may block for any number of reasons.
> +	For example, it may block because a customer node needs to be fenced,
> +	so the lock cannot be granted until the fencing is complete.
> +	The fencing, in turn, may be blocked for other reasons, such as
> +	memory allocations that caused the shrinker to be called in the first
> +	place. Optional. If not set, the default vfs prune_icache_sb is called.

So why don't you simply do what XFS does? i.e. when ->evict/destroy is
called on an inode, you clean up the VFS portion of the inode but do
not destroy/free it - you simply queue it to an internal list and
then do the cleanup/freeing in your own time?

i.e. why do you need a special callout just to defer freeing to
another thread when we already have hooks than enable you to do
this?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] GFS2: Add a gfs2-specific prune_icache_sb

[Dave Chinner]

On Fri, Jun 24, 2016 at 02:50:11PM -0500, Bob Peterson wrote:
> This patch adds a new prune_icache_sb function for the VFS slab
> shrinker to call. Trying to directly free the inodes from memory
> might deadlock because it evicts inodes, which calls into DLM
> to acquire the glock. The DLM, in turn, may block on a pending
> fence operation, which may already be blocked on memory allocation
> that caused the slab shrinker to be called in the first place.
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>

All sorts of problems with this.

> ---
>  fs/gfs2/incore.h     |  2 ++
>  fs/gfs2/ops_fstype.c |  1 +
>  fs/gfs2/quota.c      | 25 +++++++++++++++++++++++++
>  fs/gfs2/super.c      | 13 +++++++++++++
>  4 files changed, 41 insertions(+)
> 
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index a6a3389..a367459 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -757,6 +757,8 @@ struct gfs2_sbd {
>  
>  	struct task_struct *sd_logd_process;
>  	struct task_struct *sd_quotad_process;
> +	int sd_iprune; /* inodes to prune */
> +	spinlock_t sd_shrinkspin;
>  
>  	/* Quota stuff */
>  
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 4546360..65a69be 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -95,6 +95,7 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
>  	spin_lock_init(&sdp->sd_jindex_spin);
>  	mutex_init(&sdp->sd_jindex_mutex);
>  	init_completion(&sdp->sd_journal_ready);
> +	spin_lock_init(&sdp->sd_shrinkspin);
>  
>  	INIT_LIST_HEAD(&sdp->sd_quota_list);
>  	mutex_init(&sdp->sd_quota_mutex);
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index ce7d69a..5810a2c 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -1528,14 +1528,39 @@ void gfs2_wake_up_statfs(struct gfs2_sbd *sdp) {
>  int gfs2_quotad(void *data)
>  {
>  	struct gfs2_sbd *sdp = data;
> +	struct super_block *sb = sdp->sd_vfs;
>  	struct gfs2_tune *tune = &sdp->sd_tune;
>  	unsigned long statfs_timeo = 0;
>  	unsigned long quotad_timeo = 0;
>  	unsigned long t = 0;
>  	DEFINE_WAIT(wait);
>  	int empty;
> +	int rc;
> +	struct shrink_control sc = {.gfp_mask = GFP_KERNEL, };
>  

Has not set PF_MEMALLOC context here, so memory reclaim operations that
require allocation to complete won't get access to memory reserves and
hence are likely to deadlock.

>  	while (!kthread_should_stop()) {
> +		/* TODO: Deal with shrinking of dcache */

I don't think that's ever going to be allowed.

> +		/* Prune any inode cache intended by the shrinker. */
> +		spin_lock(&sdp->sd_shrinkspin);
> +		if (sdp->sd_iprune > 0) {
> +			sc.nr_to_scan = sdp->sd_iprune;
> +			if (sc.nr_to_scan > 1024)
> +				sc.nr_to_scan = 1024;

Magic number warning. Where's that come from?

> +			sdp->sd_iprune -= sc.nr_to_scan;
> +			spin_unlock(&sdp->sd_shrinkspin);
> +			rc = prune_icache_sb(sb, &sc);
> +			if (rc < 0) {
> +				spin_lock(&sdp->sd_shrinkspin);
> +				sdp->sd_iprune = 0;
> +				spin_unlock(&sdp->sd_shrinkspin);
> +			}
> +			if (sdp->sd_iprune) {
> +				cond_resched();
> +				continue;
> +			}
> +		} else {
> +			spin_unlock(&sdp->sd_shrinkspin);
> +		}

Ok, so this only scans 1024 inodes per loop.  AFAICT, that's  only
once every quotad/statfs timeout, which appears to be 60/30 jiffies
by default.

What this means is that memory pressure is not going to be able to
evict gfs2 inodes at the rate required by memory pressure. If we
have a million cached inodes, and we can only reclaim 1000 every
~50ms, then we've got a severe memory imblance issue. in that same
time, we could be pulling thousands on inodes into the cache....

>  
>  		/* Update the master statfs file */
>  		if (sdp->sd_statfs_force_sync) {
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 9b2ff353..75e8a85 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1667,6 +1667,18 @@ static void gfs2_destroy_inode(struct inode *inode)
>  	call_rcu(&inode->i_rcu, gfs2_i_callback);
>  }
>  
> +static long gfs2_prune_icache_sb(struct super_block *sb,
> +				 struct shrink_control *sc)
> +{
> +	struct gfs2_sbd *sdp;
> +
> +	sdp = sb->s_fs_info;
> +	spin_lock(&sdp->sd_shrinkspin);
> +	sdp->sd_iprune = sc->nr_to_scan + 1;
> +	spin_unlock(&sdp->sd_shrinkspin);
> +	return 0;

And so this is even more problematic. sc->nr_to_scan is set to to
either the shrinker batch size or SHRINK_BATCH (128), and the return
value of the shrinker is the number of objects freed. When the
shrinker calculates the number of objects that need to be freed from
the slab cache, it breaks it up into batch size chunks, and it calls
iteratively until it's scanned the number of objects it calculated.
This is how the shrinkers convey memory pressure - the more calls
made, the higher the memory pressure, the more the cache should be
scanned for reclaim.

The code above, if called iteratively, continually overwrites the
prune count, so it is only ever going to have a maximum of the
shrinker batch size. IOWs, if the kernel wants 100,000 inodes to be
scaned for reclaim, it will make 100 calls into this function, but
this will only pass one call on to the background thread, which then
runs some time in the next 50-500ms to do that reclaim. Hence
reclaim is simply not going to be effective under heavy inode cache
memory pressure.

And then there's an even bigger problem - the superblock shrinker
has set the flags:

	s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE;

This new shrinker is not aware of either node based reclaim or memcg
based reclaim, and hence this will break more than system balance -
it will break containers or anything else that uses memcgs to limit
memory usage of an application. It will probably also trash the gfs2
inode cache system wide as prune_icache_sb() is given no directions
for NUMA or memcg based reclaim.

[ Actually, now that I look at the code and remind myself how the
NUMA aware reclaim works, it's worse than I thought.  Passing a
sc->nid = 0 to prune_icache_sb() means it will only attempt to
shrink the inode cache on node 0. This means the gfs2 inode cache on
nodes 1..N can never be reclaimed by memory pressure. IOWs, it
simply does not work properly.... ]

Finally, returning 0 immediately from the shrinker without doing
anything essentially prevents the superblock shrinker from
throttling inode allocation to the rate at which inodes can be
reclaimed. Because reclaim will now skip all attempts to free
inodes, the memory pressure feedback loop has been broken. e.g. if
you have a pure inode cache workload (e.g. doing lots of
concurrent finds) that is generating all the memory pressure,
there's now nothing to throttle memory allocation to the rate that
memory can be reclaimed. This will cause premature ENOMEM and OOM
situations to occur, as memory reclaim will blow through without
reclaiming any inodes and hence freeing no memory.

Shrinker contexts are complex and the VFS shrinkers are much more
complex than most people understand. I strongly recommend that
filesystems leave inode cache reclaim to the superblock shrinker for
the above reasons - it's far too easy for people to get badly wrong.

If there are specific limitations on how inodes can be freed, then
move the parts of inode *freeing* that cause problems to a different
context via the ->evict/destroy callouts and trigger that external
context processing on demand. That external context can just do bulk
"if it is on the list then free it" processing, because the reclaim
policy has already been executed to place that inode on the reclaim
list.

This is essentially what XFS does, but it also uses the
->nr_cached_objects/->free_cached_objects() callouts in the
superblock shrinker to provide the reclaim rate feedback mechanism
required to throttle incoming memory allocations.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [Cluster-devel] [PATCH 2/2] GFS2: Add a gfs2-specific prune_icache_sb

[Steven Whitehouse]

Hi,

On 28/06/16 03:08, Dave Chinner wrote:
> On Fri, Jun 24, 2016 at 02:50:11PM -0500, Bob Peterson wrote:
>> This patch adds a new prune_icache_sb function for the VFS slab
>> shrinker to call. Trying to directly free the inodes from memory
>> might deadlock because it evicts inodes, which calls into DLM
>> to acquire the glock. The DLM, in turn, may block on a pending
>> fence operation, which may already be blocked on memory allocation
>> that caused the slab shrinker to be called in the first place.
>>
>> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> All sorts of problems with this.
>
>> ---
>>   fs/gfs2/incore.h     |  2 ++
>>   fs/gfs2/ops_fstype.c |  1 +
>>   fs/gfs2/quota.c      | 25 +++++++++++++++++++++++++
>>   fs/gfs2/super.c      | 13 +++++++++++++
>>   4 files changed, 41 insertions(+)
>>
>> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
>> index a6a3389..a367459 100644
>> --- a/fs/gfs2/incore.h
>> +++ b/fs/gfs2/incore.h
>> @@ -757,6 +757,8 @@ struct gfs2_sbd {
>>   
>>   	struct task_struct *sd_logd_process;
>>   	struct task_struct *sd_quotad_process;
>> +	int sd_iprune; /* inodes to prune */
>> +	spinlock_t sd_shrinkspin;
>>   
>>   	/* Quota stuff */
>>   
>> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
>> index 4546360..65a69be 100644
>> --- a/fs/gfs2/ops_fstype.c
>> +++ b/fs/gfs2/ops_fstype.c
>> @@ -95,6 +95,7 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
>>   	spin_lock_init(&sdp->sd_jindex_spin);
>>   	mutex_init(&sdp->sd_jindex_mutex);
>>   	init_completion(&sdp->sd_journal_ready);
>> +	spin_lock_init(&sdp->sd_shrinkspin);
>>   
>>   	INIT_LIST_HEAD(&sdp->sd_quota_list);
>>   	mutex_init(&sdp->sd_quota_mutex);
>> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
>> index ce7d69a..5810a2c 100644
>> --- a/fs/gfs2/quota.c
>> +++ b/fs/gfs2/quota.c
>> @@ -1528,14 +1528,39 @@ void gfs2_wake_up_statfs(struct gfs2_sbd *sdp) {
>>   int gfs2_quotad(void *data)
>>   {
>>   	struct gfs2_sbd *sdp = data;
>> +	struct super_block *sb = sdp->sd_vfs;
>>   	struct gfs2_tune *tune = &sdp->sd_tune;
>>   	unsigned long statfs_timeo = 0;
>>   	unsigned long quotad_timeo = 0;
>>   	unsigned long t = 0;
>>   	DEFINE_WAIT(wait);
>>   	int empty;
>> +	int rc;
>> +	struct shrink_control sc = {.gfp_mask = GFP_KERNEL, };
>>   
> Has not set PF_MEMALLOC context here, so memory reclaim operations that
> require allocation to complete won't get access to memory reserves and
> hence are likely to deadlock.
>
>>   	while (!kthread_should_stop()) {
>> +		/* TODO: Deal with shrinking of dcache */
> I don't think that's ever going to be allowed.
>
>> +		/* Prune any inode cache intended by the shrinker. */
>> +		spin_lock(&sdp->sd_shrinkspin);
>> +		if (sdp->sd_iprune > 0) {
>> +			sc.nr_to_scan = sdp->sd_iprune;
>> +			if (sc.nr_to_scan > 1024)
>> +				sc.nr_to_scan = 1024;
> Magic number warning. Where's that come from?
>
>> +			sdp->sd_iprune -= sc.nr_to_scan;
>> +			spin_unlock(&sdp->sd_shrinkspin);
>> +			rc = prune_icache_sb(sb, &sc);
>> +			if (rc < 0) {
>> +				spin_lock(&sdp->sd_shrinkspin);
>> +				sdp->sd_iprune = 0;
>> +				spin_unlock(&sdp->sd_shrinkspin);
>> +			}
>> +			if (sdp->sd_iprune) {
>> +				cond_resched();
>> +				continue;
>> +			}
>> +		} else {
>> +			spin_unlock(&sdp->sd_shrinkspin);
>> +		}
> Ok, so this only scans 1024 inodes per loop.  AFAICT, that's  only
> once every quotad/statfs timeout, which appears to be 60/30 jiffies
> by default.
>
> What this means is that memory pressure is not going to be able to
> evict gfs2 inodes at the rate required by memory pressure. If we
> have a million cached inodes, and we can only reclaim 1000 every
> ~50ms, then we've got a severe memory imblance issue. in that same
> time, we could be pulling thousands on inodes into the cache....
>
>>   
>>   		/* Update the master statfs file */
>>   		if (sdp->sd_statfs_force_sync) {
>> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
>> index 9b2ff353..75e8a85 100644
>> --- a/fs/gfs2/super.c
>> +++ b/fs/gfs2/super.c
>> @@ -1667,6 +1667,18 @@ static void gfs2_destroy_inode(struct inode *inode)
>>   	call_rcu(&inode->i_rcu, gfs2_i_callback);
>>   }
>>   
>> +static long gfs2_prune_icache_sb(struct super_block *sb,
>> +				 struct shrink_control *sc)
>> +{
>> +	struct gfs2_sbd *sdp;
>> +
>> +	sdp = sb->s_fs_info;
>> +	spin_lock(&sdp->sd_shrinkspin);
>> +	sdp->sd_iprune = sc->nr_to_scan + 1;
>> +	spin_unlock(&sdp->sd_shrinkspin);
>> +	return 0;
> And so this is even more problematic. sc->nr_to_scan is set to to
> either the shrinker batch size or SHRINK_BATCH (128), and the return
> value of the shrinker is the number of objects freed. When the
> shrinker calculates the number of objects that need to be freed from
> the slab cache, it breaks it up into batch size chunks, and it calls
> iteratively until it's scanned the number of objects it calculated.
> This is how the shrinkers convey memory pressure - the more calls
> made, the higher the memory pressure, the more the cache should be
> scanned for reclaim.
>
> The code above, if called iteratively, continually overwrites the
> prune count, so it is only ever going to have a maximum of the
> shrinker batch size. IOWs, if the kernel wants 100,000 inodes to be
> scaned for reclaim, it will make 100 calls into this function, but
> this will only pass one call on to the background thread, which then
> runs some time in the next 50-500ms to do that reclaim. Hence
> reclaim is simply not going to be effective under heavy inode cache
> memory pressure.
>
> And then there's an even bigger problem - the superblock shrinker
> has set the flags:
>
> 	s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE;
>
> This new shrinker is not aware of either node based reclaim or memcg
> based reclaim, and hence this will break more than system balance -
> it will break containers or anything else that uses memcgs to limit
> memory usage of an application. It will probably also trash the gfs2
> inode cache system wide as prune_icache_sb() is given no directions
> for NUMA or memcg based reclaim.
>
> [ Actually, now that I look at the code and remind myself how the
> NUMA aware reclaim works, it's worse than I thought.  Passing a
> sc->nid = 0 to prune_icache_sb() means it will only attempt to
> shrink the inode cache on node 0. This means the gfs2 inode cache on
> nodes 1..N can never be reclaimed by memory pressure. IOWs, it
> simply does not work properly.... ]
>
> Finally, returning 0 immediately from the shrinker without doing
> anything essentially prevents the superblock shrinker from
> throttling inode allocation to the rate at which inodes can be
> reclaimed. Because reclaim will now skip all attempts to free
> inodes, the memory pressure feedback loop has been broken. e.g. if
> you have a pure inode cache workload (e.g. doing lots of
> concurrent finds) that is generating all the memory pressure,
> there's now nothing to throttle memory allocation to the rate that
> memory can be reclaimed. This will cause premature ENOMEM and OOM
> situations to occur, as memory reclaim will blow through without
> reclaiming any inodes and hence freeing no memory.
>
> Shrinker contexts are complex and the VFS shrinkers are much more
> complex than most people understand. I strongly recommend that
> filesystems leave inode cache reclaim to the superblock shrinker for
> the above reasons - it's far too easy for people to get badly wrong.
>
> If there are specific limitations on how inodes can be freed, then
> move the parts of inode *freeing* that cause problems to a different
> context via the ->evict/destroy callouts and trigger that external
> context processing on demand. That external context can just do bulk
> "if it is on the list then free it" processing, because the reclaim
> policy has already been executed to place that inode on the reclaim
> list.
That causes problems, unfortunately. As soon as the inode is taken out 
of the hash, then a new inode can be created by another process doing a 
lookup (for whatever reason - NFS fh lookup, scan for unlinked but still 
allocated inodes) and we'd land up with two in memory inodes referring 
to the same on disk inode at the same time. We rely on the VFS to 
provide mutual exclusion there. We did look at leaving the inode hashed 
and making the evict async, but that didn't work either - the exact 
issue escapes me, but Bob's memory is probably better than mine on this 
issue.

The issue is that there is a dependency loop involving the glocks 
required for inode deallocation, so we have:

  1. Event occurs which requires recovery somewhere in the cluster, 
which memory is short
  2. Userspace gets blocked waiting for memory (on inode shrinker) due 
to requirement for a glock, to complete eviction
  3. The glock is blocked on the DLM
  4. The DLM is blocked until recovery is complete -> deadlock

So we have various option on where to break this loop, the big question 
being where is the best place to do it, as the "obvious" ones seem to 
have various disadvantages to them,

Steve.


>
> This is essentially what XFS does, but it also uses the
> ->nr_cached_objects/->free_cached_objects() callouts in the
> superblock shrinker to provide the reclaim rate feedback mechanism
> required to throttle incoming memory allocations.
>
> Cheers,
>
> Dave.

Re: [Cluster-devel] [PATCH 2/2] GFS2: Add a gfs2-specific prune_icache_sb

[Dave Chinner]

On Tue, Jun 28, 2016 at 10:13:32AM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On 28/06/16 03:08, Dave Chinner wrote:
> >On Fri, Jun 24, 2016 at 02:50:11PM -0500, Bob Peterson wrote:
> >>This patch adds a new prune_icache_sb function for the VFS slab
> >>shrinker to call. Trying to directly free the inodes from memory
> >>might deadlock because it evicts inodes, which calls into DLM
> >>to acquire the glock. The DLM, in turn, may block on a pending
> >>fence operation, which may already be blocked on memory allocation
> >>that caused the slab shrinker to be called in the first place.
> >>
> >>Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> >All sorts of problems with this.
.....
> >Shrinker contexts are complex and the VFS shrinkers are much more
> >complex than most people understand. I strongly recommend that
> >filesystems leave inode cache reclaim to the superblock shrinker for
> >the above reasons - it's far too easy for people to get badly wrong.
> >
> >If there are specific limitations on how inodes can be freed, then
> >move the parts of inode *freeing* that cause problems to a different
> >context via the ->evict/destroy callouts and trigger that external
> >context processing on demand. That external context can just do bulk
> >"if it is on the list then free it" processing, because the reclaim
> >policy has already been executed to place that inode on the reclaim
> >list.
> That causes problems, unfortunately. As soon as the inode is taken
> out of the hash, then a new inode can be created by another process
> doing a lookup (for whatever reason - NFS fh lookup, scan for
> unlinked but still allocated inodes) and we'd land up with two in
> memory inodes referring to the same on disk inode at the same time.

Yes, that's a problem XFS has to deal with, too. the lookup needs to
search the reclaim list, too, and if it is found there recycle that
inode rather than allocate a new one. This generaly involves
clearing the reclaim state,  calling init_inode_always(inode) on the
reclaimed inode and then treating it like a freshly allocated struct
inode...

> We did look at
> leaving the inode hashed and making the evict async, but that didn't
> work either - the exact issue escapes me, but Bob's memory is
> probably better than mine on this issue.

There's no need to play games with hashing if you layer the reclaim
list as per above. The VFS state is always consistent, and the only
times you need to care about inodes in the "limbo" state of waiting
for reclaim is when a new lookup comes in or a superblock state
transition such as freeze, remount-ro and unmount...

> The issue is that there is a dependency loop involving the glocks
> required for inode deallocation, so we have:
> 
>  1. Event occurs which requires recovery somewhere in the cluster,
> which memory is short
>  2. Userspace gets blocked waiting for memory (on inode shrinker)
> due to requirement for a glock, to complete eviction
>  3. The glock is blocked on the DLM
>  4. The DLM is blocked until recovery is complete -> deadlock
>
> So we have various option on where to break this loop, the big
> question being where is the best place to do it, as the "obvious"
> ones seem to have various disadvantages to them,

Yes, I understand the problem. Essentially you have a "non-blocking"
requirement for reclaiming inodes because the DLM dependency means
that reclaim progress cannot be guaranteed. The superblock shrinker
has always assumed that unreferenced inodes can be reclaimed at any
time when GFP_KERNEL reclaim context is given. This is clearly not
true for gfs2 - it is only safe to attempt reclaim on inodes whose
glock can be obtained without blocking.

There are two ways of dealing with this. The first is as I described
above - defer final cleanup/freeing of specific inodes to a separate
list and context out of sight of the VFS inode life cycle and hook
your inode lookup code into it. This way the reclaim of the inode
can be deferred until the glock can be obtained without affecting
normal VFS level operations.

The second is to enable the shrinker to skip inodes that cannot be
immediately reclaimed. The shrinker already does this based on the
internal VFS inode state in inode_lru_isolate(). It would seem to me
that we could add a callout in this function to allow a non-blocking
determination of inode reclaimability by the filesystem. It would
truly have to be non-blocking - I think we could only sanely call it
under the lru and inode spinlocks because if we can't reclaim it
immediately we have to skip the inode. Dropping the locks mean we
can't move the inode in the LRU, and traversal must restart. Hence
we'd replace a deadlock with a livelock if we can't skip inodes we
can't reclaim because we dropped the lru lock.

I don't know whether you can grab a glock in a truly non-blocking
fashion, which is why I didn't initially suggest it. It seems like a
lot more surgery to both the inode reclaim code and the GFS2 code to
enable a hook into reclaim in this way...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com