Re: [PATCH 8/9] xfs: collapse the ->create_done functions

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Dec 04, 2023 at 06:24:03AM +0100, Christoph Hellwig wrote:
> > +static struct xfs_log_item *
> > +xfs_attr_create_done(
> > +	struct xfs_trans		*tp,
> > +	struct xfs_log_item		*intent,
> > +	unsigned int			count)
> >  {
> > -	struct xfs_attrd_log_item		*attrdp;
> > +	struct xfs_attri_log_item	*attrip;
> > +	struct xfs_attrd_log_item	*attrdp;
> >  
> > -	ASSERT(tp != NULL);
> > +	if (!intent)
> > +		return NULL;
> > +
> > +	attrip = ATTRI_ITEM(intent);
> 
> How can we end up with a NULL intent here?

static struct xfs_log_item *
xfs_attr_create_intent(
	struct xfs_trans		*tp,
	struct list_head		*items,
	unsigned int			count,
	bool				sort)
{
	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);

	/*
	 * 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;

If the caller doesn't set XFS_DA_OP_LOGGED, then this function returns
NULL for "no log intent item".  The LOGGED flag gets set sometimes:

int
xfs_attr_change(
	struct xfs_da_args	*args)
{
	struct xfs_mount	*mp = args->dp->i_mount;
	bool			use_logging = false;
	int			error;

	ASSERT(!(args->op_flags & XFS_DA_OP_LOGGED));

	if (xfs_attr_want_log_assist(mp)) {
		error = xfs_attr_grab_log_assist(mp);
		if (error)
			return error;

>>>		args->op_flags |= XFS_DA_OP_LOGGED;
		use_logging = true;
	}

But only on a V5 filesystem with a debug kernel and only if
xfs_globals.larp is set.

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
}

> The intent passed in is
> always ->dfp_intent and I don't think that can be NULL.  No other
> implementation of ->create_done checks for it either.

If xfs_attr_create_intent returns NULL, then xfs_attr_create_done won't
create a done item either.  xfs_defer_finish_one will walk through the
state machine as always, but the operation won't be restarted by
recovery since the higher level operation state was not recorded in the
log.

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>