| Message ID | fa2fe2c5-645b-6263-3493-b59b4d096488@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [V3,RFC] xfsprogs: remove stubbed-out kernel functions out from xfs_shared.h | expand |
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@redhat.com> > --- > > 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. Yuck. Now I wish I'd pushed back harder on the patch author (Dave) to provide the xfsprogs version of this, or whatever fixes are needed to libxfs-diff to deuglify whatever the result was, rather than let this fall to the maintainer (Eric). :/ --D > > 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); > } > @@ -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__ */ > } > 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: > 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 -- */ > /* > @@ -75,6 +78,7 @@ struct xfs_perag { > spinlock_t pag_state_lock; > spinlock_t pagb_lock; /* lock for pagb_tree */ > +#ifdef __KERNEL__ > struct rb_root pagb_tree; /* ordered tree of busy extents */ > unsigned int pagb_gen; /* generation count for pagb_tree */ > wait_queue_head_t pagb_wait; /* woken when pagb_gen changes */ > @@ -90,9 +94,6 @@ struct xfs_perag { > spinlock_t pag_buf_lock; /* lock for pag_buf_hash */ > struct rhashtable pag_buf_hash; > - /* for rcu-safe freeing */ > - struct rcu_head rcu_head; > - > /* background prealloc block trimming */ > struct delayed_work pag_blockgc_work; > @@ -102,6 +103,7 @@ struct xfs_perag { > * or have some other means to control concurrency. > */ > struct rhashtable pagi_unlinked_hash; > +#endif /* __KERNEL__ */ > }; > int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, > diff --git a/libxfs/xfs_shared.h b/libxfs/xfs_shared.h > index bafee48c..25c4cab5 100644 > --- a/libxfs/xfs_shared.h > +++ b/libxfs/xfs_shared.h > @@ -180,24 +180,4 @@ struct xfs_ino_geometry { > }; > -/* Faked up kernel bits */ > -struct rb_root { > -}; > - > -#define RB_ROOT (struct rb_root) { } > - > -typedef struct wait_queue_head { > -} wait_queue_head_t; > - > -#define init_waitqueue_head(wqh) do { } while(0) > - > -struct rhashtable { > -}; > - > -struct delayed_work { > -}; > - > -#define INIT_DELAYED_WORK(work, func) do { } while(0) > -#define cancel_delayed_work_sync(work) do { } while(0) > - > #endif /* __XFS_SHARED_H__ */ >
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@redhat.com> > --- > > 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.
On 11/4/21 5:38 PM, Dave Chinner wrote: > 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. Ok. I will split this up into the right patch granularity, but is this the endpoint you're looking for? One #ifdef in each of xfs_ag.[ch], two total. The delayed work init/cancel assymmetry is a little odd, but I'll get over it. diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h index 15bae1ff..2ca3b9b2 100644 --- a/libxfs/libxfs_priv.h +++ b/libxfs/libxfs_priv.h @@ -673,6 +673,9 @@ static inline void xfs_iunlink_destroy(struct xfs_perag *pag) { } xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *mp, xfs_agnumber_t agcount); +/* Faked up kernel bits */ +#define cancel_delayed_work_sync(work) do { } while(0) + /* Keep static checkers quiet about nonstatic functions by exporting */ int xfs_rtbuf_get(struct xfs_mount *mp, struct xfs_trans *tp, xfs_rtblock_t block, int issum, struct xfs_buf **bpp); diff --git a/libxfs/xfs_ag.c b/libxfs/xfs_ag.c index 9eda6eba..149f9857 100644 --- a/libxfs/xfs_ag.c +++ b/libxfs/xfs_ag.c @@ -246,6 +246,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); @@ -255,6 +256,7 @@ xfs_initialize_perag( init_waitqueue_head(&pag->pagb_wait); pag->pagb_count = 0; pag->pagb_tree = RB_ROOT; +#endif /* __KERNEL_ */ error = xfs_buf_hash_init(pag); if (error) diff --git a/libxfs/xfs_ag.h b/libxfs/xfs_ag.h index 4c6f9045..ef04a537 100644 --- a/libxfs/xfs_ag.h +++ b/libxfs/xfs_ag.h @@ -64,8 +64,11 @@ struct xfs_perag { /* Blocks reserved for the reverse mapping btree. */ struct xfs_ag_resv pag_rmapbt_resv; - /* -- kernel only structures below this line -- */ + /* for rcu-safe freeing */ + struct rcu_head rcu_head; +#ifdef __KERNEL__ + /* -- kernel only structures below this line -- */ /* * Bitsets of per-ag metadata that have been checked and/or are sick. * Callers should hold pag_state_lock before accessing this field. @@ -90,9 +93,6 @@ struct xfs_perag { spinlock_t pag_buf_lock; /* lock for pag_buf_hash */ struct rhashtable pag_buf_hash; - /* for rcu-safe freeing */ - struct rcu_head rcu_head; - /* background prealloc block trimming */ struct delayed_work pag_blockgc_work; @@ -102,6 +102,7 @@ struct xfs_perag { * or have some other means to control concurrency. */ struct rhashtable pagi_unlinked_hash; +#endif /* __KERNEL__ */ }; int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, diff --git a/libxfs/xfs_shared.h b/libxfs/xfs_shared.h index bafee48c..25c4cab5 100644 --- a/libxfs/xfs_shared.h +++ b/libxfs/xfs_shared.h @@ -180,24 +180,4 @@ struct xfs_ino_geometry { }; -/* Faked up kernel bits */ -struct rb_root { -}; - -#define RB_ROOT (struct rb_root) { } - -typedef struct wait_queue_head { -} wait_queue_head_t; - -#define init_waitqueue_head(wqh) do { } while(0) - -struct rhashtable { -}; - -struct delayed_work { -}; - -#define INIT_DELAYED_WORK(work, func) do { } while(0) -#define cancel_delayed_work_sync(work) do { } while(0) - #endif /* __XFS_SHARED_H__ */
On Fri, Nov 05, 2021 at 11:40:57AM -0500, Eric Sandeen wrote: > On 11/4/21 5:38 PM, Dave Chinner wrote: > > > 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. > Ok. > > I will split this up into the right patch granularity, but is this the > endpoint you're looking for? One #ifdef in each of xfs_ag.[ch], two total. Yup. > The delayed work init/cancel assymmetry is a little odd, but I'll > get over it. > > diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h > index 15bae1ff..2ca3b9b2 100644 > --- a/libxfs/libxfs_priv.h > +++ b/libxfs/libxfs_priv.h > @@ -673,6 +673,9 @@ static inline void xfs_iunlink_destroy(struct xfs_perag *pag) { } > xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *mp, > xfs_agnumber_t agcount); > +/* Faked up kernel bits */ > +#define cancel_delayed_work_sync(work) do { } while(0) Comment is completely redundant. libxfs_priv.h is entirely for "faked up kernel bits". I'd also put this up near the top of the file near the definition of struct iomap, not place it randomly in amongst a bunch of XFS definitions. Cheers, Dave.
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); } @@ -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__ */ } 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: 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 -- */ /* @@ -75,6 +78,7 @@ struct xfs_perag { spinlock_t pag_state_lock; spinlock_t pagb_lock; /* lock for pagb_tree */ +#ifdef __KERNEL__ struct rb_root pagb_tree; /* ordered tree of busy extents */ unsigned int pagb_gen; /* generation count for pagb_tree */ wait_queue_head_t pagb_wait; /* woken when pagb_gen changes */ @@ -90,9 +94,6 @@ struct xfs_perag { spinlock_t pag_buf_lock; /* lock for pag_buf_hash */ struct rhashtable pag_buf_hash; - /* for rcu-safe freeing */ - struct rcu_head rcu_head; - /* background prealloc block trimming */ struct delayed_work pag_blockgc_work; @@ -102,6 +103,7 @@ struct xfs_perag { * or have some other means to control concurrency. */ struct rhashtable pagi_unlinked_hash; +#endif /* __KERNEL__ */ }; int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t agcount, diff --git a/libxfs/xfs_shared.h b/libxfs/xfs_shared.h index bafee48c..25c4cab5 100644 --- a/libxfs/xfs_shared.h +++ b/libxfs/xfs_shared.h @@ -180,24 +180,4 @@ struct xfs_ino_geometry { }; -/* Faked up kernel bits */ -struct rb_root { -}; - -#define RB_ROOT (struct rb_root) { } - -typedef struct wait_queue_head { -} wait_queue_head_t; - -#define init_waitqueue_head(wqh) do { } while(0) - -struct rhashtable { -}; - -struct delayed_work { -}; - -#define INIT_DELAYED_WORK(work, func) do { } while(0) -#define cancel_delayed_work_sync(work) do { } while(0) - #endif /* __XFS_SHARED_H__ */
Remove these kernel stubs by #ifdeffing code instead. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- 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.