Re: [PATCH 2/3] xfs: speed up parent pointer operations when possible

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

On Wed, Jan 21, 2026 at 07:22:20AM -0800, Christoph Hellwig wrote:
> On Tue, Jan 20, 2026 at 10:39:46PM -0800, Darrick J. Wong wrote:
> > and fall back to the attr intent machinery if that doesn't work.  We can
> > use the same helpers for regular xattrs and parent pointers.
> 
> Not just can, but do :)  This should really help with ACL inheritance
> as well.
> 
> > +/* Can we bypass the attr intent mechanism for better performance? */
> > +static inline bool
> > +xfs_attr_can_shortcut(
> 
> This is really a could and not a can, it might still not be possible
> and we bail out.  Maybe reflect that in at least the comment, if not
> also the name?

How about:

/*
 * Decide if it is theoretically possible to try to bypass the attr
 * intent mechanism for better performance.  Other constraints (e.g.
 * available space in the existing structure) are not considered
 * here.
 */
static inline bool
xfs_attr_can_shortcut(

> > +	if (rmt_blks || !xfs_attr_can_shortcut(args->dp)) {
> > +		xfs_attr_defer_add(args, XFS_ATTR_DEFER_REPLACE);
> > +		return 0;
> > +	}
> > +
> > +	args->op_flags |= XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE;
> > +
> > +	error = xfs_attr_sf_removename(args);
> > +	if (error)
> > +		return error;
> > +	if (args->attr_filter & XFS_ATTR_PARENT) {
> > +		/*
> > +		 * Move the new name/value to the regular name/value slots and
> > +		 * zero out the new name/value slots because we don't need to
> > +		 * log them for a PPTR_SET operation.
> > +		 */
> > +		xfs_attr_update_pptr_replace_args(args);
> > +		args->new_name = NULL;
> > +		args->new_namelen = 0;
> > +		args->new_value = NULL;
> > +		args->new_valuelen = 0;
> > +	}
> > +	args->op_flags &= ~XFS_DA_OP_REPLACE;
> > +
> > +	error = xfs_attr_try_sf_addname(args);
> 
> This mirrors what the state machine would do.  Although I wonder if
> we should simply try to do the replace in one go.  But I guess I can
> look into that as an incremental optimization later.

<nod> I *think* that there might be some benign clumsiness going on with
the actual attr intent machinery -- when we create a pptr replace
operation, we log five things (attri header, old name, new name, old
value, new value).  The attr op remains XFS_ATTRI_OP_FLAGS_PPTR_REPLACE
throughout the intent item's processing, which means that the log
recovery code expects the intent item to have all four buffers even if
the log already recorded removing the old pptr.

Where the weirdness comes is if we relog the intent item after calling
xfs_attr_update_pptr_replace_args.  I think that can cause a shift in
the logged items from (attri header, "foo", "bar", XXX, YYY) to (attri
header, "bar", "bar", YYY, YYY).  It'd look weird in logprint, but the
recovery machinery will do the right thing whether or not it finds
bar/YYY.

For this edge case of replacing a pptr where we deleted the old pptr but
failed to add the new one, we're using PPTR_SET to set the new pptr, so
we only want to log (attri header, new name, new value).

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

Thanks for reading through all these patches!

--D