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:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > I found a race involving the larp control knob, aka the debugging knob
> > that lets developers enable logging of extended attribute updates:
> >
> > Thread 1 Thread 2
> >
> > echo 0 > /sys/fs/xfs/debug/larp
> > setxattr(REPLACE)
> > xfs_has_larp (returns false)
> > xfs_attr_set
> >
> > echo 1 > /sys/fs/xfs/debug/larp
> >
> > xfs_attr_defer_replace
> > xfs_attr_init_replace_state
> > xfs_has_larp (returns true)
> > xfs_attr_init_remove_state
> >
> > <oops, wrong DAS state!>
> >
> > This isn't a particularly severe problem right now because xattr logging
> > is only enabled when CONFIG_XFS_DEBUG=y, and developers *should* know
> > what they're doing.
> >
> > However, the eventual intent is that callers should be able to ask for
> > the assistance of the log in persisting xattr updates. This capability
> > might not be required for /all/ callers, which means that dynamic
> > control must work correctly. Once an xattr update has decided whether
> > or not to use logged xattrs, it needs to stay in that mode until the end
> > of the operation regardless of what subsequent parallel operations might
> > do.
> >
> > Therefore, it is an error to continue sampling xfs_globals.larp once
> > xfs_attr_change has made a decision about larp, and it was not correct
> > for me to have told Allison that ->create_intent functions can sample
> > the global log incompat feature bitfield to decide to elide a log item.
> >
> > Instead, create a new op flag for the xfs_da_args structure, and convert
> > all other callers of xfs_has_larp and xfs_sb_version_haslogxattrs within
> > the attr update state machine to look for the operations flag.
>
> *nod*
>
> ....
>
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 4a28c2d77070..135d44133477 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -413,18 +413,20 @@ xfs_attr_create_intent(
> > struct xfs_mount *mp = tp->t_mountp;
> > struct xfs_attri_log_item *attrip;
> > struct xfs_attr_intent *attr;
> > + struct xfs_da_args *args;
> >
> > ASSERT(count == 1);
> >
> > - if (!xfs_sb_version_haslogxattrs(&mp->m_sb))
> > - return NULL;
> > -
> > /*
> > * Each attr item only performs one attribute operation at a time, so
> > * this is a list of one
> > */
> > attr = list_first_entry_or_null(items, struct xfs_attr_intent,
> > xattri_list);
> > + args = attr->xattri_da_args;
> > +
> > + if (!(args->op_flags & XFS_DA_OP_LOGGED))
> > + return NULL;
>
> Hmmmm. For the non-LARP case, do we need to be checking
>
> if (!attr)
> return NULL;
>
> now?
I don't think that's necessary, since the struct xfs_attr_intent is
always allocated and used to track the incore state of the operation,
even when we aren't going to use the log items.
> > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > index 35e13e125ec6..149a8f537b06 100644
> > --- 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.
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
