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! 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(-) >
