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
