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@xxxxxx> Thanks! --D
