Re: [PATCH] xfs: xfs_buf cache destroy isn't RCU safe

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 19, 2022 at 09:58:51AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Darrick and Sachin Sant reported that xfs/435 and xfs/436 would
> report an non-empty xfs_buf slab on module remove. This isn't easily
> to reproduce, but is clearly a side effect of converting the buffer
> caceh to RUC freeing and lockless lookups. Sachin bisected and
> Darrick hit it when testing the patchset directly.
> 
> Turns out that the xfs_buf slab is not destroyed when all the other
> XFS slab caches are destroyed. Instead, it's got it's own little
> wrapper function that gets called separately, and so it doesn't have
> an rcu_barrier() call in it that is needed to drain all the rcu
> callbacks before the slab is destroyed.
> 
> Fix it by removing the xfs_buf_init/terminate wrappers that just
> allocate and destroy the xfs_buf slab, and move them to the same
> place that all the other slab caches are set up and destroyed.
> 
> Reported-and-tested-by: Sachin Sant <sachinp@linux.ibm.com>
> Fixes: 298f34224506 ("xfs: lockless buffer lookup")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Makes sense to put the slab teardown in the same place as all the other
slab teardowns,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_buf.c   | 25 +------------------------
>  fs/xfs/xfs_buf.h   |  6 ++----
>  fs/xfs/xfs_super.c | 22 +++++++++++++---------
>  3 files changed, 16 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ccc09dc27e46..8878b0069854 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -21,7 +21,7 @@
>  #include "xfs_error.h"
>  #include "xfs_ag.h"
>  
> -static struct kmem_cache *xfs_buf_cache;
> +struct kmem_cache *xfs_buf_cache;
>  
>  /*
>   * Locking orders
> @@ -2302,29 +2302,6 @@ xfs_buf_delwri_pushbuf(
>  	return error;
>  }
>  
> -int __init
> -xfs_buf_init(void)
> -{
> -	xfs_buf_cache = kmem_cache_create("xfs_buf", sizeof(struct xfs_buf), 0,
> -					 SLAB_HWCACHE_ALIGN |
> -					 SLAB_RECLAIM_ACCOUNT |
> -					 SLAB_MEM_SPREAD,
> -					 NULL);
> -	if (!xfs_buf_cache)
> -		goto out;
> -
> -	return 0;
> -
> - out:
> -	return -ENOMEM;
> -}
> -
> -void
> -xfs_buf_terminate(void)
> -{
> -	kmem_cache_destroy(xfs_buf_cache);
> -}
> -
>  void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
>  {
>  	/*
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index d5a9c5ce763d..a561f6ee4c1d 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -15,6 +15,8 @@
>  #include <linux/uio.h>
>  #include <linux/list_lru.h>
>  
> +extern struct kmem_cache *xfs_buf_cache;
> +
>  /*
>   *	Base types
>   */
> @@ -306,10 +308,6 @@ extern int xfs_buf_delwri_submit(struct list_head *);
>  extern int xfs_buf_delwri_submit_nowait(struct list_head *);
>  extern int xfs_buf_delwri_pushbuf(struct xfs_buf *, struct list_head *);
>  
> -/* Buffer Daemon Setup Routines */
> -extern int xfs_buf_init(void);
> -extern void xfs_buf_terminate(void);
> -
>  static inline xfs_daddr_t xfs_buf_daddr(struct xfs_buf *bp)
>  {
>  	return bp->b_maps[0].bm_bn;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 4edee1d3784a..3d27ba1295c9 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1967,11 +1967,19 @@ xfs_init_caches(void)
>  {
>  	int		error;
>  
> +	xfs_buf_cache = kmem_cache_create("xfs_buf", sizeof(struct xfs_buf), 0,
> +					 SLAB_HWCACHE_ALIGN |
> +					 SLAB_RECLAIM_ACCOUNT |
> +					 SLAB_MEM_SPREAD,
> +					 NULL);
> +	if (!xfs_buf_cache)
> +		goto out;
> +
>  	xfs_log_ticket_cache = kmem_cache_create("xfs_log_ticket",
>  						sizeof(struct xlog_ticket),
>  						0, 0, NULL);
>  	if (!xfs_log_ticket_cache)
> -		goto out;
> +		goto out_destroy_buf_cache;
>  
>  	error = xfs_btree_init_cur_caches();
>  	if (error)
> @@ -2145,6 +2153,8 @@ xfs_init_caches(void)
>  	xfs_btree_destroy_cur_caches();
>   out_destroy_log_ticket_cache:
>  	kmem_cache_destroy(xfs_log_ticket_cache);
> + out_destroy_buf_cache:
> +	kmem_cache_destroy(xfs_buf_cache);
>   out:
>  	return -ENOMEM;
>  }
> @@ -2178,6 +2188,7 @@ xfs_destroy_caches(void)
>  	xfs_defer_destroy_item_caches();
>  	xfs_btree_destroy_cur_caches();
>  	kmem_cache_destroy(xfs_log_ticket_cache);
> +	kmem_cache_destroy(xfs_buf_cache);
>  }
>  
>  STATIC int __init
> @@ -2283,13 +2294,9 @@ init_xfs_fs(void)
>  	if (error)
>  		goto out_destroy_wq;
>  
> -	error = xfs_buf_init();
> -	if (error)
> -		goto out_mru_cache_uninit;
> -
>  	error = xfs_init_procfs();
>  	if (error)
> -		goto out_buf_terminate;
> +		goto out_mru_cache_uninit;
>  
>  	error = xfs_sysctl_register();
>  	if (error)
> @@ -2346,8 +2353,6 @@ init_xfs_fs(void)
>  	xfs_sysctl_unregister();
>   out_cleanup_procfs:
>  	xfs_cleanup_procfs();
> - out_buf_terminate:
> -	xfs_buf_terminate();
>   out_mru_cache_uninit:
>  	xfs_mru_cache_uninit();
>   out_destroy_wq:
> @@ -2373,7 +2378,6 @@ exit_xfs_fs(void)
>  	kset_unregister(xfs_kset);
>  	xfs_sysctl_unregister();
>  	xfs_cleanup_procfs();
> -	xfs_buf_terminate();
>  	xfs_mru_cache_uninit();
>  	xfs_destroy_workqueues();
>  	xfs_destroy_caches();
> -- 
> 2.36.1
>