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

Christoph Hellwig
2024-03-27T16:53:38Z

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

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

> +
> +#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?