* Re: [PATCH] xfs: xfs_buf cache destroy isn't RCU safe
2022-07-18 23:58 ` [PATCH] xfs: xfs_buf cache destroy isn't RCU safe Dave Chinner
@ 2022-07-19 1:22 ` Darrick J. Wong
2022-07-19 3:56 ` Christoph Hellwig
2022-07-28 9:35 ` Carlos Maiolino
2 siblings, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2022-07-19 1:22 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xfs: xfs_buf cache destroy isn't RCU safe
2022-07-18 23:58 ` [PATCH] xfs: xfs_buf cache destroy isn't RCU safe Dave Chinner
2022-07-19 1:22 ` Darrick J. Wong
@ 2022-07-19 3:56 ` Christoph Hellwig
2022-07-28 9:35 ` Carlos Maiolino
2 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2022-07-19 3:56 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: xfs_buf cache destroy isn't RCU safe
2022-07-18 23:58 ` [PATCH] xfs: xfs_buf cache destroy isn't RCU safe Dave Chinner
2022-07-19 1:22 ` Darrick J. Wong
2022-07-19 3:56 ` Christoph Hellwig
@ 2022-07-28 9:35 ` Carlos Maiolino
2 siblings, 0 replies; 4+ messages in thread
From: Carlos Maiolino @ 2022-07-28 9:35 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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>
> ---
> 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
>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
--
Carlos Maiolino
^ permalink raw reply [flat|nested] 4+ messages in thread