Re: [PATCHSET v10r1d2 00/17] xfs: encode parent pointer name in xattr key

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Mar 24, 2023 at 8:19 PM Allison Henderson
<allison.henderson@oracle.com> wrote:
>
> On Thu, 2023-03-16 at 12:17 -0700, Darrick J. Wong wrote:
> > Hi all,
> >
> > As I've mentioned in past comments on the parent pointers patchset,
> > the
> > proposed ondisk parent pointer format presents a major difficulty for
> > online directory repair.  This difficulty derives from encoding the
> > directory offset of the dirent that the parent pointer is mirroring.
> > Recall that parent pointers are stored in extended attributes:
> >
> >     (parent_ino, parent_gen, diroffset) -> (dirent_name)
> >
> > If the directory is rebuilt, the offsets of the new directory entries
> > must match the diroffset encoded in the parent pointer, or the
> > filesystem becomes inconsistent.  There are a few ways to solve this
> > problem.
> >
> > One approach would be to augment the directory addname function to
> > take
> > a diroffset and try to create the new entry at that offset.  This
> > will
> > not work if the original directory became corrupt and the parent
> > pointers were written out with impossible diroffsets (e.g.
> > overlapping).
> > Requiring matching diroffsets also prevents reorganization and
> > compaction of directories.
> >
> > This could be remedied by recording the parent pointer diroffset
> > updates
> > necessary to retain consistency, and using the logged parent pointer
> > replace function to rewrite parent pointers as necessary.  This is a
> > poor choice from a performance perspective because the logged xattr
> > updates must be committed in the same transaction that commits the
> > new
> > directory structure.  If there are a large number of diroffset
> > updates,
> > then the directory commit could take an even longer time.
> >
> > Worse yet, if the logged xattr updates fill up the transaction,
> > repair
> > will have no choice but to roll to a fresh transaction to continue
> > logging.  This breaks repair's policy that repairs should commit
> > atomically.  It may break the filesystem as well, since all files
> > involved are pinned until the delayed pptr xattr processing
> > completes.
> > This is a completely bad engineering choice.
> >
> > Note that the diroffset information is not used anywhere in the
> > directory lookup code.  Observe that the only information that we
> > require for a parent pointer is the inverse of an pre-ftype dirent,
> > since this is all we need to reconstruct a directory entry:
> >
> >     (parent_ino, dirent_name) -> NULL
> >
> > The xattr code supports xattrs with zero-length values, surprisingly.
> > The parent_gen field makes it easy to export parent handle
> > information,
> > so it can be retained:
> >
> >     (parent_ino, parent_gen, dirent_name) -> NULL
> >
> > Moving the ondisk format to this format is very advantageous for
> > repair
> > code.  Unfortunately, there is one hitch: xattr names cannot exceed
> > 255
> > bytes due to ondisk format limitations.  We don't want to constrain
> > the
> > length of dirent names, so instead we create a special VLOOKUP mode
> > for
> > extended attributes that allows parent pointers to require matching
> > on
> > both the name and the value.
> >
> > The ondisk format of a parent pointer can then become:
> >
> >     (parent_ino, parent_gen, dirent_name[0:242]) ->
> > (dirent_name[243:255])

With VLOOKUP in place, why is this better than
(parent_ino, parent_gen) -> (dirent_name)

Is it because the dabtree hash is calculated only on the key
and changing that would be way more intrusive?

Does that mean that user can create up to 2^12
parent pointers with the same hash and we are fine with it?

I don't think it is a problem, just wanted to understand and
ask that the reason for this part in the format be explained.

Thanks,
Amir.