On Thu, Nov 04, 2021 at 12:15:04PM -0500, Eric Sandeen wrote:
> Remove these kernel stubs by #ifdeffing code instead.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> Dave preferred #ifdefs over stubs, and this is what I came up with.
>
> Honestly, I think this is worse, and will lead to more libxfs-sync pain
> unless we're willing to scatter #ifdefs around the kernel code as well,
> but I figured I'd put this out there for discussion.
>
> diff --git a/libxfs/xfs_ag.c b/libxfs/xfs_ag.c
> index 9eda6eba..c01986f7 100644
> --- a/libxfs/xfs_ag.c
> +++ b/libxfs/xfs_ag.c
> @@ -170,7 +170,9 @@ __xfs_free_perag(
> {
> struct xfs_perag *pag = container_of(head, struct xfs_perag, rcu_head);
> +#ifdef __KERNEL__
> ASSERT(!delayed_work_pending(&pag->pag_blockgc_work));
> +#endif /* __KERNEL__ */
> ASSERT(atomic_read(&pag->pag_ref) == 0);
> kmem_free(pag);
> }
> @@ -192,9 +194,11 @@ xfs_free_perag(
> ASSERT(pag);
> ASSERT(atomic_read(&pag->pag_ref) == 0);
> +#ifdef __KERNEL__
> cancel_delayed_work_sync(&pag->pag_blockgc_work);
> xfs_iunlink_destroy(pag);
> xfs_buf_hash_destroy(pag);
> +#endif /* __KERNEL__ */
> call_rcu(&pag->rcu_head, __xfs_free_perag);
> }
These can be stubbed in libxfs_priv.h as we do with all other kernel
functions we don't use:
#define delayed_work_pending(a) ((void)0)
#define cancel_delayed_work_sync(a) ((void)0)
#define xfs_iunlink_destroy(a) ((void)0)
#define xfs_buf_hash_destroy(a) ((void)0)
That is the normal way we avoid needing these ifdef KERNEL clauses
in the libxfs C code.
> @@ -246,6 +250,7 @@ xfs_initialize_perag(
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> +#ifdef __KERNEL__
> /* Place kernel structure only init below this point. */
> spin_lock_init(&pag->pag_ici_lock);
> spin_lock_init(&pag->pagb_lock);
> @@ -267,6 +272,7 @@ xfs_initialize_perag(
> /* first new pag is fully initialized */
> if (first_initialised == NULLAGNUMBER)
> first_initialised = index;
> +#endif /* __KERNEL__ */
> }
endif is in the wrong place - it needs to be before the
first_initialised checks because that is necessary for error
unwinding.
> index = xfs_set_inode_alloc(mp, agcount);
> @@ -277,10 +283,12 @@ xfs_initialize_perag(
> mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
> return 0;
> +#ifdef __KERNEL__
> out_hash_destroy:
> xfs_buf_hash_destroy(pag);
> out_remove_pag:
> radix_tree_delete(&mp->m_perag_tree, index);
> +#endif /* __KERNEL__ */
> out_free_pag:
> kmem_free(pag);
> out_unwind_new_pags:
Again, stubbing out the functions like so:
#define xfs_buf_hash_init(a) ((void)0)
#define xfs_unlink_init(a) ((void)0)
means that the conditional init code doesn't need to be ifdef'd out
and so the error unwinding doesn't need to be ifdef'd out, either.
And, FWIW, you missed the xfs_buf_hash_destroy/xfs_iunlink_destroy
calls in the unwinding code....
> diff --git a/libxfs/xfs_ag.h b/libxfs/xfs_ag.h
> index 4c6f9045..dda1303e 100644
> --- a/libxfs/xfs_ag.h
> +++ b/libxfs/xfs_ag.h
> @@ -64,6 +64,9 @@ struct xfs_perag {
> /* Blocks reserved for the reverse mapping btree. */
> struct xfs_ag_resv pag_rmapbt_resv;
> + /* for rcu-safe freeing */
> + struct rcu_head rcu_head;
> +
> /* -- kernel only structures below this line -- */
> /*
Moving the rcu_head needs to be done in a separate patch, as that
needs to be done on the kernel side, too. When this change went into
the kernel, we didn't have userspace RCU so it was considered a
kernel only structure....
With those changes, we end up with some new stubs in libxfs_priv.h
and two places where we need #ifdef __KERNEL__ in xfs_ag.[ch]. Most
of the mess in this patch goes away....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
