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

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

On Fri, Mar 24, 2023 at 05:10:19PM +0000, Allison Henderson 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])
> > 
> > Because we can always look up a specific parent pointer.  Most of the
> > patches in this patchset prepare the high level xattr code and the
> > lower
> > level logging code to do this correctly, and the last patch switches
> > the
> > ondisk format.
> > 
> > If you're going to start using this mess, you probably ought to just
> > pull from my git trees, which are linked below.
> > 
> > This is an extraordinary way to destroy everything.  Enjoy!
> > Comments and questions are, as always, welcome.
> > kernel git tree:
> So I gave this set a look over and for the most part I think it's ok as
> long as folks agree on the new format?  It's a lot different from what
> folks originally articulated that they wanted, but for the most part
> the format change doesnt affect parent pointer mechanics as much as it
> will affect other features that may use it.  I havnt seen much
> complaints in response, so if it's all the same to everyone else, I am
> fine with moving forward with it?  Thanks all!

Yes, let's move forward.   As I mentioned on IRC last night I'll merge
my changes into parent pointers v11 and put that out next weekish.

--D

> Allison
> 
> 
> > https://urldefense.com/v3/__https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=pptrs-name-in-attr-key__;!!ACWV5N9M2RV99hQ!NDopQXFVRjDfqgJvGMQRzS3fXyyd1SC4HfGl4dxSMWxNPsNKScecjp7bZYX-dDhL5tatk_xxJFdQwCj9YNGq$
> >  
> > 
> > xfsprogs git tree:
> > https://urldefense.com/v3/__https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=pptrs-name-in-attr-key__;!!ACWV5N9M2RV99hQ!NDopQXFVRjDfqgJvGMQRzS3fXyyd1SC4HfGl4dxSMWxNPsNKScecjp7bZYX-dDhL5tatk_xxJFdQwILBe7jj$
> >  
> > 
> > fstests git tree:
> > https://urldefense.com/v3/__https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=pptrs-name-in-attr-key__;!!ACWV5N9M2RV99hQ!NDopQXFVRjDfqgJvGMQRzS3fXyyd1SC4HfGl4dxSMWxNPsNKScecjp7bZYX-dDhL5tatk_xxJFdQwI5iKKgk$
> >  
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       |   66 +++++---
> >  fs/xfs/libxfs/xfs_attr.h       |    5 +
> >  fs/xfs/libxfs/xfs_attr_leaf.c  |   41 ++++-
> >  fs/xfs/libxfs/xfs_da_btree.h   |    6 +
> >  fs/xfs/libxfs/xfs_da_format.h  |   39 ++++-
> >  fs/xfs/libxfs/xfs_fs.h         |    2 
> >  fs/xfs/libxfs/xfs_log_format.h |   31 +++-
> >  fs/xfs/libxfs/xfs_parent.c     |  215 +++++++++++++++++----------
> >  fs/xfs/libxfs/xfs_parent.h     |   46 +++---
> >  fs/xfs/libxfs/xfs_trans_resv.c |    7 +
> >  fs/xfs/scrub/dir.c             |   34 +---
> >  fs/xfs/scrub/dir_repair.c      |   87 +++--------
> >  fs/xfs/scrub/parent.c          |   48 +-----
> >  fs/xfs/scrub/parent_repair.c   |   55 ++-----
> >  fs/xfs/scrub/trace.h           |   65 +-------
> >  fs/xfs/xfs_attr_item.c         |  318
> > +++++++++++++++++++++++++++++++---------
> >  fs/xfs/xfs_attr_item.h         |    3 
> >  fs/xfs/xfs_inode.c             |   30 ++--
> >  fs/xfs/xfs_ondisk.h            |    1 
> >  fs/xfs/xfs_parent_utils.c      |    8 +
> >  fs/xfs/xfs_symlink.c           |    3 
> >  fs/xfs/xfs_xattr.c             |    5 +
> >  22 files changed, 660 insertions(+), 455 deletions(-)
> >