Re: [PATCH 1/1] xfs: online repair of symbolic links

Darrick J. Wong
2024-03-29T20:58:55Z

On Fri, Mar 29, 2024 at 01:44:51PM -0700, Darrick J. Wong wrote:
> On Wed, Mar 27, 2024 at 09:53:38AM -0700, Christoph Hellwig wrote:
> > > /* Write the symlink target into the inode. */
> > > int
> > > -xfs_symlink_write_target(
> > > +__xfs_symlink_write_target(
> > > struct xfs_trans *tp,
> > > struct xfs_inode *ip,
> > > + xfs_ino_t owner,
> >
> > The xfs_symlink_write_target/__xfs_symlink_write_target split seems
> > a bit pointless with just a single real caller for either variant.
> > Why not just pass the owner to xfs_symlink_write_target and do away
> > with __xfs_symlink_write_target?

Oops, I forgot to respond to this -- yeah, I'll get rid of this trivial
helper.

--D

> > > +/*
> > > + * Symbolic Link Repair
> > > + * ====================
> > > + *
> > > + * We repair symbolic links by reading whatever target data we can find, up to
> > > + * the first NULL byte. Zero length symlinks are turned into links to the
> > > + * current directory.
> >
> > Are we actually doing that? xrep_setup_symlink sets up a link with
> > the "." target (and could use a comment on why), but we're always
> > writing the long dummy target below now, or am I missing something?
>
> If the target that we salvage has the same strlen as i_size, then we'll
> rewrite what we found into the symlink. In all other cases, yes, we
> write out the DUMMY_TARGET string.
>
> IOWs, the comment is out of date. Here's what I have now:
>
> /*
> * Symbolic Link Repair
> * ====================
> *
> * We repair symbolic links by reading whatever target data we can find, up to
> * the first NULL byte. If the recovered target strlen matches i_size, then
> * we rewrite the target. In all other cases, we replace the target with an
> * overly long string that cannot possibly resolve. The new target is written
> * into a private hidden temporary file, and then a file contents exchange
> * commits the new symlink target to the file being repaired.
> */
>
> > > +/* Set us up to repair the rtsummary file. */
> >
> > I don't think that's what it does :)
> >
> > > + * We cannot use xfs_exchmaps_estimate because we have not yet
> > > + * constructed the replacement rtsummary and therefore do not know how
> > > + * many extents it will use. By the time we do, we will have a dirty
> > > + * transaction (which we cannot drop because we cannot drop the
> > > + * rtsummary ILOCK) and cannot ask for more reservation.
> >
> > No rtsummary here either..
>
> Oops. Fixed both of those things. :(
>
> > > +
> > > +#define DUMMY_TARGET \
> > > + "The target of this symbolic link could not be recovered at all and " \
> > > + "has been replaced with this explanatory message. To avoid " \
> > > + "accidentally pointing to an existing file path, this message is " \
> > > + "longer than the maximum supported file name length. That is an " \
> > > + "acceptable length for a symlink target on XFS but will produce " \
> > > + "File Name Too Long errors if resolved."
> >
> > Haha. Can this cause the repair to run into ENOSPC if the previous
> > corrupted symlink was way shorter?
>
> Yes. In that case, xrep_symlink_rebuild will fail to write DUMMY_TARGET
> into sc->tempip, we ifree the tempfile (with its '.' target), and return
> the error to userspace.
>
> --D
>