On Fri, Feb 17, 2023 at 08:02:29PM +0000, Allison Henderson wrote: > On Thu, 2023-02-16 at 12:06 -0800, Darrick J. Wong wrote: > > Hi everyone, > > > > This deluge contains all of the additions to the parent pointers > > patchset that I've been working on for the past month. The kernel > > and > > xfsprogs patchsets are based on Allison's v9r2 tag from last week; > > the fstests patches are merely a part of my development tree. To > > recap <snip> > Ermergersh, thats a lot! Thanks for all the hard work. I feel like if > we don't come up with a plan for review though, people may not know > where to start for these deluges! Lets see... if we had to break this > down, I think would divide it up between the existing parent pointers > and the new pptr propositions for ofsck. That's a good place to cleave. > Then further divide it among > kernel space, user space and test case. If I had to pick only one of > these to focus attention on, probably it should be new ofsck changes in > the kernel space, since the rest of the deluge is really contingent on > it. Yup. Though you ought to read through the offline fsck patches too. Those take a very different approach to resolving parent pointers. So much of repair is based on nuking directories that I don't know there's a good way to rebuild them from parent pointers. A thought I had was that when we decide to zap a directory due to problems in the directory blocks themselves, we could them initiate a scan of the parent pointers to try to find all the dirents we can. I ran into problems with that approach because libxfs_iget allocates fresh xfs_inode objects (instead of caching and sharing them like the kernel does) and that made it really hard to scan things in a coherent manner. > So now we've narrowed this down to a few subsets: > > [PATCHSET v9r2d1 0/3] xfs: bug fixes for parent pointers > [PATCHSET v9r2d1 0/4] xfs: rework the GETPARENTS ioctl, If you read through these two patchsets and think they're ok, then either fold the fixes into the main series or tack them on the end, whichever is easier. If you tack them on the end, please add your own SOB tags. > [PATCHSET v9r2d1 00/23] xfs: online fsck support patches > [PATCHSET v9r2d1 0/7] xfs: online repair of directories > [PATCHSET v9r2d1 0/2] xfs: online checking of parent pointers > [PATCHSET v9r2d1 0/3] xfs: online checking of parent pointers > [PATCHSET v9r2d1 0/2] xfs: online checking of directories The fsck functionality exists to prove the point that directory repair is /very/ awkward if we have to update p_diroffset. As such, they focused on getting the main parts right ... but with the obvious problem of making pptrs dependent on online fsck part 1 getting merged. Speaking of which -- can we merge online fsck for 6.4? Please? :) > [PATCHSET v9r2d1 0/5] xfs: encode parent pointer name in xattr key Resolving the questions presented by this series is critical to nailing down the ondisk format and merging the feature. But we'll get to that below. > [PATCHSET v9r2d1 0/3] xfs: use flex arrays for XFS_IOC_GETPARENTS, I'd like to know what you think about converting the ioctl definition to flex arrays instead of the fixed size structs. I'm not sure where to put this series, though. If you decide that you want 'em, then ideally they'd be in xfs_fs.h from the introduction of XFS_IOC_GETPARENTS, but I don't see any point in backporting them around "xfs: rework the GETPARENTS ioctl". (I would be ok if you rolled all of it into patch 25 from the original v9 set.) > Of those, I think "xfs: encode parent pointer name in xattr key" is the > only one that might impact other features since it's changeing the > ondisk format from when we first started the effort years ago. So > probably that might be the best place for people to start since if this > needs to change it might impact some of the other subsets in the > deluge, or even features they are working on if they've based anything > on the existing pptr set. Bingo! The biggest question about the format change is (IMHO) whether we're ok with using a hash function for parent pointer names that don't fit in the attr key space, and which hash? The sha2 family was designed to be collision resistant, but I don't anticipate that will last forever. The hash is computed from (the full name and the child generation number) when the dirent name is longer than 243 bytes. The first 179 bytes of the dirent name are still written in the parent pointer attr name. An attacker would have to find a collision that only changes the last 76 bytes of the dirent name, and they'd have to know the generation number at runtime. (Note: dirent names shorter than 243 bytes are written directly into the parent pointer xattr name, no hashing required.) I /think/ that's good enough, but I'm no cryptanalyst. The alternative would be to change the xattr format so that the namelen field in the leaf structure to encode *only* the name component of the parent pointer. This would lead to a lot of special cased xattr code and probably a lot of bugs and other stupid problems, which is why I didn't take that route. Thoughts? > I feel like a 5 patch subset is a very reasonable thing to ask people > to give their attention to. That way they dont get lost in things like > nits for optimizations that might not even matter if something it > depends on changes. > > For the most part I am ok with changeing the format as long as everyone > is aware and in agreement so that we dont get caught up re-coding > efforts that seem to have stuggled with disagreements now on the scale > of decades. Some of these patches were already very old by the time I > got them! Hheehhe. Same here -- rmap was pretty old by the time I started pushing that for reals. :) > On a side note, there are some preliminary patches of kernel side > parent pointers that are either larp fixes or refactoring not sensitive > to the proposed ofsck changes. These patches a have been floating > around for a while now, so if no one has any gripes, I think just > merging those would help cut down the amount of rebaseing, user space > porting and patch reviewing that goes on for every version. (maybe the > first 1 though 7 of the 28 patch set, if folks are ok with that) I thought about doing that for 6.3, but I found enough bugs in the locking stuff (recall the first bugfix series) that I held back. I'm not sure about the two "Increase <blah>" patches -- they'll bloat kernel structures without a real user for them. <shrug> > I think the shear size of some of these sets tend to work against them, > as people likely cannot afford the time block they present on the > surface. Agreed. At this point, I've worked through enough of the parent pointers code to understand what's going on that I'm ok with merging it once we settle the above question. FWIW the whole series (kernel+xfsprogs+fstests) has been passing my nightly QA farm for a couple of weeks now despite my constant hammering on it, so I think the implementation is ready. > So I think we would do well to find a way to introduce them > at a reasonable pace and keep attention focused on the subsections that > should require more than others, and hopefully keep thing moving in a > progressive direction. I disagree -- I want to merge online fsck part 1 so I can get that out of my dev trees. Then I want to focus on getting this over the finish line and merged. But then I'm not known for incrementalism. :P --D > Thx! > Allison >
