On Wed, Dec 06, 2023 at 09:41:48PM -0800, Christoph Hellwig wrote: > On Wed, Dec 06, 2023 at 06:42:44PM -0800, Darrick J. Wong wrote: > > #define XFS_DFORK_DPTR(dip) \ > > - ((char *)dip + xfs_dinode_size(dip->di_version)) > > + ((void *)dip + xfs_dinode_size(dip->di_version)) > > #define XFS_DFORK_APTR(dip) \ > > (XFS_DFORK_DPTR(dip) + XFS_DFORK_BOFF(dip)) > > #define XFS_DFORK_PTR(dip,w) \ > > > + if (XFS_DFORK_BOFF(dip) >= mp->m_sb.sb_inodesize) > > xchk_ino_set_corrupt(sc, ino); > > This should be a prep patch. Done. > Otherwise I'm still a bit worried about the symlink pointing to ? > and suspect we need a clear and documented strategy for things that > can change data for applications before doing something like that. For a brief second I thought about adding another ZAPPED health flag, like I just did for the data/attr forks. Then I realized that for symbolic link targets this doesn't make sense because we've lost the target data so there's no extended recovery that can be applied. Unfortunately this leaves me stuck because targets are arbitrary null terminated strings, so there's no bulletproof way to communicate "target has been lost, do not try to follow this path" without risking that the same directory actually contains a file with that name. At this point, we can't even iget the dead symlink to find the parent pointers so we can delete the inode from the directory tree, so that's also not an option. > The rest looks good to me. Oh good. :) --D
