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

Darrick J. Wong
2024-03-29T20:44:53Z

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?
>
> > +/*
> > + * 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