Re: [PATCH 14/16] xfs: optimize adding the first 8-byte inode to a shortform directory

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

On Thu, May 02, 2024 at 06:25:09AM +0200, Christoph Hellwig wrote:
> On Wed, May 01, 2024 at 02:50:56PM -0700, Darrick J. Wong wrote:
> > I noticed a few places where we pass offset == 0 here.  That's ok as a
> > null value because the start of a shortform directory is always the
> > header, correct?
> 
> The start of the "physical" layout has the header, but offset is the
> "logic" d_offset offset.  The start of it it reserved for (but not
> actually used by) the "." and ".." entries that will occupy the space
> when converted out of the short form.  Probably also needs a comment.
> 
> > Ok, so this isn't needed anymore because the ino8 conversion now adds
> > the new dirent?
> 
> Yes.
> 
> > > -		xfs_dir2_sf_toino8(args);
> > > +		xfs_dir2_sf_toino8(args, 0);
> > 
> > This is a replace, so we pass 0 here effectively as a null value?
> 
> Exactly.

ok good.

> > > @@ -1250,6 +1275,17 @@ xfs_dir2_sf_toino8(
> > >  				xfs_dir2_sf_get_ino(mp, oldsfp, oldsfep));
> > >  		xfs_dir2_sf_put_ftype(mp, sfep,
> > >  				xfs_dir2_sf_get_ftype(mp, oldsfep));
> > > +
> > > +		/*
> > > +		 * If there is a new entry to add it once we reach the specified
> > > +		 * offset.
> > 
> > It took me a minute of staring at the if test logic to figure out what
> > we're doing here.  If, after, reformatting a directory entry, the next
> > entry is the offset where _pick wants us to place the new dirent, we
> > should jump sfep to the next entry, and then add the new entry.
> > 
> > Is that right?  And we can't simplify the logic to:
> > 
> > 	if (new_offset && new_offset = xfs_dir2_sf_get_offset(sfep))
> 
> == ?

Yes, double-equals, not single-equals.

> > Because _pick might want us to add the entry at the end of the directory
> > but we haven't incremented sfp->count yet, so the loop body will not be
> > executed in that case.
> > 
> > Is it ever the case that the entry get added in the middle of a
> > shortform directory?
> 
> Yes, that is the hard case.  There is no good reason to add it in
> the middle, but we've encoded that the "logical" offset for a
> shortform directly needs to fit into the physical size of a single
> directory block when converted to block format in asserts and verifiers
> and are stuck with it.  Otherwise we could have just always added it
> at the end..

<nod> I think the mechanics of this patch look ok, but this:

		xfs_dir2_sf_toino8(args, 0);

worries me because the reader has to know that zero is never a valid
offset for adding a dirent, vs:

#define XFS_DIR2_DATA_AOFF_NULL	((xfs_dir2_data_aoff_t)0)

		xfs_dir2_sf_toino8(args, XFS_DIR2_DATA_AOFF_NULL);

shouts that we're not trying to add anything.

--D