On Sun, Jun 05, 2022 at 05:08:46PM -0700, Darrick J. Wong wrote: > On Mon, Jun 06, 2022 at 08:47:43AM +1000, Dave Chinner wrote: > > On Sun, Jun 05, 2022 at 09:37:01AM -0700, Darrick J. Wong wrote: > > > --- a/fs/xfs/xfs_xattr.c > > > +++ b/fs/xfs/xfs_xattr.c > > > @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist( > > > xlog_drop_incompat_feat(mp->m_log); > > > } > > > > > > +#ifdef DEBUG > > > +static inline bool > > > +xfs_attr_want_log_assist( > > > + struct xfs_mount *mp) > > > +{ > > > + /* Logged xattrs require a V5 super for log_incompat */ > > > + return xfs_has_crc(mp) && xfs_globals.larp; > > > +} > > > +#else > > > +# define xfs_attr_want_log_assist(mp) false > > > +#endif > > > > If you are moving this code, let's clean it up a touch so that it > > is the internal logic that is conditional, not the function itself. > > > > static inline bool > > xfs_attr_want_log_assist( > > struct xfs_mount *mp) > > { > > #ifdef DEBUG > > /* Logged xattrs require a V5 super for log_incompat */ > > return xfs_has_crc(mp) && xfs_globals.larp; > > #else > > return false; > > #endif > > } > > I don't mind turning this into a straight function move. I'd figured > that Linus' style preference is usually against putting conditional > compilation inside functions, but for a static inline I really don't > care either way. Well, for conditional helper functions, having the static inline for type checking in all builds is better than having a macro that makes it go away silently when those build options are not turned on. Better would probably be: if (!IS_ENABLED(CONFIG_XFS_DEBUG)) return false; /* Logged xattrs require a V5 super for log_incompat */ return xfs_has_crc(mp) && xfs_globals.larp; And all the ifdef mess goes away at that point. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx