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@xxxxxx>
>
