Re: [PATCH 4/5] xfs: correct the parent pointer space reservation comment

[Darrick J. Wong]

On Sat, Aug 08, 2026 at 05:40:21PM -0600, Javier Tia wrote:
> The comment on xfs_parent_calc_space_res() claims parent pointers are
> "always the first attr in an attr tree".  They are not: a parent pointer
> is recorded per dirent, so an inode with N hardlinks carries N of them,
> and `xfs_io -c "parent -p"` on a 31-link file lists 31.  By the Nth link
> the attr fork is in leaf or node format and the insert is not into a
> fresh tree.
> 
> The reservation itself is fine, which is what makes the comment worth
> fixing rather than the code.  XFS_DAENTER_SPACE_RES() reserves
> XFS_DA_NODE_MAXDEPTH blocks plus a bmap allowance for each, i.e. enough
> to split every level of a maximum-depth attr dabtree.  That depth is a
> format ceiling, not a runtime property, so the result cannot depend on
> the format the fork happens to be in.  Anyone auditing a reservation
> shortfall here reads the comment, concludes the sizing rests on an
> assumption that demonstrably does not hold, and goes looking for a bug
> that is not there.
> 
> Record why no double split allowance is needed either, since that is one
> of two visible differences from xfs_attr_calc_size() and is not obvious
> from the expression: a parent pointer's name is a dirent name and its
> value is a struct xfs_parent_rec, so the leaf entry is local and at most
> round_up(3 + 255 + 12, 4) = 272 bytes.  Parent pointers require V5 and
> therefore XFS_MIN_CRC_BLOCKSIZE, so the smallest half-block this can be
> compared against is 512 and the double split branch is unreachable on
> every mountable geometry.  Locality is decided against a different
> threshold, xfs_attr_leaf_entsize_local_max() at three quarters of a
> block, which the 272 bytes also clears.
> 
> Record the other difference too.  The second term hands a byte count to
> XFS_NEXTENTADD_SPACE_RES(), whose parameter counts mappings, so it asks
> for more extent-add allowance than the one mapping a parent pointer
> adds.  The factor depends on the block size, because the macro divides
> by XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(), so the comment says only that it
> over-reserves - a patch whose whole point is that the old comment stated
> a geometry-dependent thing as invariant should not do the same.  That it
> over-reserves is why it is not a bug and why this patch leaves it alone.
> 
> Signed-off-by: Javier Tia <floss@xxxxxxx>
> ---
>  fs/xfs/libxfs/xfs_trans_space.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_space.c b/fs/xfs/libxfs/xfs_trans_space.c
> index 9b8f495c9049..c4cd547033e5 100644
> --- a/fs/xfs/libxfs/xfs_trans_space.c
> +++ b/fs/xfs/libxfs/xfs_trans_space.c
> @@ -22,8 +22,23 @@ xfs_parent_calc_space_res(
>  	unsigned int		namelen)
>  {
>  	/*
> -	 * Parent pointers are always the first attr in an attr tree, and never
> -	 * larger than a block
> +	 * A parent pointer is recorded per dirent, so an inode with N links
> +	 * carries N of them and the attr fork can already be in leaf or node
> +	 * format when one is added.  That does not affect the reservation:
> +	 * XFS_DAENTER_SPACE_RES covers a split at every level of a
> +	 * maximum-depth attr dabtree, whatever format the fork is in now.
> +	 *
> +	 * The name is a dirent name and the value is a struct xfs_parent_rec,
> +	 * so the leaf entry is always local and never exceeds 272 bytes.
> +	 * Parent pointers require V5, hence a 1k minimum block size, so the
> +	 * entry always stays under half a block and this needs none of the
> +	 * double split allowance that xfs_attr_calc_size() makes.
> +	 *
> +	 * The second term hands a byte count to a macro whose parameter counts
> +	 * mappings, so it asks for more extent-add allowance than the single
> +	 * mapping a parent pointer adds - how much more depends on the block
> +	 * size.  It over-reserves either way, which is why it is left alone:
> +	 * correcting the unit would shrink a reservation that is only generous.

We should probably reduce this some day (new feature bit), but in the
meantime this checks out.  The commit message could be a lot shorter
since you don't need to reiterate the diff there....

Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>

--D

>  	 */
>  	return XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) +
>  	       XFS_NEXTENTADD_SPACE_RES(mp, namelen, XFS_ATTR_FORK);
> -- 
> Javier Tia
>