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?
> 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
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
