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. > > @@ -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)) == ? > 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..
