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

["Darrick J. Wong" <djwong@kernel.org>]

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
>