On Mon, Jul 01, 2024 at 11:27:09AM +1000, Dave Chinner wrote: > On Tue, Jun 18, 2024 at 06:06:22PM -0700, Darrick J. Wong wrote: > > On Tue, Jun 18, 2024 at 04:21:12PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > > > xfs_init_new_inode ignores the init_xattrs parameter for filesystems > > > that have ATTR2 enabled but not ATTR. As a result, the first file to be > > > created by the kernel will not have an attr fork created to store acls. > > > Storing that first acl will add ATTR to the superblock flags, so chances > > > are nobody has noticed this previously. > > > > > > However, this is disastrous on a filesystem with parent pointers because > > > it requires that a new linkable file /must/ have a pre-existing attr > > > fork. > > How are we creating a parent pointer filesystem that doesn't have > XFS_SB_VERSION_ATTRBIT set in it? I thought that mkfs.xfs was going > to always set this.... <shrug> The first three versions didn't set ATTRBIT; somewhere between v4 and v5 Allison changed mkfs to set ATTRBIT; and as of v13 it still does. That said, there aren't actually any parent pointers on an empty filesystem because the root dir is empty and the rt/quota inode are children of the superblock. So, technically speaking, it's not *required* for mkfs to set it, at least not until we start adding metadir inodes. At no point during the development of parent pointers has xfs_repair ever validated that ATTRBIT is set if PARENT is enabled -- it only checks that ATTRBIT is set if any attr forks are found. > > > The preproduction version of mkfs.xfs have always set this, but > > > as xfs_sb.c doesn't validate that pptrs filesystems have ATTR set, we > > > must catch this case. > > Which is sounds like it is supposed to - how is parent pointers > getting enabled such that XFS_SB_VERSION_ATTRBIT is not actually > set? Someone who fuzzes the filesystem could turn off ATTRBIT on an empty fs. That's a valid configuration since there are also no parent pointers. Or at least I'm assuming it is since xfs_repair has never complained about ATTRBIT being set on a filesystem with no attr forks, and nobody's suggested adding that enforcement in the 6 years the parent pointer series has been out for review. Getting back to the story, if someone mounts that empty filesystem and tries to create a directory structure, the fs blows up. This patch fixes that situation without adding more ways that mount can fail. > > > Note that the sb verifier /does/ require ATTR2 for V5 filesystems, so we > > > can fix both problems by amending xfs_init_new_inode to set up the attr > > > fork for either ATTR or ATTR2. > > True, but it still seems to me like we should be fixing mkfs.xfs and > the superblock verifier to do the right thing given this is all > still experimental and we're allowed to fix on-disk format bugs > like this. > > Can we add that to the superblock verifier so that the parent > pointer filesystems will not mount if mkfs is not setting the > XFS_SB_VERSION_ATTRBIT correctly when the parent pointer feature is > enabled? > > Worst case is that early testers will need to use xfs_db to set the > XFS_SB_VERSION_ATTRBIT and then the filesystem will mount again... Patches welcome. --D > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
