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
