Re: [PATCH 17/32] xfs: parent pointer attribute creation

Darrick J. Wong
2024-04-10T21:50:27Z

On Tue, Apr 09, 2024 at 10:44:56PM -0700, Christoph Hellwig wrote:
>
> One thing that might be worth documenting in a comment or at least
> the commit log is why we have this three phase split between
> allocating the daargs, doing all the work and freeing it.
>
> As far as I can tell that is because the da_args need to be around
> until transaction commit because xfs_attr_intent has a pointer to
> the da_args and not a full copy.

Correct. I think the xfs_attr_intent could make its own full copy and
iterate on that, but that would break the existing behavior that the
xfs_attr_set caller can look at the end state of the xfs_attr_set
operation.

AFAICT the only xfs_attr_* callers that care about the end state at all
are xfs_attr_lookup functions (because they might want to get the
value). I don't *think* any of the xfs_attr_set callers actually care.
The log intent creation function will snapshot the
name/value/oldname/newvalue buffers, so we're already doing large(ish)
allocations deep in transaction context.

On the other hand, the current code has fewer copies to make because we
"know" that the da_args has to persist until transaction commit
because...

> So unless the attrs are on stack
> they need to be free after transaction commit, and as the normal
> dir operation args are not on the stack we don't want to add the
> attr one to the stack here. We could probably allocate the da_args
> in the main parent pointer helpers, but that would require a NOFAIL
> allocation and maybe lead to odd calling conventions, but maybe
> someone directly involved can further refine that reasoning.

...the da_args (for the parent pointer op) is already allocated as part
of xfs_parent_args in xfs_parent_start. If the kernel supported alloca
(HAHAAHA) then we wouldn't need the xfs_parent_args_cache.

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

Thanks!

--D