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