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

[Christoph Hellwig <hch@infradead.org>]

On Wed, Feb 28, 2024 at 12:52:13PM -0800, Darrick J. Wong wrote:
> I overlooked something this morning -- if the caller passes in
> XFS_SCRUB_IFLAG_FORCE_REBUILD, that might be the free space defragmenter
> trying to get us to move the remote target block somewhere else.  For
> that usecase, if the symlink scrub doesn't find any problems and we read
> in exactly i_size bytes, I think we want to write that back to the
> symlink, and not the DUMMY_TARGET.

Yes, I think we really want that :) 

> Something like:
> 
> 	if (FORCE_REBUILD && !CORRUPT) {

Maybe I need to read the code a little more, but shouldn't this
simply be !corrupt?  Or an assert that if it is not corrupt it is
a force rebuild?  Or am I missing a use case for !corrupt &&
!force_rebuild?

> 	/*
> 	 * Change an empty target into a dummy target and clear the symlink
> 	 * target zapped flag.
> 	 */
> 	if (target_buf[0] == 0) {
> 		sc->sick_mask |= XFS_SICK_INO_SYMLINK_ZAPPED;
> 		sprintf(target_buf, DUMMY_TARGET);
> 	}
> 
> Can we allow that without risking truncation making the symlink point to
> some unintended place?

I can't think of anything that would truncated it, what do you have in
mind?